8000 Automatically provide Messenger Doctrine schema to "diff" by weaverryan · Pull Request #36655 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 5, 2020
Merged

Automatically provide Messenger Doctrine schema to "diff" #36655

merged 1 commit into from
May 5, 2020

Conversation

@weaverryan
Copy link
Member
@weaverryan weaverryan commented May 1, 2020 •
edited
Loading
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

@weaverryan weaverryan requested a review from sroze as a code owner May 1, 2020 15:31
@weaverryan weaverryan mentioned this pull request May 1, 2020
Closed
@weaverryan weaverryan force-pushed the doctrine-schema-provider branch from 4ecf58d to c49b512 Compare May 1, 2020 15:33
@weaverryan
Copy link
Member Author

Basically, if this makes sense, I will:

A) Complete this for Messenger with tests
B) Propose the DoctrineBundle PR
C) (At least) make a list of the other areas in the system that need this type of thing

@weaverryan weaverryan force-pushed the doctrine-schema-provider branch 3 times, most recently from 1d7afdb to bf481f1 Compare May 1, 2020 18:48
@weaverryan weaverryan force-pushed the doctrine-schema-provider branch from 284d020 to 516ef20 Compare May 1, 2020 19:52
@weaverryan weaverryan changed the title [WIP] Automatically provide Messenger Doctrine schema to "diff" Automatically provide Messenger Doctrine schema to "diff" May 1, 2020
@nicolas-grekas nicolas-grekas added this to the next milestone May 1, 2020
@weaverryan weaverryan force-pushed the doctrine-schema-provider branch 3 times, most recently from 2e2aa17 to 594077a Compare May 2, 2020 10:27
@Koc
Copy link
Contributor
Koc commented May 2, 2020

Should we deprecate schema filters from doctrine/DoctrineBundle#1037 PR if current will be merged?

@weaverryan weaverryan requested a review from jderusse as a code owner May 2, 2020 14:03
@weaverryan
Copy link
Member Author

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.

@weaverryan weaverryan force-pushed the doctrine-schema-provider branch from 566f557 to 018ca2e Compare May 2, 2020 14:27
@weaverryan weaverryan mentioned this pull request May 2, 2020
Merged
@weaverryan
Copy link
Member Author

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!

@weaverryan weaverryan modified the milestones: next, 5.1 May 4, 2020
nicolas-grekas
nicolas-grekas approved these changes May 4, 2020
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nice!

@fabpot fabpot force-pushed the doctrine-schema-provider branch from baf34be to 2dd9c3c Compare May 5, 2020 06:14
@fabpot
Copy link
Member
fabpot commented May 5, 2020

Thank you @weaverryan.

@fabpot fabpot mentioned this pull request May 5, 2020
Closed
@fabpot fabpot merged commit b9d4149 into symfony:master May 5, 2020
@weaverryan weaverryan deleted the doctrine-schema-provider branch May 5, 2020 10:19
@fabpot fabpot mentioned this pull request May 5, 2020
Merged
@bendavies
Copy link
Contributor
bendavies commented May 7, 2020 •
edited
Loading

@weaverryan what does a migration look like if the postgres messenger trigger is updated, after being migrated for the first time?

@ostrolucky ostrolucky mentioned this pull request Jun 8, 2020
Closed
@weaverryan weaverryan mentioned this pull request Feb 12, 2021
Merged
@greg0ire greg0ire mentioned this pull request Jan 26, 2023
Closed
@DavidGarciaCat DavidGarciaCat mentioned this pull request Jan 26, 2023
Closed
This was referenced May 19, 2023
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@sroze sroze Awaiting requested review from sroze

@jderusse jderusse Awaiting requested review from jderusse

Assignees

No one assigned

Labels

Feature Ready Status: Reviewed

Projects

None yet

Milestone

5.1

Development

Successfully merging this pull request may close these issues.

6 participants

@weaverryan @Koc @fabpot @bendavies @nicolas-grekas @carsonbot
0