Doctrine messenger schema #36629

fabpot · 2020-04-29T21:09:19Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	n/a
License	MIT
Doc PR	n/a

When using the Doctrine Messenger integration, auto_setup is true by default meaning that the table is created on the fly.

When using Doctrine Migrations, that's an issue as the table is not managed by migrations, so a make:migration will include the removal of the table. There are workarounds.

But more importantly, if you're using some Doctrine bundle to wrap your tests with transactions to speed them up, things will break. auto_setup will set itself to false after creating the table, but for the next test, the table will be wiped out. And that's just the beginning of more issues.

Fixing it the "proper way" involves setting auto_setup to false and creating a proper migration. The problem comes from the fact that the schema is not exposed, so you need to recreate it yourself.

So, I think we need to get rid of that auto_setup flag which does more harm than good.

This PR does a first step in that direction by exposing the schema changes so that you can create a migration as a one liner (thanks to make:migration):

namespace DoctrineMigrations;

use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;
use Symfony\Component\Messenger\Bridge\Doctrine\Transport\Migration as MessengerMigration;

final class VersionXXX extends AbstractMigration
{
    public function up(Schema $schema): void
    {
        $m = new MessengerMigration();
        $m->up($schema, $this->connection);
    }

    public function down(Schema $schema): void
    {
        // don't get me started with down() in migrations :)
    }
}

We could even imagine having it generated automatically via make:migration --messenger?

Less magic and good integration with best practices is a nice bonus.

fabpot · 2020-04-29T21:11:43Z

If you agree with the general idea (not sure about the names), we could/should do it for other places where we are currently using the same strategy of generating tables on the fly (caches, ...).

weaverryan · 2020-04-29T21:20:49Z

And yes, auto_setup is super fun.... but SUPER magic. The only improvement I can think of is this... which may not even be possible, but it seems like the ideal flow:

A) I activate Messenger with a Doctrine dsn
B) Messenger (via DoctrineBundle I guess) somehow makes Doctrine aware of the new schema
C) make:migration naturally generates the SQL needed thanks to the new schema that Messenger is telling it about.

Also, with this solution, we would still need the workaround in DoctrineBundle to avoid make:migration from generating a DROP TABLE in future migrations. My imaginary solution would also handle that... if it's possible...

src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/Migration.php

fabpot · 2020-04-30T05:59:28Z

One decision to make: where to "manage" these migrations. Schema manipulations are in DBAL, so that's the lower level where we could do something. But it could also be part of a higher level of abstraction: DoctrineBundle, Migration, MigrationBundle, MakerBundle.

weaverryan · 2020-04-30T10:21:30Z

Hi again o/

I just talked with Nicolas. I think we have a "best of all worlds" idea. But also, I see no harm in exposing the Schema like in this PR in addition to (hopefully) an extra solution.

Idea:

We create some new interface + tag that systems can implement to "advertise" the "Schema" that they want to add to the system - e.g. DoctrineSchemaProviderInterface. This would live in DoctrineBridge.

use Doctrine\DBAL\Schema;

interface DoctrineSchemaProviderInterface
{
    public function getSchema(): Schema;

    // this is a "maybe" - would be used for the pgsql case of the trigger SQL
    // I think it may be possible to inject it
    public function getExtraSql(Table $table, AbstractPlatform $platform): string
}

We create a class that implements this for each affected part in Symfony. Each implementation would live in the correct component/package (e.g. a MessengerDoctrineSchemaProvider inside of symfony/doctrine-messenger)
In DoctrineBundle, we register these services and give each of them a new tag.
In DoctrineBundle, we create a listener to ToolEvents::postGenerateSchema(). This "hooks into" the "diff" process. In this listener, we loop over all the services from (2) via the tag and dynamically add their Schema.

Then, things like make:diff "just work" AND we avoid the "environment variable" problem: we need to do all this resolution at runtime so we have the final env vars.

I'll work on this tomorrow (Friday)

Cheers!

src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/Migration.php

weaverryan · 2020-05-01T15:31:42Z

Here's the proof of concept - it was a bit simpler than I expected: #36655

nicolas-grekas · 2020-05-04T16:50:30Z

Closing in favor of #36655

…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"

fabpot requested a review from sroze as a code owner April 29, 2020 21:09

carsonbot added Status: Needs Review Feature labels Apr 29, 2020

fabpot force-pushed the doctrine-messenger-schema branch 2 times, most recently from 77f8d48 to 611189e Compare April 29, 2020 21:16

dunglas reviewed Apr 29, 2020

View reviewed changes

src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/Migration.php Show resolved Hide resolved

fabpot force-pushed the doctrine-messenger-schema branch 2 times, most recently from 9c24923 to ca4e016 Compare April 30, 2020 05:47

fabpot force-pushed the doctrine-messenger-schema branch from ca4e016 to 21268f9 Compare April 30, 2020 06:28

[Messenger] Expose the Doctrine Schema to help with migrations

8c990f0

fabpot force-pushed the doctrine-messenger-schema branch from 21268f9 to 8c990f0 Compare April 30, 2020 06:29

Shine-neko reviewed Apr 30, 2020

View reviewed changes

src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/Migration.php Show resolved Hide resolved

weaverryan mentioned this pull request May 1, 2020

Automatically provide Messenger Doctrine schema to "diff" #36655

Merged

2 tasks

nicolas-grekas modified the milestones: next, 5.1 May 1, 2020

nicolas-grekas closed this May 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Doctrine messenger schema #36629

Doctrine messenger schema #36629

fabpot commented Apr 29, 2020 •

edited

Loading

fabpot commented Apr 29, 2020

weaverryan commented Apr 29, 2020

fabpot commented Apr 30, 2020 •

edited

Loading

weaverryan commented Apr 30, 2020 •

edited

Loading

weaverryan commented May 1, 2020

nicolas-grekas commented May 4, 2020

Doctrine messenger schema #36629

Doctrine messenger schema #36629

Conversation

fabpot commented Apr 29, 2020 • edited Loading

fabpot commented Apr 29, 2020

weaverryan commented Apr 29, 2020

fabpot commented Apr 30, 2020 • edited Loading

weaverryan commented Apr 30, 2020 • edited Loading

weaverryan commented May 1, 2020

nicolas-grekas commented May 4, 2020

fabpot commented Apr 29, 2020 •

edited

Loading

fabpot commented Apr 30, 2020 •

edited

Loading

weaverryan commented Apr 30, 2020 •

edited

Loading