Fixes for compatibility with DBAL 3 #1263

greg0ire · 2020-11-16T08:25:10Z

I tested this on a real project, it allows to install the project without errors.

Fixes #1262 , fixes #1264

How can I test this?

composer config repositories.greg0ire vcs https://github.com/greg0ire/DoctrineBundle
composer require doctrine/doctrine-bundle "dev-dbal-3-compat as 2.2.1"

ostrolucky · 2020-11-16T08:38:55Z

Looks good so far, ask me to review after it's no longer draft please.

greg0ire · 2020-11-16T08:40:50Z

Looks good so far, ask me to review after it's no longer draft please.

I need to revert the composer.json change and detect 2.10.0, will work on it this evening but feel fre to do it and push to my branch 👍

The alternative to that would be to merge and release doctrine/dbal#4386

stof · 2020-11-16T17:35:32Z

Resources/config/dbal.xml

@@ -23,11 +23,7 @@
        <service id="Doctrine\DBAL\Driver\Connection" alias="database_connection" public="false" />
        <service id="Doctrine\DBAL\Connection" alias="database_connection" public="false" />

-        <service id="doctrine.dbal.logger.chain" class="%doctrine.dbal.logger.chain.class%" public="false" abstract="true">
-            <call method="addLogger">
-                <argument type="service" id="doctrine.dbal.logger" />


wasn't this call needed ? I don't see its replacement

The replacement is in PHP, it's the very first lines of this PR

greg0ire · 2020-11-16T17:41:48Z

DependencyInjection/DoctrineExtension.php

+            $chainLogger->addMethodCall('addLogger', [$profilingLogger]);
+        } else {
+            $chainLogger->addArgument([$profilingLogger]);
+        }


@stof here is the replacement you asked for in https://github.com/doctrine/DoctrineBundle/pull/1263/files#r524450538 (I think it's equivalent)

this is not equivalent. As you need to pass an array of loggers to the constructor rather than multiple calls to addLogger, you need to build an array containing them all.

But there is only one logger before this PR, right?

What are you suggesting exactly? This?

$chainLogger->replaceArgument( 0, array_merge($chainLogger->getArgument(0), [$logger]) );

no. Cases where the chain logger are used are precisely cases where we have 2 loggers:

doctrine.dbal.logger logging to your app PSR-3 logger (generally Monolog)

the profiling logger which logs query executions for the profiler

Your suggestion won't work if the reference to doctrine.dbal.logger is added to the constructor of the doctrine.dbal.logger.chain abstract definition, because the child definition won't have anything yet for the argument. Instead, you should just use [$logger, $profilingLogger] as the value of the argument

@stof none of the calls to addLogger in this codebase are performed on the same service. I don't get your point.

UPD: didn't see your answer, I will try that.

In the existing code, the call done in XML is done on the abstract service definition which is extended by the definitions in each of the DBAL connections.

so after resolving the definition inheritance, each chain logger will have 2 addLogger calls:

the one inherited from the abstract definition for doctrine.dbal.logger

the one set on themselves for the connection-specific profiling logger.

stof · 2020-11-16T17:47:43Z

DependencyInjection/DoctrineExtension.php

