-
-
Notifications
You must be signed in to change notification settings - Fork 474
feat: add logging middleware registration per connection with logger service #2156
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 3.1.x
Are you sure you want to change the base?
Changes from 3 commits
e65b266
a5456ac
1aafca1
9c72fc3
bab865a
03dde76
30d17ab
508856f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1451,8 +1451,22 @@ private function registerDbalMiddlewares( | |
| $loader->load('middlewares.php'); | ||
|
|
||
| $loggingMiddlewareAbstractDef = $container->getDefinition('doctrine.dbal.logging_middleware'); | ||
|
|
||
| foreach ($connWithLogging as $connName) { | ||
| // Register logging middlewares only when a logger service is available | ||
| if (! $container->has('logger')) { | ||
| continue; | ||
| } | ||
|
|
||
| // Preserve legacy behavior: also tag the abstract definition per-connection | ||
| $loggingMiddlewareAbstractDef->addTag('doctrine.middleware', ['connection' => $connName, 'priority' => 10]); | ||
|
|
||
| // Create a child service with a dedicated Monolog channel | ||
| $id = sprintf('doctrine.dbal.logging_middleware.%s', $connName); | ||
|
||
| $child = new ChildDefinition('doctrine.dbal.logging_middleware'); | ||
| $child->addTag('doctrine.middleware', ['connection' => $connName, 'priority' => 10]); | ||
| $child->addTag('monolog.logger', ['channel' => sprintf('doctrine.%s', $connName)]); | ||
|
||
| $container->setDefinition($id, $child); | ||
| } | ||
|
|
||
| $container->getDefinition('doctrine.debug_data_holder')->replaceArgument(0, $connWithBacktrace); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1064,6 +1064,8 @@ public function testAsDoctrineListenerAttribute(): void | |
| public function testRegistrationsWithMiddlewaresAndSfDebugMiddleware(): void | ||
| { | ||
| $container = $this->getContainer(); | ||
| // Register a dummy logger service to enable logging middleware registration | ||
| $container->setDefinition('logger', (new Definition('\stdClass'))->setPublic(true)); | ||
|
Comment on lines
+1067
to
+1068
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see why are these needed. Probably because of my other comment. |
||
| $extension = new DoctrineExtension(); | ||
|
|
||
| $config = BundleConfigurationBuilder::createBuilder() | ||
|
|
@@ -1133,6 +1135,8 @@ public function testRegistrationsWithMiddlewaresAndSfDebugMiddleware(): void | |
| public function testDefinitionsToLogAndProfile(): void | ||
| { | ||
| $container = $this->getContainer(); | ||
| // Register a dummy logger service to enable logging middleware registration | ||
| $container->setDefinition('logger', (new Definition('\stdClass'))->setPublic(true)); | ||
| $extension = new DoctrineExtension(); | ||
|
|
||
| $config = BundleConfigurationBuilder::createBuilder() | ||
|
|
@@ -1206,6 +1210,86 @@ public function testDefinitionsToLogQueriesLoggingFalse(): void | |
| $this->assertArrayNotHasKey('doctrine.middleware', $abstractMiddlewareDefTags); | ||
| } | ||
|
|
||
| public function testLoggingMiddlewarePerConnectionWithLoggerPresent(): void | ||
| { | ||
| $container = $this->getContainer(); | ||
| // Register a dummy logger service to enable logging middleware registration | ||
| $container->setDefinition('logger', (new Definition('\stdClass'))->setPublic(true)); | ||
|
|
||
| $extension = new DoctrineExtension(); | ||
|
|
||
| $config = BundleConfigurationBuilder::createBuilder() | ||
| ->addConnection([ | ||
| 'connections' => [ | ||
| 'conn1' => [ | ||
| 'password' => 'foo', | ||
| 'logging' => true, | ||
| 'profiling' => false, | ||
| ], | ||
| 'conn2' => [ | ||
| 'password' => 'bar', | ||
| 'logging' => true, | ||
| 'profiling' => false, | ||
| ], | ||
| ], | ||
| ]) | ||
| ->build(); | ||
|
|
||
| $extension->load([$config], $container); | ||
|
|
||
| // Abstract definition should be present and tagged per connection | ||
| $this->assertTrue($container->hasDefinition('doctrine.dbal.logging_middleware')); | ||
| $abstractTags = $container->getDefinition('doctrine.dbal.logging_middleware')->getTags(); | ||
| $this->assertArrayHasKey('doctrine.middleware', $abstractTags); | ||
| $this->assertContains(['connection' => 'conn1', 'priority' => 10], $abstractTags['doctrine.middleware']); | ||
| $this->assertContains(['connection' => 'conn2', 'priority' => 10], $abstractTags['doctrine.middleware']); | ||
|
|
||
| // Child services should be created per connection with proper tags | ||
| $this->assertTrue($container->hasDefinition('doctrine.dbal.logging_middleware.conn1')); | ||
| $this->assertTrue($container->hasDefinition('doctrine.dbal.logging_middleware.conn2')); | ||
|
|
||
| $child1Tags = $container->getDefinition('doctrine.dbal.logging_middleware.conn1')->getTags(); | ||
| $child2Tags = $container->getDefinition('doctrine.dbal.logging_middleware.conn2')->getTags(); | ||
|
|
||
| $this->assertArrayHasKey('doctrine.middleware', $child1Tags); | ||
| $this->assertContains(['connection' => 'conn1', 'priority' => 10], $child1Tags['doctrine.middleware']); | ||
| $this->assertArrayHasKey('monolog.logger', $child1Tags); | ||
| $this->assertContains(['channel' => 'doctrine.conn1'], $child1Tags['monolog.logger']); | ||
|
|
||
| $this->assertArrayHasKey('doctrine.middleware', $child2Tags); | ||
| $this->assertContains(['connection' => 'conn2', 'priority' => 10], $child2Tags['doctrine.middleware']); | ||
| $this->assertArrayHasKey('monolog.logger', $child2Tags); | ||
| $this->assertContains(['channel' => 'doctrine.conn2'], $child2Tags['monolog.logger']); | ||
| } | ||
|
|
||
| public function testLoggingMiddlewareNotRegisteredWithoutLogger(): void | ||
| { | ||
| $container = $this->getContainer(); | ||
| // No logger service defined here | ||
|
|
||
| $extension = new DoctrineExtension(); | ||
|
|
||
| $config = BundleConfigurationBuilder::createBuilder() | ||
| ->addConnection([ | ||
| 'connections' => [ | ||
| 'conn1' => [ | ||
| 'password' => 'foo', | ||
| 'logging' => true, | ||
| 'profiling' => false, | ||
| ], | ||
| ], | ||
| ]) | ||
| ->build(); | ||
|
|
||
| $extension->load([$config], $container); | ||
|
|
||
| // Abstract definition exists by default, but should not be tagged nor should child definitions exist | ||
| $this->assertTrue($container->hasDefinition('doctrine.dbal.logging_middleware')); | ||
| $abstractTags = $container->getDefinition('doctrine.dbal.logging_middleware')->getTags(); | ||
| $this->assertArrayNotHasKey('doctrine.middleware', $abstractTags); | ||
| $this->assertFalse($container->hasDefinition('doctrine.dbal.logging_middleware.conn1')); | ||
| } | ||
|
|
||
| #[RequiresMethod(Driver::class, '__construct')] | ||
| public function testDefinitionsIdleConnection(): void | ||
| { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be moved below? Otherwise doctrine.middleware tag is not being added to logging middleware
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 9c72fc3 and bab865a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really solved. Line 1475 is still skipped if logger doesn't exist, while that is not the case without this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, should be resolved now at 30d17ab