Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Automatically provide Messenger Doctrine schema to "diff"#36655
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
4ecf58d toc49b512Compareweaverryan commentedMay 1, 2020
Basically, if this makes sense, I will: A) Complete this for Messenger with tests |
1d7afdb tobf481f1Comparesrc/Symfony/Component/Messenger/Bridge/Doctrine/Transport/Connection.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
284d020 to516ef20Compare2e2aa17 to594077aCompareKoc commentedMay 2, 2020
Should we deprecate schema filters fromdoctrine/DoctrineBundle#1037 PR if current will be merged? |
weaverryan commentedMay 2, 2020
@Koc At least for Messenger & Cache, definitely. In fact, I'd argue we should we remove them: this feature will replace them For Lock & PdoSessionStorage, they will probably need to stay, as this PR doesn't fix those situations - at least not all the way. |
566f557 to018ca2eCompareweaverryan commentedMay 2, 2020
This is ready - test failures look unrelated. I'm super happy with the Messenger & Cache integration and very dissatisfied with the Lock & Session integration. My instinct is that we need to save a proper Lock & Session solution for later. Cheers! |
nicolas-grekas 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.
super nice!
fabpot commentedMay 5, 2020
Thank you@weaverryan. |
bendavies commentedMay 7, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@weaverryan what does a migration look like if the postgres messenger trigger is updated, after being migrated for the first time? |
Uh oh!
There was an error while loading.Please reload this page.
This follows this conversation:#36629 (comment) - it automatically adds SQL to Doctrine's migration/diff system when features are added the require a database table:
The new feature works for:
A) Messenger Doctrine transport
FULL support
Works perfectly: configure a doctrine transport and run
make:migrationNote: There is no current way to disable this. So if you have
auto_setupON and yourun
make:migrationbefore trying Messenger, it will generate the table SQL. Adding aflag to disable it might be very complicated, because we need to know (in DoctrineBundle, at compile time) whether or not this feature is enabled/disabled so that we can decidenot to add
messenger_messagesto theschema_filter.B)
PdoAdapterfrom CacheFULL support
Works perfectly: configure a doctrine transport and run
make:migrationC)
PdoStorefrom LockPARTIAL support
I added
PdoStore::configureSchema()but did NOT add a listener. WhilePdoStoredoes accept a DBALConnection, I don't think it's possible via theframework.lockconfig to create aPdoStorethat is passed aConnection. In other words: if we added a listener that calledPdoStore::configureSchemaif the user configured apdolock, that service willnever have aConnectionobject... so it's kind of worthless.NEED: A proper way to inject a DBAL
ConnectionintoPdoStoreviaframework.lockconfig.D)
PdoSessionHandlerNO support
This class doesn't accept a DBAL
Connectionobject. And so, we can't reliably create a listener to add the schema because (if there are multiple connections) we wouldn't know which Connection to use.We could compare (
===) thePDOinstance insidePdoSessionHandlerto the wrappedPDOconnection in Doctrine. That would only work if the user has configured theirPdoSessionHandlerto re-use the Doctrine PDO connection.The
PdoSessionHandleralready has acreateTable()method on it to help with manual migration. But... it's not easy to call from a migration because you would need to fetch thePdoSessionHandlerservice from the container. Adding somethingNEED: Either:
A) A way for
PdoSessionHandlerto use a DBAL Connectionor
B) We try to hack this feature by comparing the
PDOinstances in the event subscriberor
C) We add an easier way to access the
createTable()method from inside a migration.TODOs