Skip to content

[Lock] Split PdoStore into DoctrineDbalStore #43332

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
Oct 19, 2021
Merged

[Lock] Split PdoStore into DoctrineDbalStore #43332

merged 1 commit into from
Oct 19, 2021

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Oct 6, 2021 •
edited
Loading

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? yes
Tickets Fix #42962
License MIT
Doc PR WIP

Main changes:

  • New: Created classes DoctrineDbalStore and DoctrineDbalPostgreSqlStore.
  • Deprecated: Using Doctrine\DBAL\Connection or <driver>:// DSN in PdoStore or PostgreSqlStore is deprecated in 5.4 and will be removed in 6.0. Use DoctrineDbalStore or DoctrineDbalPostgreSqlStore instead.
  • New: Mysqli driver is supported by DoctrineDbalStore (thanks to numerical params)
  • BC: When using a malformed DBAL DSN, the error will be thrown when the DoctrineDbalStore or DoctrineDbalPostgreSqlStore is instanced, instead of the first usage of the connection.

@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@GromNaN GromNaN force-pushed the dbal/lock-pdo-5.4 branch from 7e5c686 to 70d9543 Compare October 6, 2021 07:14
@carsonbot carsonbot closed this Oct 6, 2021
@nicolas-grekas nicolas-grekas reopened this Oct 6, 2021
@GromNaN GromNaN force-pushed the dbal/lock-pdo-5.4 branch from 70d9543 to 54d687b Compare October 6, 2021 08:19
stof
stof reviewed Oct 6, 2021
View reviewed changes
@derrabus
Copy link
Member

derrabus commented Oct 6, 2021

I think, we can simplify PdoStore already:

  • PdoStore gets a new private property $dbalStore.
  • If PdoStore is constructed with a DBAL connection instance or a DBAL-compatible DSN, a deprecation is triggered and $dbalStore is initialized with a DbalStore instance.
  • In all public methods, we check if $dbalStore is not null and if that is the case, we simply proxy the call to $dbalStore.

This should allow us to remove all DBAL-related code from PdoStore already. And on 6.0, we simply remove the proxy mechanism.

@derrabus
Copy link
Member

derrabus commented Oct 6, 2021

Oh, and thank you very much for working on this topic!

@GromNaN GromNaN marked this pull request as ready for review October 6, 2021 15:43
@GromNaN GromNaN requested a review from jderusse as a code owner October 6, 2021 15:43
@carsonbot carsonbot added this to the 5.4 milestone Oct 6, 2021
@GromNaN GromNaN force-pushed the dbal/lock-pdo-5.4 branch 2 times, most recently from 39bcf0c to 7922f8b Compare October 6, 2021 17:44
@GromNaN GromNaN force-pushed the dbal/lock-pdo-5.4 branch from 7922f8b to e1b3c34 Compare October 6, 2021 17:46
@GromNaN GromNaN changed the title [Lock] Split PdoStore into DbalStore [Lock] Split PdoStore into DoctrineDbalStore Oct 6, 2021
@GromNaN GromNaN force-pushed the dbal/lock-pdo-5.4 branch 3 times, most recently from 6a80a44 to f7d493a Compare October 7, 2021 00:03
@GromNaN GromNaN force-pushed the dbal/lock-pdo-5.4 branch 2 times, most recently from 952ca0e to af32763 Compare October 7, 2021 07:43
@GromNaN GromNaN requested a review from derrabus October 7, 2021 07:44
@GromNaN GromNaN force-pushed the dbal/lock-pdo-5.4 branch from af32763 to 53f84af Compare October 7, 2021 07:49
@derrabus
Copy link
Member

derrabus commented Oct 7, 2021

Can you take a look at the failing tests?

derrabus
derrabus approved these changes Oct 7, 2021
View reviewed changes
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

Great work, thank you!

@fabpot
Copy link
Member

fabpot commented Oct 19, 2021

Thank you @GromNaN.

@fabpot fabpot force-pushed the dbal/lock-pdo-5.4 branch from 14d99b6 to 6dcaae1 Compare October 19, 2021 07:18
@fabpot fabpot merged commit aec94ef into symfony:5.4 Oct 19, 2021
@fabpot fabpot mentioned this pull request Oct 19, 2021
Closed
@JohJohan JohJohan mentioned this pull request Oct 20, 2021
Closed
This was referenced Nov 5, 2021
Merged
Merged
@martinssipenko
Copy link
Contributor

The new DoctrineDbalStore does not create the table automatically in case it's not present, was this done intentionally?

cc @GromNaN @derrabus

@GromNaN
Copy link
Member Author

GromNaN commented Nov 30, 2021

The new DoctrineDbalStore does not create the table automatically in case it's not present, was this done intentionally?

This is actually a regression.

@martinssipenko
Copy link
Contributor

@GromNaN alright, I've created an issue (#44369) and will make an attempt to fix it.

@GromNaN GromNaN deleted the dbal/lock-pdo-5.4 branch December 1, 2021 10:26
@HypeMC HypeMC mentioned this pull request Nov 5, 2023
Merged
nicolas-grekas added a commit that referenced this pull request Nov 20, 2023
@nicolas-grekas
bug #52459 [Cache][HttpFoundation][Lock] Fix PDO store not creating t…
48be4b3 
…able + add tests (HypeMC)

This PR was merged into the 5.4 branch.

Discussion
----------

[Cache][HttpFoundation][Lock] Fix PDO store not creating table + add tests

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | -
| License       | MIT

Unlike e.g. SQLite, PostgreSQL doesn't throw an exception on `$conn->prepare()` if the table is missing, it instead throws it on `$stmt->execute()`, so the table never gets created.

The table used to get created, but the behavior was broken with #43332.

Commits
-------

cb5d832 [Cache][Lock] Fix PDO store not creating table + add tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@derrabus derrabus derrabus approved these changes

@jderusse jderusse Awaiting requested review from jderusse jderusse is a code owner

Assignees
No one assigned
Labels
Deprecation Feature Lock Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

Split PDO and DBAL adapters
7 participants
@GromNaN @carsonbot @derrabus @fabpot @martinssipenko @stof @nicolas-grekas