[Messenger] Testing LoggingMiddleware and minor improvements #26912

yceruto · 2018-04-13T02:54:41Z

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

This add tests for LoggingMiddleware class

Similar to translator.logging a logging service doesn't make sense without a hard logger dependency and according to:

symfony/src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

Lines 52 to 54 in 6bbb5bc

    
           if (!$container->getParameter('kernel.debug') || !$container->has('logger')) { 
        
               $container->removeDefinition('messenger.middleware.debug.logging'); 
        
           }

it must be removed if logger alias doesn't exists.

yceruto · 2018-04-13T04:57:07Z

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

@@ -49,10 +49,6 @@ public function process(ContainerBuilder $container)
            return;
        }

-        if (!$container->getParameter('kernel.debug') || !$container->has('logger')) {
-            $container->removeDefinition('messenger.middleware.debug.logging');
-        }


I needed move this part to FrameworkExtension to make tests pass (see old failures)

yceruto · 2018-04-13T04:58:32Z

src/Symfony/Component/Messenger/composer.json

        "symfony/serializer": "~3.4|~4.0",
        "symfony/dependency-injection": "~3.4.6|~4.0",
        "symfony/http-kernel": "~3.4|~4.0",
        "symfony/property-access": "~3.4|~4.0",
        "symfony/var-dumper": "~3.4|~4.0",
-        "symfony/property-access": "~3.4|~4.0",


Removing as duplicated entry (unrelated change)

ogizanagi · 2018-04-13T08:28:57Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -1446,6 +1446,10 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder

        $loader->load('messenger.xml');

