-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4ecf58d
to
c49b512
Compare
Basically, if this makes sense, I will: A) Complete this for Messenger with tests |
1d7afdb
to
bf481f1
Compare
src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/Connection.php
Outdated
Show resolved
Hide resolved
284d020
to
516ef20
Compare
2e2aa17
to
594077a
Compare
Should we deprecate schema filters from doctrine/DoctrineBundle#1037 PR if current will be merged? |
@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
to
018ca2e
Compare
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! |
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!
baf34be
to
2dd9c3c
Compare
Thank you @weaverryan. |
@weaverryan what does a migration look like if the postgres messenger trigger is updated, after being migrated for the first time? |
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:migration
Note: There is no current way to disable this. So if you have
auto_setup
ON and yourun
make:migration
before 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 decide not to add
messenger_messages
to theschema_filter
.B)
PdoAdapter
from CacheFULL support
Works perfectly: configure a doctrine transport and run
make:migration
C)
PdoStore
from LockPARTIAL support
I added
PdoStore::configureSchema()
but did NOT add a listener. WhilePdoStore
does accept a DBALConnection
, I don't think it's possible via theframework.lock
config to create aPdoStore
that is passed aConnection
. In other words: if we added a listener that calledPdoStore::configureSchema
if the user configured apdo
lock, that service will never have aConnection
object... so it's kind of worthless.NEED: A proper way to inject a DBAL
Connection
intoPdoStore
viaframework.lock
config.D)
PdoSessionHandler
NO support
This class doesn't accept a DBAL
Connection
object. 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 (
===
) thePDO
instance insidePdoSessionHandler
to the wrappedPDO
connection in Doctrine. That would only work if the user has configured theirPdoSessionHandler
to re-use the Doctrine PDO connection.The
PdoSessionHandler
already 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 thePdoSessionHandler
service from the container. Adding somethingNEED: Either:
A) A way for
PdoSessionHandler
to use a DBAL Connectionor
B) We try to hack this feature by comparing the
PDO
instances in the event subscriberor
C) We add an easier way to access the
createTable()
method from inside a migration.TODOs