Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] Doctrine Transport: Support setting auto_setup from DSN#32392
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
bendavies commentedJul 5, 2019
travis failure seems unrelated. |
| // test options from options array wins over options from dsn | ||
| [ | ||
| 'dsn' =>'doctrine://default?table_name=name_from_dsn&redeliver_timeout=1200&queue_name=normal', | ||
| 'dsn' =>'doctrine://default?table_name=name_from_dsn&redeliver_timeout=1200&queue_name=normal&auto_setup=true', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not use1 and0 instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no reason other than the docs are written withtrue andfalse everywhere, and it matches the yaml.
I can add another test covering that1 and0 are handled correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMHO, supportingtrue andfalse is a mistake (I mean, there are strings). When using a query string, 0 and 1 is the "native" way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
sure, i'm not precious either way, but note that boolean strings are also supported in the AMQP transport, i.eamqp://localhost?persistent=true
what do you want to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
do we have a consistent way to handle DSN's? something like enqueue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think consistency is the key here. If we are already supportingtrue/false elsewhere, let's support it here. Adding support for1/0 would be nice as well.
We don't have a generic way of handling DSNs, but as we are now using them in many different components, it probably makes sense to create something? Could it be a new component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
added covering test for0/1
fabpot left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
auto_setup=1 seems missing in the tests.
bendavies commentedJul 8, 2019
Added specifically. |
fabpot commentedJul 8, 2019
@bendavies Would you like to have a look at creating a |
fabpot commentedJul 8, 2019
Thank you@bendavies. |
… from DSN (bendavies)This PR was squashed before being merged into the 4.3 branch (closes#32392).Discussion----------[Messenger] Doctrine Transport: Support setting auto_setup from DSN| Q | A| ------------- | ---| Branch? | 4.3| Bug fix? | yes| New feature? | no <!-- please update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | <!-- required for new features -->It was not possible to set `auto_setup` via the dsn, as the result would always be a string, resulting in setup always being performed because a bool is required.The same was true if `auto_setup` was provided in `options` as a string.I've fixed ensuring that the final configuration contains a bool for `auto_setup`.Additionally, constructing the `configuration` was overly complex and hard to grok, so I've hopefully simplified it as part of this PR.As an aside the three transports all do configuration construction in different ways with varying styles. It would be nice to neaten them up.Commits-------213dfd1 [Messenger] Doctrine Transport: Support setting auto_setup from DSN
bendavies commentedJul 10, 2019
@fabpot it seems there is a good usecase for a new component. |
Simperfit commentedJul 10, 2019
@bendavies@fabpot I have time to do it if you want to :) |
Uh oh!
There was an error while loading.Please reload this page.
It was not possible to set
auto_setupvia the dsn, as the result would always be a string, resulting in setup always being performed because a bool is required.The same was true if
auto_setupwas provided inoptionsas a string.I've fixed ensuring that the final configuration contains a bool for
auto_setup.Additionally, constructing the
configurationwas overly complex and hard to grok, so I've hopefully simplified it as part of this PR.As an aside the three transports all do configuration construction in different ways with varying styles. It would be nice to neaten them up.