+        if (!$container->getParameter('kernel.debug') || !$container->hasAlias('logger')) {


Actually, the !$container->hasAlias('logger') part could be removed, as the Fwb always registers a default logger implementation since #24300

But anyway that'll be handled differently with #26864 as it'll have to be opt-in explicitly.

AFAIK doing so will always remove the middleware. I don't see any value in changing what was just changed in #26896 :)

The !$container->hasAlias('logger') check in the fwb extension will indeed lead to the middleware removal, even if there is in fact a logger alias declared by the monolog bundle or the kernel LoggerPass, so it's wrong. But as stated above, this check is not necessary anyway, as there will always be a logger alias.

Regarding #26896 , I agree with @yceruto: It doesn't really make sense to me having a LoggingMiddleware without a hard logger dependency. It solves an issue we usually solve at compilation time by removing proper services.

I'll try this again soon.

sroze · 2018-04-13T08:48:47Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -1446,6 +1446,10 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder

        $loader->load('messenger.xml');

+        if (!$container->getParameter('kernel.debug') || !$container->hasAlias('logger')) {


AFAIK doing so will always remove the middleware. I don't see any value in changing what was just changed in #26896 :)

sroze · 2018-04-13T08:49:27Z

src/Symfony/Component/Messenger/Tests/Debug/LoggingMiddlewareTest.php

+        $next = $this->createPartialMock(\stdClass::class, array('__invoke'));
+        $next->expects($this->once())->method('__invoke')->with($message);
+
+        (new LoggingMiddleware($logger))->handle($message, $next);


Can you test that it returns the value returned by the previous middleware as well?

yceruto · 2018-04-13T14:28:37Z

@ogizanagi the tests fail again, I guess LoggerPass is processed after MessengerPass? or not?

symfony/src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php

Line 90 in 6bbb5bc

    
           $container->addCompilerPass(new LoggerPass(), PassConfig::TYPE_BEFORE_OPTIMIZATION, -32);

Any suggestions?

ogizanagi · 2018-04-13T16:05:27Z

@yceruto : IIUC, the FrameworkExtensionTest does not build the FrameworkBundle, so compiler passes like the LoggerPass registered in the fwb are never processed.

Anyway, you moved it back to the MessengerPass, so this check is kind of required here as you can theoretically use the pass shipped within the component out of the Fwb usage, just with DI. So, let's keep this check.

yceruto · 2018-04-13T22:13:03Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.xml

@@ -52,7 +52,7 @@

        <!-- Logging & Debug -->
        <service id="messenger.middleware.debug.logging" class="Symfony\Component\Messenger\Debug\LoggingMiddleware">


Another workaround could be creating from scratch this service definition in MessengerPass, only if debug and logger are enabled/exists, wdyt?

We usually try as much as possible to declare all services in xml files so everything is in one place, favoring discovery/inspections and allowing to jump from classes to definitions with the PhpStorm Symfony plugin.

nicolas-grekas

composer.json should be alpha ordered
could also validator+process work with 3.4? if yes, let's do the change here.

yceruto · 2018-04-14T17:53:36Z

Done! (failures are unrelated)

Status: Needs Review

Changes done

sroze · 2018-04-15T21:29:48Z

Thank you @yceruto.

…ents (yceruto) This PR was merged into the 4.1-dev branch. Discussion ---------- [Messenger] Testing LoggingMiddleware and minor improvements | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - - This add tests for `LoggingMiddleware` class - Similar to [`translator.logging`](https://github.com/symfony/symfony/blob/6bbb5bcc5294fdd540bec35541f77b87e212fc5f/src/Symfony/Component/Translation/LoggingTranslator.php#L33) a logging service doesn't make sense without a [hard `logger` dependency](https://github.com/symfony/symfony/blob/6bbb5bcc5294fdd540bec35541f77b87e212fc5f/src/Symfony/Bundle/FrameworkBundle/Resources/config/translation.xml#L25-L29) and according to: https://github.com/symfony/symfony/blob/6bbb5bcc5294fdd540bec35541f77b87e212fc5f/src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php#L52-L54 it must be removed if `logger` alias doesn't exists. Commits ------- eedad4c Make logger arg a hard dependency, remove dead code and add tests

carsonbot added the Status: Needs Review label Apr 13, 2018

yceruto force-pushed the messenger/logging_middleware branch 6 times, most recently from a35bbe2 to 0f87c02 Compare April 13, 2018 04:45

yceruto commented Apr 13, 2018

View reviewed changes

ogizanagi reviewed Apr 13, 2018

View reviewed changes

sroze added the Messenger label Apr 13, 2018

sroze suggested changes Apr 13, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 13, 2018

yceruto force-pushed the messenger/logging_middleware branch 2 times, most recently from 462ca65 to dafd241 Compare April 13, 2018 14:22

yceruto force-pushed the messenger/logging_middleware branch 7 times, most recently from 45ca387 to ecdb38c Compare April 13, 2018 17:09

carsonbot removed the Status: Needs Work label Apr 13, 2018

ogizanagi approved these changes Apr 13, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Apr 13, 2018

yceruto commented Apr 13, 2018

View reviewed changes

nicolas-grekas added this to the 4.1 milestone Apr 14, 2018

nicolas-grekas approved these changes Apr 14, 2018 •

edited

Loading

View reviewed changes

carsonbot removed the Status: Reviewed label Apr 14, 2018

nicolas-grekas previously requested changes Apr 14, 2018

View reviewed changes

carsonbot added the Status: Needs Work label Apr 14, 2018

yceruto force-pushed the messenger/logging_middleware branch from ecdb38c to eedad4c Compare April 14, 2018 17:36

Make logger arg a hard dependency, remove dead code and add tests

eedad4c

carsonbot added Status: Needs Review and removed Status: Needs Work labels Apr 14, 2018

sroze approved these changes Apr 15, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 15, 2018

sroze merged commit eedad4c into symfony:master Apr 15, 2018

yceruto deleted the messenger/logging_middleware branch April 16, 2018 11:14

	if (!$container->getParameter('kernel.debug') \|\| !$container->has('logger')) {
	$container->removeDefinition('messenger.middleware.debug.logging');
	}

		@@ -1446,6 +1446,10 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder

		$loader->load('messenger.xml');

		if (!$container->getParameter('kernel.debug') \|\| !$container->hasAlias('logger')) {

		@@ -52,7 +52,7 @@

		<!-- Logging & Debug -->
		<service id="messenger.middleware.debug.logging" class="Symfony\Component\Messenger\Debug\LoggingMiddleware">

Uh oh!

[Messenger] Testing LoggingMiddleware and minor improvements #26912

[Messenger] Testing LoggingMiddleware and minor improvements #26912

Uh oh!

Conversation

yceruto commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yceruto Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yceruto commented Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ogizanagi commented Apr 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagi Apr 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

yceruto commented Apr 14, 2018

Uh oh!

sroze commented Apr 15, 2018

Uh oh!

Uh oh!

yceruto commented Apr 13, 2018 •

edited

Loading

yceruto Apr 13, 2018 •

edited

Loading

yceruto commented Apr 13, 2018 •

edited

Loading

ogizanagi Apr 14, 2018 •

edited

Loading