NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Lock] Use platform to identify the PDO driver#43281

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

Jump to bottom

Merged

derrabus merged 1 commit intosymfony:5.4fromJean85:fix-lock-dbal-driver

Oct 4, 2021

Merged

[Lock] Use platform to identify the PDO driver#43281

derrabus merged 1 commit intosymfony:5.4fromJean85:fix-lock-dbal-driver

Oct 4, 2021

Conversation

Copy link

Contributor

Jean85 commentedOct 1, 2021

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix#43048
License	MIT

This should fix the issue in the same way we did for#42011. I'm not sure if I should add/change any test...

Jean85 requested a review fromjderusse as acode owner

October 1, 2021 20:02

carsonbot added Status: Needs Review Bug Lock labels

Oct 1, 2021

carsonbot added this to the5.4 milestone

Oct 1, 2021

Jean85 mentioned this pull request

Oct 1, 2021

PostgreSqlStore breaks Sentry usage due to hard-coded instanceof checks.#43048

Closed

stof reviewed

Oct 1, 2021

View reviewed changes

src/Symfony/Component/Lock/Store/PdoStore.phpShow resolvedHide resolved

jderusse reviewed

Oct 1, 2021

View reviewed changes

Copy link

Member

jderusse left a comment

Choose a reason for hiding this comment

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

This PR revert what have been done in7eca3a5#diff-98857ac5bb047b333495db5092ea4b875924022056c5b5522d7ecb224fac5f6f

@nicolas-grekas do you remember why you changed this in a merge commit?

derrabus requested changes

Oct 2, 2021

View reviewed changes

src/Symfony/Component/Lock/Store/PdoStore.php OutdatedShow resolvedHide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Oct 2, 2021

derrabus approved these changes

Oct 2, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels

Oct 2, 2021

jderusse reviewed

Oct 2, 2021

View reviewed changes

Copy link

Member

jderusse left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Regarding the@derrabus comment, this PR~~brings nothing but~~ introduce a future deprecation for upcoming Doctrine 3.2

I'm 👎 for this change

edit: this PR fixes the issue referenced in the description

Copy link

Member

@jderusse this PR fixes compatibility with driver decorators (inspecting the driver class to find the target platform is a mistake). See the ticket referenced in the issue description, which describes precisely such a bug.

Copy link

Member

jderusse commentedOct 3, 2021

@jderusse this PR fixes compatibility with driver decorators (inspecting the driver class to find the target platform is a mistake). See the ticket referenced in the issue description, which describes precisely such a bug.

Still..getName will be removed in the next major version (doctrine/dbal#4763) fixing the issue by introducing a dependency with a (soon) deprecated method is not the right way to fix the issue IMHO.