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

[Cache] Support decorated Dbal drivers in PdoAdapter#42011

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
Tobion merged 1 commit intosymfony:4.4fromJeroeny:dbal
Jul 14, 2021

Conversation

@Jeroeny
Copy link
Contributor

@JeroenyJeroeny commentedJul 7, 2021•
edited
Loading

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsExplanation in this PR
LicenseMIT

Doctrine v3 supports middleware for Drivers. Upon creating the Connection,middleware->wrap(Driver): Driveris called, in which the middleware can wrap/decorate the Driver class. So that it can perform tracingfor example.

When this happens, the Driver class inside the Connection is no longer one of Doctrine's well known Driver classes. ThePdoAdapter uses this class to determine the database platform. Which breaks once the Driver is decorated and no longer one of the classeslisted in thePdoAdapter.

Since Dbal exposes this middleware as a feature, I think it would be nice for thePdoAdapter to support this.

To solve this, thegetDatabasePlatform can be used. This returns aDoctrine\DBAL\Platforms\AbstractPlatform which defines the abstract methodgetName. This returns a value very similar to the list in thePdoAdapter. The names don't match exactly, so therefor a small mapping is done to get right the name used in the adapter. As far as a I know, there'd be no other implications with this change.

Related:getsentry/sentry-symfony#530

@derrabus
Copy link
Member

Can you provide a test for this change, please? I think we should target 4.4 and file this as a bugfix.

@derrabusderrabus added this to the4.4 milestoneJul 7, 2021
@JeroenyJeroeny changed the base branch from5.4 to4.4July 7, 2021 12:25
@Jeroeny
Copy link
ContributorAuthor

Can you provide a test for this change, please? I think we should target 4.4 and file this as a bugfix.

Test added and rebased on 4.4.

$config->setMiddlewares([$middleware]);

$connection = DriverManager::getConnection(['driver' =>'pdo_sqlite','path' =>self::$dbFile],$config);
}else {
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

This is fordoctrine/dbal v2, which doesn't have the middleware. Not sure if the test should cover this or something like aSkippedTestSuiteError should be thrown.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather call$this->markTestSkipped as the feature does not exist properly for v2

derrabus and Jeroeny reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

I agree. If the test is irrelevant for DBAL 2, just skip it if we‘re not testing against DBAL 3.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. If the test is irrelevant for DBAL 2, just skip it if we‘re not testing against DBAL 3.

I don't think that this fix is irrelevant for DBAL 2, because you could still decorate/wrap the driver manually anyway, see#42022 and the originating issuegetsentry/sentry-symfony#530.

In general, I think we should change theswitch to use$this->conn->getDatabasePlatform()->getName() as a reliable source of information.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

because you could still decorate/wrap the driver manually anyway

But not as official dbal v2 functionality right? You can pass adriverClass to the configuration, which is then constructed without arguments, making it less usable to function as decorator. Or it'd be via Reflection changing the private property_driver to public at runtime. Which is in my opinion already such an edge case, that Symfony shouldn't have to test against that.

Copy link
Contributor

@Jean85Jean85 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR! This should fix#42022

I don't understand why you didn't use$this->conn->getDatabasePlatform()->getName() as you suggested yourself though...

$config->setMiddlewares([$middleware]);

$connection = DriverManager::getConnection(['driver' =>'pdo_sqlite','path' =>self::$dbFile],$config);
}else {
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. If the test is irrelevant for DBAL 2, just skip it if we‘re not testing against DBAL 3.

I don't think that this fix is irrelevant for DBAL 2, because you could still decorate/wrap the driver manually anyway, see#42022 and the originating issuegetsentry/sentry-symfony#530.

In general, I think we should change theswitch to use$this->conn->getDatabasePlatform()->getName() as a reliable source of information.

@Jeroeny
Copy link
ContributorAuthor

Jeroeny commentedJul 8, 2021•
edited
Loading

I don't understand why you didn't use$this->conn->getDatabasePlatform()->getName() as you suggested yourself though...

I did though? SeePdoTrait:458

Edit: I see there's a slight difference ($driver->getDatabasePlatform()->getName()), but I'm not sure how important that difference is?

@Jean85
Copy link
Contributor

I don't understand why you didn't use$this->conn->getDatabasePlatform()->getName() as you suggested yourself though...

I did though? SeePdoTrait:458

Edit: I see there's a slight difference ($driver->getDatabasePlatform()->getName()), but I'm not sure how important that difference is?

My suggestion is to changethe wholeswitch to drive the choice using$driver->getDatabasePlatform()->getName(), because theinstaceof will be broken by wrappers.

@Jeroeny
Copy link
ContributorAuthor

My suggestion is to changethe wholeswitch to drive the choice using$driver->getDatabasePlatform()->getName(), because theinstaceof will be broken by wrappers.

That's good feedback 👍 . Though I'd like to get some feedback on that from a Symfony maintainer as well, if possible. They're the ones to approve the PR after all.

@Jean85
Copy link
Contributor

My suggestion is to changethe wholeswitch to drive the choice using$driver->getDatabasePlatform()->getName(), because theinstaceof will be broken by wrappers.

That's good feedback +1 . Though I'd like to get some feedback on that from a Symfony maintainer as well, if possible. They're the ones to approve the PR after all.

@nicolas-grekas already weighed in in#42022 (comment), but I'm not sure if he got the full implication of my suggestion.

@nicolas-grekas
Copy link
Member

change the whole switch

That works for me if the resulting code remains compatible with the range of dbal versions we support currently.

Jeroeny reacted with thumbs up emoji

Copy link
Member

@derrabusderrabus left a comment

Choose a reason for hiding this comment

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

In patch-types.php, we maintain a list of files that should be ignored by that script. Please add your newDriverWrapper fixture there to make the tests pass.

@derrabus
Copy link
Member

LGTM, but I'd turn the@return annotations of theDriverWrapper fixture into proper return types. This way, it does not matter that much if we exclude it from the patch-types script.

Jeroeny reacted with thumbs up emoji

@Tobion
Copy link
Contributor

Thank you@Jeroeny.

@TobionTobion merged commite5c96c4 intosymfony:4.4Jul 14, 2021
This was referencedJul 26, 2021
derrabus added a commit that referenced this pull requestOct 4, 2021
This PR was squashed before being merged into the 5.4 branch.Discussion----------[Lock] Use platform to identify the PDO driver| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       |Fix#43048| License       | MITThis should fix the issue in the same way we did for#42011. I'm not sure if I should add/change any test...Commits-------687a7ed [Lock] Use platform to identify the PDO driver
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@derrabusderrabusderrabus approved these changes

@nicolas-grekasnicolas-grekasAwaiting requested review from nicolas-grekas

+2 more reviewers

@Jean85Jean85Jean85 requested changes

@TobionTobionTobion approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

7 participants

@Jeroeny@derrabus@Jean85@nicolas-grekas@Tobion@stof@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp