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
Add support for new MessengerTransportDoctrineSchemaSubscriber #1163
Add support for new MessengerTransportDoctrineSchemaSubscriber #1163
Conversation
Have you considered just adding composer conflict rule for messenger < 5.1? I wouldn't mind doing that personally, as 2.1 is exclusively targetted to bring support for symfony 5.1 components and it's release date will be synced with symfony 5.1 release. |
Well, that would mean that any project using the 4.4 LTS won't be able to upgrade to DoctrineBundle 2.1, and so Doctrine will have to provide LTS support for 2.0.x of the bundle. I'd rather keep support for 4.4 and 5.x of Symfony in the same maintained version of the bundle (meaning that we don't have to support 2 branches, and Symfony 4.4 users still benefit from future improvements of the bundle). |
…ff" (weaverryan) This PR was squashed before being merged into the 5.1-dev branch. Discussion ---------- Automatically provide Messenger Doctrine schema to "diff" | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Alternative to #36629 | License | MIT | Doc PR | TODO - WILL be needed 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 you run `make:migration` before trying Messenger, it will generate the table SQL. Adding a flag 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 the `schema_filter`. ### B) `PdoAdapter` from Cache **FULL support** Works perfectly: configure a doctrine transport and run `make:migration` ### C) `PdoStore` from Lock **PARTIAL support** I added `PdoStore::configureSchema()` but did NOT add a listener. While `PdoStore` *does* accept a DBAL `Connection`, I don't think it's possible via the `framework.lock` config to create a `PdoStore` that is passed a `Connection`. In other words: if we added a listener that called `PdoStore::configureSchema` if the user configured a `pdo` lock, that service will *never* have a `Connection` object... so it's kind of worthless. **NEED**: A proper way to inject a DBAL `Connection` into `PdoStore` via `framework.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 (`===`) the `PDO` instance inside `PdoSessionHandler` to the wrapped `PDO` connection in Doctrine. That would only work if the user has configured their `PdoSessionHandler` to re-use the Doctrine PDO connection. The `PdoSessionHandler` *already* has a `createTable()` method on it to help with manual migration. But... it's not easy to call from a migration because you would need to fetch the `PdoSessionHandler` service from the container. Adding something **NEED**: Either: A) A way for `PdoSessionHandler` to use a DBAL Connection or B) We try to hack this feature by comparing the `PDO` instances in the event subscriber or C) We add an easier way to access the `createTable()` method from inside a migration. TODOs * [X] Determine service injection XML needed for getting all PdoAdapter pools * [ ] Finish DoctrineBundle PR: doctrine/DoctrineBundle#1163 Commits ------- 2dd9c3c Automatically provide Messenger Doctrine schema to "diff"
…ff" (weaverryan) This PR was squashed before being merged into the 5.1-dev branch. Discussion ---------- Automatically provide Messenger Doctrine schema to "diff" | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Alternative to #36629 | License | MIT | Doc PR | TODO - WILL be needed This follows this conversation: symfony/symfony#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 you run `make:migration` before trying Messenger, it will generate the table SQL. Adding a flag 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 the `schema_filter`. ### B) `PdoAdapter` from Cache **FULL support** Works perfectly: configure a doctrine transport and run `make:migration` ### C) `PdoStore` from Lock **PARTIAL support** I added `PdoStore::configureSchema()` but did NOT add a listener. While `PdoStore` *does* accept a DBAL `Connection`, I don't think it's possible via the `framework.lock` config to create a `PdoStore` that is passed a `Connection`. In other words: if we added a listener that called `PdoStore::configureSchema` if the user configured a `pdo` lock, that service will *never* have a `Connection` object... so it's kind of worthless. **NEED**: A proper way to inject a DBAL `Connection` into `PdoStore` via `framework.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 (`===`) the `PDO` instance inside `PdoSessionHandler` to the wrapped `PDO` connection in Doctrine. That would only work if the user has configured their `PdoSessionHandler` to re-use the Doctrine PDO connection. The `PdoSessionHandler` *already* has a `createTable()` method on it to help with manual migration. But... it's not easy to call from a migration because you would need to fetch the `PdoSessionHandler` service from the container. Adding something **NEED**: Either: A) A way for `PdoSessionHandler` to use a DBAL Connection or B) We try to hack this feature by comparing the `PDO` instances in the event subscriber or C) We add an easier way to access the `createTable()` method from inside a migration. TODOs * [X] Determine service injection XML needed for getting all PdoAdapter pools * [ ] Finish DoctrineBundle PR: doctrine/DoctrineBundle#1163 Commits ------- 2dd9c3c3c8 Automatically provide Messenger Doctrine schema to "diff"
…ff" (weaverryan) This PR was squashed before being merged into the 5.1-dev branch. Discussion ---------- Automatically provide Messenger Doctrine schema to "diff" | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Alternative to #36629 | License | MIT | Doc PR | TODO - WILL be needed This follows this conversation: symfony/symfony#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 you run `make:migration` before trying Messenger, it will generate the table SQL. Adding a flag 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 the `schema_filter`. ### B) `PdoAdapter` from Cache **FULL support** Works perfectly: configure a doctrine transport and run `make:migration` ### C) `PdoStore` from Lock **PARTIAL support** I added `PdoStore::configureSchema()` but did NOT add a listener. While `PdoStore` *does* accept a DBAL `Connection`, I don't think it's possible via the `framework.lock` config to create a `PdoStore` that is passed a `Connection`. In other words: if we added a listener that called `PdoStore::configureSchema` if the user configured a `pdo` lock, that service will *never* have a `Connection` object... so it's kind of worthless. **NEED**: A proper way to inject a DBAL `Connection` into `PdoStore` via `framework.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 (`===`) the `PDO` instance inside `PdoSessionHandler` to the wrapped `PDO` connection in Doctrine. That would only work if the user has configured their `PdoSessionHandler` to re-use the Doctrine PDO connection. The `PdoSessionHandler` *already* has a `createTable()` method on it to help with manual migration. But... it's not easy to call from a migration because you would need to fetch the `PdoSessionHandler` service from the container. Adding something **NEED**: Either: A) A way for `PdoSessionHandler` to use a DBAL Connection or B) We try to hack this feature by comparing the `PDO` instances in the event subscriber or C) We add an easier way to access the `createTable()` method from inside a migration. TODOs * [X] Determine service injection XML needed for getting all PdoAdapter pools * [ ] Finish DoctrineBundle PR: doctrine/DoctrineBundle#1163 Commits ------- 2dd9c3c3c8 Automatically provide Messenger Doctrine schema to "diff"
e96d4e6
to
7a6308f
Compare
Pending tests, this is ready! It works well on a real project and I managed to get a nice "integration" test on the cache compiler pass, which is a bit more complex. |
64a3ab2
to
128b799
Compare
Tests are green :) |
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.
Nice thanks !
128b799
to
d7b61d6
Compare
Thanks @weaverryan! |
For symfony/symfony#36655
This adds service integration for 2 new schema subscribers. They allow
doctrine:schema:update
to automatically see tables needed by Messenger & cache.I've tested this in a real project and it works beautifully: the table show up only when activated (i.e. when using a
doctrine
transport in messenger or configuring a real cache pool that uses thepdo
adapter.