Skip to content
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

[Lock] Split PdoStore into DoctrineDbalStore #43332

Merged
merged 1 commit into from
Oct 19, 2021

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Oct 6, 2021

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

src/Symfony/Component/Lock/Store/DbalStore.php Outdated Show resolved Hide resolved
src/Symfony/Component/Lock/Store/DbalStore.php Outdated Show resolved Hide resolved
src/Symfony/Component/Lock/Store/DbalStore.php Outdated Show resolved Hide resolved
src/Symfony/Component/Lock/Store/DbalStore.php Outdated Show resolved Hide resolved
src/Symfony/Component/Lock/Store/PdoStore.php Outdated Show resolved Hide resolved
@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 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
@derrabus
Copy link
Member

derrabus commented Oct 7, 2021

Can you take a look at the failing tests?

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.

@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
nicolas-grekas added a commit that referenced this pull request Nov 20, 2023
…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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split PDO and DBAL adapters
7 participants