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

[DoctrineBridge] Allow to use a middleware instead of DbalLogger#45491

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
fabpot merged 1 commit intosymfony:5.4froml-vo:add_middleware_datacollector
Mar 25, 2022

Conversation

@l-vo
Copy link
Contributor

@l-vol-vo commentedFeb 20, 2022•
edited
Loading

QA
Branch?5.4
Bug fix?no
New feature?yes
Deprecations?no
TicketsStep toward the fix ofdoctrine/DoctrineBundle#1431 (mentioned too in#44313 and#44495)
LicenseMIT
Doc PR

The SqlLogger that is used in doctrine bridge and doctrine bundle has been deprecated and replaced by a system of Middleware.

A work has started on Doctrine bundle withdoctrine/DoctrineBundle#1456 anddoctrine/DoctrineBundle#1472

This PR suggest to add a middleware thats covers what was previously done byDbalLogger andDebugStack.

Another PR will follow in DoctrineBundle for the integration.

dmaicher reacted with thumbs up emojiderrabus reacted with rocket emoji
@l-vo
Copy link
ContributorAuthor

status: needs work

@l-vol-voforce-pushed theadd_middleware_datacollector branch 4 times, most recently from122ec76 to0281685CompareFebruary 20, 2022 20:57
@l-vo
Copy link
ContributorAuthor

status: needs review

@carsonbot
Copy link

Hey!

I think@lucasaba has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

