[Messenger] Doctrine Transport: Support setting auto_setup from DSN #32392
Conversation
|
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.
no reason other than the docs are written with true and false everywhere, and it matches the yaml.
I can add another test covering that 1 and 0 are handled correctly?
There was a problem hiding this comment.
IMHO, supporting true and false 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.
sure, i'm not precious either way, but note that boolean strings are also supported in the AMQP transport, i.e amqp://localhost?persistent=true
what do you want to do?
There was a problem hiding this comment.
do we have a consistent way to handle DSN's? something like enqueue
There was a problem hiding this comment.
I think consistency is the key here. If we are already supporting true/false elsewhere, let's support it here. Adding support for 1/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.
added covering test for 0/1
|
Added specifically. |
|
@bendavies Would you like to have a look at creating a |
|
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 <!-- see https://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
@fabpot it seems there is a good usecase for a new component. |
|
@bendavies @fabpot I have time to do it if you want to :) |
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.