Skip to content

Moving the AMQP exchange from env var to config #608

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Moving the AMQP exchange from env var to config #608

wants to merge 1 commit into from

Conversation

weaverryan
Copy link
Member

@weaverryan weaverryan commented Jun 10, 2019

Q A
License MIT

This should make it easier (more obvious how) to specify multiple transports using the same
AMQP connection config, but with different exchanges. Using a different exchange for each transport is the "main" way of supporting multiple transports (you can also use 1 exchange with multiple queues, but then it's up to you to change the exchange type to direct and manage binding and routing keys).

Docs issue: symfony/symfony-docs#11713

From Twitter convo: https://twitter.com/webdevilopers/status/1136177625406332928?s=21

Cheers!

This should make it easier to specify multiple transports using the same
AMQP connection config, but with different exchanges.
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request passes validation.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request passes validation.

@@ -5,7 +5,7 @@ framework:

transports:
# https://symfony.com/doc/current/messenger.html#transports
# async: '%env(MESSENGER_TRANSPORT_DSN)%'
# async: '%env(MESSENGER_TRANSPORT_DSN)%/messages_exchange'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe using ?exchange[name]=messages is more self-explaining. Using the path for the virtual host and exchange is not self-explaining at all.

Copy link
Contributor

@Tobion Tobion Jun 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or using the config options directly below the dsn. this would also make sure that options passed in the dsn would keep working. Something like

async:
    dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
    options:
        exchange:
            name: messages

@webdevilopers
Copy link

This indeed makes it more obvious! 👍

I'm no Redis user but does this change apply to the "messages" suffix too?
MESSENGER_TRANSPORT_DSN=redis://localhost:6379/messages
Or is this a general "collector"?

I also like the suggestions by @Tobion . I guess this should be added to docs in a different PR.

async:
    dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
    options:
        exchange:
            name: messages

He states this would also make sure that options passed in the dsn would keep working.
If I remember correctly setting a custom exchange in the config will not be "recognized" as long as the default exchange was set inside the env?

@weaverryan
Copy link
Member Author

Sorry for the delay in replying!

I realized, thanks to your suggestion, that my PR in fact does not work. The problem is that this async transport is currently written to work with any transport that you configure in .env - amqp, but also maybe doctrine or redis.

So, I'm not sure what to do. the only way to make messenger.yaml fully work for any transport is for 100% of the connection config to be in .env. My ideas include:

A) Do nothing. Close this PR, and update the AMQP documentation to help people.
B) Create 3 different async examples in messenger.yaml, one for redis, doctrine & amqp. I'm not sure I love this - that's a lot of noise in that file, and you'd now need to know which example to uncomment.

@fabpot
Copy link
Member

fabpot commented Jun 19, 2019

I would be in favor of option A.

@maxhelias
Copy link
Contributor

I realized, thanks to your suggestion, that my PR in fact does not work. The problem is that this async transport is currently written to work with any transport that you configure in .env - amqp, but also maybe doctrine or redis.

Normally every transport has his own parser for support this or i'm wrong ?

@weaverryan
Copy link
Member Author

Normally every transport has his own parser for support this or i'm wrong

You're right - every transport supports different options as query parameters (or under the options key). The problem is that the amqp transport in messenger.yaml is meant to be "generic" - it's meant to work out-of-the-box with any transport. And so, it can't contain any transport-specific options.

I'm going to open a docs issue about this, and will hopefully document it soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants