Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged

Conversation

weaverryan
Copy link
Member

@weaverryanweaverryan commentedMay 1, 2020
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
Deprecations?no
TicketsAlternative to#36629
LicenseMIT
Doc PRTODO - 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 runmake:migration

Note: There is no current way to disable this. So if you haveauto_setup ON and you
runmake: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 decidenot to addmessenger_messages to theschema_filter.

B)PdoAdapter from Cache

FULL support
Works perfectly: configure a doctrine transport and runmake:migration

C)PdoStore from Lock

PARTIAL support
I addedPdoStore::configureSchema() but did NOT add a listener. WhilePdoStoredoes 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 willnever have aConnection object... so it's kind of worthless.

NEED: A proper way to inject a DBALConnection intoPdoStore viaframework.lock config.

D)PdoSessionHandler

NO support

This class doesn't accept a DBALConnection 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.

ThePdoSessionHandleralready 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 something

NEED: Either:

A) A way forPdoSessionHandler to use a DBAL Connection
or
B) We try to hack this feature by comparing thePDO instances in the event subscriber
or
C) We add an easier way to access thecreateTable() method from inside a migration.

TODOs

Devristo and andreybolonin reacted with hooray emoji
@weaverryan
Copy link
MemberAuthor

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

@weaverryanweaverryanforce-pushed thedoctrine-schema-provider branch 3 times, most recently from1d7afdb tobf481f1CompareMay 1, 2020 18:48
@weaverryanweaverryanforce-pushed thedoctrine-schema-provider branch from284d020 to516ef20CompareMay 1, 2020 19:52
@weaverryanweaverryan changed the title[WIP] Automatically provide Messenger Doctrine schema to "diff"Automatically provide Messenger Doctrine schema to "diff"May 1, 2020
@nicolas-grekasnicolas-grekas added this to thenext milestoneMay 1, 2020
@weaverryanweaverryanforce-pushed thedoctrine-schema-provider branch 3 times, most recently from2e2aa17 to594077aCompareMay 2, 2020 10:27
@Koc
Copy link
Contributor

Koc commentedMay 2, 2020

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

@weaverryan
Copy link
MemberAuthor

Should we deprecate schema filters fromdoctrine/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
Copy link
MemberAuthor

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!

@weaverryanweaverryan modified the milestones:next,5.1May 4, 2020
Copy link
Member

@nicolas-grekasnicolas-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!

@fabpotfabpotforce-pushed thedoctrine-schema-provider branch frombaf34be to2dd9c3cCompareMay 5, 2020 06:14
@fabpot
Copy link
Member

Thank you@weaverryan.

@fabpotfabpot merged commitb9d4149 intosymfony:masterMay 5, 2020
@weaverryanweaverryan deleted the doctrine-schema-provider branchMay 5, 2020 10:19
@fabpotfabpot mentioned this pull requestMay 5, 2020
@bendavies
Copy link
Contributor

bendavies commentedMay 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?

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@srozesrozeAwaiting requested review from sroze

@jderussejderusseAwaiting requested review from jderusse

Assignees
No one assigned
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

[8]ページ先頭

©2009-2025 Movatter.jp