@@ -85,6 +87,14 @@ protected function dbalLoad(array $config, ContainerBuilder $container)
    {
        $loader = new XmlFileLoader($container, new FileLocator(__DIR__ . '/../Resources/config'));
        $loader->load('dbal.xml');
+        $chainLogger     = $container->getDefinition('doctrine.dbal.logger.chain');
+        $profilingLogger = new Reference('doctrine.dbal.logger');


This variable name is really confusing, as doctrine.dbal.logger is not the profiling logger, but the logger logging your queries in the Symfony logs through PSR-3.

greg0ire · 2020-11-16T18:27:06Z

Tests/ContainerTest.php

+        $container = $this->createDbalOnlyTestContainer();
+        $this->assertInstanceOf(
+            LoggerChain::class,
+            $container->get('doctrine.dbal.logger.chain.default')


I must have messed up somewhere because although the service definition has 2 loggers, that object has only one when I dump it.

Oh but when I remove the first else branch in this PR, it works.

See my comment on the broken code.

I just pushed, does it look correct or do I still misunderstand? Sorry, I'm quite tired today 😅

This now looks good.

Pushed again

stof · 2020-11-16T18:30:10Z

DependencyInjection/DoctrineExtension.php

+            // doctrine/dbal < 2.10.0
+            $chainLogger->addMethodCall('addLogger', [$logger]);
+        } else {
+            $chainLogger->addArgument([$logger]);


this must be removed. You are adding an argument here with an array with 1 element, and then later adding a second argument with an array with 2 elements. But the ChainLogger expects the list of loggers as its first argument so that's why things don't work.

an alternative would be to use replaceArgument in the ChildDefinition

That might be better 🤔 in case people are using the abstract service definition to build their custom service.

Pushed again, this time with replaceArgument

if they use it to build their own custom service, they will need to change their code for the DBAL 3 way anyway.

Yes, but I'm switching to the constructor as soon as possible (2.10.0), not as late as possible (3.0.0), because a deprecation layer might be added later on the DBAL. This means people using DBAL >=2.10 and building their own custom service should need this.

Pushed again, this time with replaceArgument

We could even go a cleaner way to prepare for the day we will require DBAL 2.10+ in the bundle (hopefully soon, if the PR adding back PHP 7.1 support in 2.12 is accepted):

stop using ChildDefinition in loadDbalConnection but a normal Definition using the chainlogger class (this requires adding the addLogger call for $logger in the branch dealing with DBAL 2.9, but that's fine)

mark the doctrine.dbal.logger.chain abstract service as deprecated (in case someone was using it for their own service)

That does seem more straightforward, let me try 👍

stof · 2020-11-16T18:30:38Z

Tests/ContainerTest.php

+        $container = $this->createDbalOnlyTestContainer();
+        $this->assertInstanceOf(
+            LoggerChain::class,
+            $container->get('doctrine.dbal.logger.chain.default')


See my comment on the broken code.

greg0ire · 2020-11-16T19:00:35Z

Resources/config/dbal.xml

-                <argument type="service" id="doctrine.dbal.logger" />
-            </call>
+        <service id="doctrine.dbal.logger.chain" class="%doctrine.dbal.logger.chain.class%" public="false" abstract="true" >
+            <!-- TODO: deprecate this service in 2.3.0 -->


@stof introducing a new deprecation is minor, so I should do it in a subsequent PR to master I think.

DependencyInjection/DoctrineExtension.php

Tests/ContainerTest.php

Tests/TestCase.php

Passing them through addLogger() is deprecated and no longer works with DBAL 3 which we support.

That command is built from Doctrine\DBAL\Tools\Console\Command\ImportCommand, which no longer exists in DBAL 3 (it is removed in favor of using a client application).

greg0ire · 2020-11-19T08:09:23Z

@stof can you please give this another review?

greg0ire · 2020-11-19T19:13:16Z

I suppose the 2 existing reviews are enough already :)

dmaicher · 2020-12-12T15:42:22Z

@greg0ire I think we need some of those changes also on 1.12.x, or?

As it also allows installing dbal 3: https://github.com/doctrine/DoctrineBundle/blob/1.12.x/composer.json#L29

greg0ire · 2020-12-12T16:02:18Z

@dmaicher you're right, I had no idea dbal 3 was supported on 1.12.x (or why, it sounds weird, since that version of the bundle is supposed to be left unmaintained soon).

dmaicher · 2020-12-12T16:04:46Z

@dmaicher you're right, I had no idea dbal 3 was supported on 1.12.x (or why, it sounds weird, since that version of the bundle is supposed to be left unmaintained soon).

Yeah I also just found out while debugging some travis CI failures while working on dmaicher/doctrine-test-bundle#136 😕

ostrolucky · 2020-12-12T16:54:28Z

with symfony 3.4 out of maintenance, 1.12.x is pretty much dead now, all that is left is just officially mark it as unmaintained on website config

greg0ire requested a review from ostrolucky November 16, 2020 08:25

greg0ire added the Bug label Nov 16, 2020

greg0ire linked an issue Nov 16, 2020 that may be closed by this pull request

Call to undefined method Doctrine\DBAL\Logging\LoggerChain::addLogger() with DBAL 3 #1262

Closed

greg0ire removed the request for review from ostrolucky November 16, 2020 08:29

greg0ire marked this pull request as draft November 16, 2020 08:29

greg0ire force-pushed the dbal-3-compat branch 3 times, most recently from 740f645 to 3171edc Compare November 16, 2020 17:30

stof reviewed Nov 16, 2020

View reviewed changes

greg0ire force-pushed the dbal-3-compat branch from 3171edc to 9ee90f7 Compare November 16, 2020 17:40

greg0ire commented Nov 16, 2020

View reviewed changes

stof reviewed Nov 16, 2020

View reviewed changes

greg0ire force-pushed the dbal-3-compat branch from 9ee90f7 to e41eca2 Compare November 16, 2020 18:03

greg0ire marked this pull request as ready for review November 16, 2020 18:12

greg0ire force-pushed the dbal-3-compat branch from e41eca2 to a34fb11 Compare November 16, 2020 18:20

greg0ire commented Nov 16, 2020

View reviewed changes

stof reviewed Nov 16, 2020

View reviewed changes

greg0ire force-pushed the dbal-3-compat branch 3 times, most recently from a0268d8 to 065a36c Compare November 16, 2020 18:59

greg0ire commented Nov 16, 2020

View reviewed changes

greg0ire requested a review from ostrolucky November 16, 2020 19:31

greg0ire mentioned this pull request Nov 17, 2020

Attempted to load class "ImportCommand" from namespace "Doctrine\DBAL\Tools\Console\Command" with DBAL 3 #1264

Closed

greg0ire linked an issue Nov 17, 2020 that may be closed by this pull request

Attempted to load class "ImportCommand" from namespace "Doctrine\DBAL\Tools\Console\Command" with DBAL 3 #1264

Closed

greg0ire requested a review from stof November 17, 2020 08:02

ostrolucky reviewed Nov 17, 2020

View reviewed changes

DependencyInjection/DoctrineExtension.php Show resolved Hide resolved

Tests/ContainerTest.php Show resolved Hide resolved

Tests/ContainerTest.php Outdated Show resolved Hide resolved

Tests/TestCase.php Outdated Show resolved Hide resolved

greg0ire force-pushed the dbal-3-compat branch from 1597913 to b9d53b7 Compare November 17, 2020 18:42

greg0ire changed the title ~~Fix for compatibility with DBAL 3~~ Fixes for compatibility with DBAL 3 Nov 17, 2020

greg0ire added 2 commits November 17, 2020 22:14

Pass loggers through the constructor

2c377e2

Passing them through addLogger() is deprecated and no longer works with DBAL 3 which we support.

Register doctrine:database:import conditionally

b79661a

That command is built from Doctrine\DBAL\Tools\Console\Command\ImportCommand, which no longer exists in DBAL 3 (it is removed in favor of using a client application).

greg0ire force-pushed the dbal-3-compat branch from b9d53b7 to b79661a Compare November 17, 2020 21:15

greg0ire requested a review from ostrolucky November 17, 2020 22:33

ostrolucky approved these changes Nov 18, 2020

View reviewed changes

SenseException approved these changes Nov 18, 2020

View reviewed changes

greg0ire merged commit 74c3bab into doctrine:2.2.x Nov 19, 2020

greg0ire deleted the dbal-3-compat branch November 19, 2020 19:13

greg0ire added this to the 2.2.2 milestone Nov 19, 2020

Fixes for compatibility with DBAL 3 #1263

Fixes for compatibility with DBAL 3 #1263

Conversation

greg0ire commented Nov 16, 2020 • edited Loading

How can I test this?

ostrolucky commented Nov 16, 2020

greg0ire commented Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Nov 19, 2020

greg0ire commented Nov 19, 2020

dmaicher commented Dec 12, 2020

greg0ire commented Dec 12, 2020

dmaicher commented Dec 12, 2020

ostrolucky commented Dec 12, 2020

greg0ire commented Nov 16, 2020 •

edited

Loading

greg0ire commented Nov 16, 2020 •

edited

Loading

greg0ire Nov 16, 2020 •

edited

Loading

greg0ire Nov 16, 2020 •

edited

Loading

greg0ire Nov 16, 2020 •

edited

Loading

greg0ire Nov 16, 2020 •

edited

Loading