{
$query =null;
if (1 === ++$this->nestingLevel) {
$this->debugDataHolder->addQuery($this->connectionName,$query =newQuery('"START TRANSACTION"'));
Copy link
Member

Choose a reason for hiding this comment

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

why the double quotes ? They are not part of the SQL query starting a transaction.

Copy link
Member

Choose a reason for hiding this comment

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

same for COMMIT and ROLLBACK

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

It's the format that is currently used in the profiler:https://github.com/doctrine/dbal/blob/2afc6f00e8ff145ab386e9d25af293db98418bf5/src/Connection.php#L1297; should I change it anyway ?

@stof
Copy link
Member

@nicolas-grekas@fabpot given that this PR is about moving away from a deprecated DBAL API for the collector of the Symfony webprofiler, I suggest merging that into 5.4 rather than 6.1, so that the Symfony LTS does not use the deprecated API for years for the debug features.

Copy link
Member

@stofstof left a comment

Choose a reason for hiding this comment

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

All classes in theSymfony\Bridge\Doctrine\Middleware\Debug namespace except for the middleware itself and the DebugDataHolder should be tagged@internal. The various decorators are not meant to be reused by other code than our middleware (the DebugDataHolder needs to be wired by DoctrineBundle, which is why it cannot be internal)

@l-vol-voforce-pushed theadd_middleware_datacollector branch from2eca244 tof660a87CompareFebruary 28, 2022 20:27
@l-vo
Copy link
ContributorAuthor

status: needs review


publicfunctionstart():void
{
$this->start =microtime(true);
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this duplicate the logic from the stopwatch? How about using the duration from the stopwatch instead?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Stopwatch is an optional dependency of Doctrine bridge. Not using the stopwatch here allows to have durations even when stopwatch is not installed.

Copy link
Member

Choose a reason for hiding this comment

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

I find it kind of acceptable that timings are available only if the stopwatch is installed.

Copy link
ContributorAuthor

@l-vol-voMar 13, 2022•
edited
Loading

Choose a reason for hiding this comment

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

The point is it's currently done in this way in DebugStack. So using stopwatch instead will lead to a loss of query durations occurring on a patch version (for an application without stopwatch). WDYT ?

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.

Wrong button… I'm getting old. 🙈

@l-vol-voforce-pushed theadd_middleware_datacollector branch 2 times, most recently from8aeaacc to8de3964CompareMarch 13, 2022 16:12
@l-vo
Copy link
ContributorAuthor

l-vo commentedMar 13, 2022•
edited
Loading

@derrabus@stof target branch changed to 5.4. Should I do another PR to 6.1 after merge to restore modern additions brought by php >=8 ?

@l-vo
Copy link
ContributorAuthor

status: needs review

@derrabus
Copy link
Member

Should I do another PR to 6.1 after merge to restore modern additions brought by php >=8 ?

If we don't do this already while merging up, please create that PR.

l-vo reacted with thumbs up emoji

@l-vol-voforce-pushed theadd_middleware_datacollector branch from2d36823 to20d0806CompareMarch 22, 2022 20:09
@fabpot
Copy link
Member

Thank you@l-vo.

@fabpot
Copy link
Member

That's a breaking change assetMiddlewares() was introduced in DBAL 3, so this code is not compatible with Doctrine DBAL 2.

@fabpot
Copy link
Member

Nervermind, the BC break is in DoctrineBundle 2.6.

@l-vol-vo deleted the add_middleware_datacollector branchApril 2, 2022 13:57
@l-vo
Copy link
ContributorAuthor

l-vo commentedApr 2, 2022

@fabpot there is this kind of bc layer on Doctrine bundle:https://github.com/doctrine/DoctrineBundle/blob/0620e230c6529011b305e07efa0df0678bf71a6d/DoctrineBundle.php#L67. Did I miss another thing ?

@fabpot
Copy link
Member

Here is the error I get:

!!  Symfony\Component\ErrorHandler\Error\UndefinedMethodError {#5710!!    #message: "Attempted to call an undefined method named "setMiddlewares" of class "Doctrine\DBAL\Configuration"."!!    #code: 0!!    #file: "./var/cache/dev/ContainerCojB6IU/App_KernelDevDebugContainer.php"!!    #line: 1233!!    trace: {!!      ./var/cache/dev/ContainerCojB6IU/App_KernelDevDebugContainer.php:1233 {!!        ContainerCojB6IU\App_KernelDevDebugContainer->getDoctrine_Dbal_DefaultConnectionService()!!        › $a->setSQLLogger(new \Doctrine\DBAL\Logging\LoggerChain([0 => new \Symfony\Bridge\Doctrine\Logger\DbalLogger($b, ($this->privates['debug.stopwatch'] ?? ($this->privates['debug.stopwatch'] = new \Symfony\Component\Stopwatch\Stopwatch(true)))), 1 => ($this->privates['doctrine.dbal.logger.profiling.default'] ?? ($this->privates['doctrine.dbal.logger.profiling.default'] = new \Doctrine\DBAL\Logging\DebugStack()))]));!!        › $a->setMiddlewares([]);!!        › !!      }

@l-vo
Copy link
ContributorAuthor

l-vo commentedApr 2, 2022

By chance, is it a project with sentry-symfony ?

@fabpot
Copy link
Member

Yes, it is!

l-vo reacted with thumbs up emoji

@l-vo
Copy link
ContributorAuthor

l-vo commentedApr 2, 2022•
edited
Loading

it has been spotted ondoctrine/DoctrineBundle#1485, sentry-symfony aliases an internal interface as Doctrine\DBAL\Middleware that fools the detection. Accorded to the issue in DoctrineBundle, it won't be fixed on DoctrineBundle side. From@dmaicher:

Ok so I see 2 options:

  • report this issue onsentry-symfony as they really should not alias/define a Doctrine DBAL interface...
  • we look into changing the "middleware avaiable detection" on our side here so it works with Sentry

I personally would vote for the first option as this alias magic can really cause a lot of confusion and what the f**k moments as we see here.

@l-vo
Copy link
ContributorAuthor

l-vo commentedApr 2, 2022

Fixed on sentry-symfony side bygetsentry/sentry-symfony#608

nicolas-grekas added a commit that referenced this pull requestApr 19, 2022
…g middlewares (l-vo)This PR was submitted for the 6.0 branch but it was merged into the 6.1 branch instead.Discussion----------[DoctrineBridge] Restore PHP8 features for Doctrine debug middlewares| Q             | A| ------------- | ---| Branch?       | 6.1| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       || License       | MIT| Doc PR        | To merge#45491 in 5.4, the code has been changed to be 7.2 compliant. This PR restores the original code for Symfony versions that support php >= 8.0 (see#45491 (comment)).Commits-------f4bdd82 Restore PHP8 features for Doctrine debug middlewares
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@derrabusderrabusderrabus requested changes

@fabpotfabpotfabpot approved these changes

@stofstofstof approved these changes

@wouterjwouterjAwaiting requested review from wouterj

@chalasrchalasrAwaiting requested review from chalasr

@xabbuhxabbuhAwaiting requested review from xabbuh

@ycerutoycerutoAwaiting requested review from yceruto

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

5 participants

@l-vo@carsonbot@stof@derrabus@fabpot
[8]ページ先頭
©2009-2025 Movatter.jp