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

l-vo · 2022-02-20T14:45:41Z

Q	A
Branch?	5.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Step toward the fix of doctrine/DoctrineBundle#1431 (mentioned too in #44313 and #44495)
License	MIT
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 with doctrine/DoctrineBundle#1456 and doctrine/DoctrineBundle#1472

This PR suggest to add a middleware thats covers what was previously done by DbalLogger and DebugStack.

Another PR will follow in DoctrineBundle for the integration.

l-vo · 2022-02-20T14:58:50Z

status: needs work

l-vo · 2022-02-20T21:05:30Z

status: needs review

carsonbot · 2022-02-21T10:50:06Z

Hey!

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

Cheers!

Carsonbot

stof · 2022-02-24T09:56:20Z

src/Symfony/Bridge/Doctrine/Middleware/Debug/Connection.php

+    {
+        $query = null;
+        if (1 === ++$this->nestingLevel) {
+            $this->debugDataHolder->addQuery($this->connectionName, $query = new Query('"START TRANSACTION"'));


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

same for COMMIT and ROLLBACK

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 ?

src/Symfony/Bridge/Doctrine/Middleware/Debug/Query.php

src/Symfony/Bridge/Doctrine/Tests/DataCollector/DoctrineDataCollectorTest.php

stof · 2022-02-24T10:21:10Z

@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.

stof

All classes in the Symfony\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-vo · 2022-02-28T20:43:55Z

status: needs review

src/Symfony/Bridge/Doctrine/Middleware/Debug/DebugDataHolder.php

src/Symfony/Bridge/Doctrine/Middleware/Debug/Connection.php

src/Symfony/Bridge/Doctrine/Middleware/Debug/Query.php

derrabus · 2022-03-07T15:24:25Z

src/Symfony/Bridge/Doctrine/Middleware/Debug/Query.php

+
+    public function start(): void
+    {
+        $this->start = microtime(true);


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

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

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

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 ?

src/Symfony/Bridge/Doctrine/Middleware/Debug/Middleware.php

derrabus

Wrong button… I'm getting old. 🙈

l-vo · 2022-03-13T16:27:11Z

@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 · 2022-03-13T16:27:31Z

status: needs review

derrabus · 2022-03-22T14:03:49Z

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.

src/Symfony/Bridge/Doctrine/Middleware/Debug/DebugDataHolder.php

fabpot · 2022-03-25T13:00:34Z

Thank you @l-vo.

fabpot · 2022-04-02T13:25:45Z

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

fabpot · 2022-04-02T13:32:52Z

Nervermind, the BC break is in DoctrineBundle 2.6.

l-vo · 2022-04-02T14:16:56Z

@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 · 2022-04-02T14:46:21Z

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 · 2022-04-02T15:29:26Z

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

fabpot · 2022-04-02T15:49:01Z

Yes, it is!

l-vo · 2022-04-02T16:05:43Z

it has been spotted on doctrine/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 on sentry-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 · 2022-04-02T16:09:56Z

Fixed on sentry-symfony side by getsentry/sentry-symfony#608

…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

carsonbot added Status: Needs Review Feature labels Feb 20, 2022

carsonbot added this to the 6.1 milestone Feb 20, 2022

l-vo force-pushed the add_middleware_datacollector branch from f3dc761 to 4c4cd5a Compare February 20, 2022 14:52

carsonbot added Status: Needs Work and removed Status: Needs Review labels Feb 20, 2022

l-vo force-pushed the add_middleware_datacollector branch 4 times, most recently from 122ec76 to 0281685 Compare February 20, 2022 20:57

carsonbot added Status: Needs Review and removed Status: Needs Work labels Feb 20, 2022

stof requested changes Feb 24, 2022

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Feb 24, 2022

stof requested changes Feb 24, 2022

View reviewed changes

l-vo force-pushed the add_middleware_datacollector branch from 2eca244 to f660a87 Compare February 28, 2022 20:27

carsonbot added Status: Needs Review and removed Status: Needs Work labels Feb 28, 2022

derrabus approved these changes Mar 7, 2022

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Mar 7, 2022

derrabus requested changes Mar 7, 2022

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Mar 7, 2022

l-vo force-pushed the add_middleware_datacollector branch 3 times, most recently from 8aeaacc to 8de3964 Compare March 13, 2022 16:12

derrabus reviewed Mar 22, 2022

View reviewed changes

src/Symfony/Bridge/Doctrine/Middleware/Debug/DebugDataHolder.php Outdated Show resolved Hide resolved

stof approved these changes Mar 22, 2022

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Mar 22, 2022

l-vo force-pushed the add_middleware_datacollector branch from 8de3964 to 2d36823 Compare March 22, 2022 19:58

Allow to use a middleware instead of DbalLogger

20d0806

l-vo force-pushed the add_middleware_datacollector branch from 2d36823 to 20d0806 Compare March 22, 2022 20:09

fabpot approved these changes Mar 25, 2022

View reviewed changes

fabpot merged commit 1b2c9b5 into symfony:5.4 Mar 25, 2022

stof mentioned this pull request Mar 30, 2022

Implement a DBAL middleware for the profiler collector doctrine/DoctrineBundle#1431

Closed

This was referenced Apr 2, 2022

Release v5.4.7 #45915

Merged

Release v6.0.7 #45916

Merged

l-vo deleted the add_middleware_datacollector branch April 2, 2022 13:57

l-vo mentioned this pull request Apr 16, 2022

[DoctrineBridge] Restore PHP8 features for Doctrine debug middlewares #46090

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

l-vo commented Feb 20, 2022 •

edited

Loading

l-vo commented Feb 20, 2022

l-vo commented Feb 20, 2022

carsonbot commented Feb 21, 2022

stof Feb 24, 2022

stof Feb 24, 2022

l-vo Feb 28, 2022

stof commented Feb 24, 2022

stof left a comment

l-vo commented Feb 28, 2022

derrabus Mar 7, 2022

l-vo Mar 13, 2022

derrabus Mar 13, 2022

l-vo Mar 13, 2022 •

edited

Loading

derrabus left a comment

l-vo commented Mar 13, 2022 •

edited

Loading

l-vo commented Mar 13, 2022

derrabus commented Mar 22, 2022

fabpot commented Mar 25, 2022

fabpot commented Apr 2, 2022

fabpot commented Apr 2, 2022

l-vo commented Apr 2, 2022

fabpot commented Apr 2, 2022

l-vo commented Apr 2, 2022

fabpot commented Apr 2, 2022

l-vo commented Apr 2, 2022 •

edited

Loading

l-vo commented Apr 2, 2022

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

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

Conversation

l-vo commented Feb 20, 2022 • edited Loading

l-vo commented Feb 20, 2022

l-vo commented Feb 20, 2022

carsonbot commented Feb 21, 2022

stof Feb 24, 2022

Choose a reason for hiding this comment

stof Feb 24, 2022

Choose a reason for hiding this comment

l-vo Feb 28, 2022

Choose a reason for hiding this comment

stof commented Feb 24, 2022

stof left a comment

Choose a reason for hiding this comment

l-vo commented Feb 28, 2022

derrabus Mar 7, 2022

Choose a reason for hiding this comment

l-vo Mar 13, 2022

Choose a reason for hiding this comment

derrabus Mar 13, 2022

Choose a reason for hiding this comment

l-vo Mar 13, 2022 • edited Loading

Choose a reason for hiding this comment

derrabus left a comment

Choose a reason for hiding this comment

l-vo commented Mar 13, 2022 • edited Loading

l-vo commented Mar 13, 2022

derrabus commented Mar 22, 2022

fabpot commented Mar 25, 2022

fabpot commented Apr 2, 2022

fabpot commented Apr 2, 2022

l-vo commented Apr 2, 2022

fabpot commented Apr 2, 2022

l-vo commented Apr 2, 2022

fabpot commented Apr 2, 2022

l-vo commented Apr 2, 2022 • edited Loading

l-vo commented Apr 2, 2022

l-vo commented Feb 20, 2022 •

edited

Loading

l-vo Mar 13, 2022 •

edited

Loading

l-vo commented Mar 13, 2022 •

edited

Loading

l-vo commented Apr 2, 2022 •

edited

Loading