Movatterモバイル変換

yceruto force-pushed themessenger/logging_middleware branch 6 times, most recently froma35bbe2 to0f87c02Compare

April 13, 2018 04:45

yceruto commented

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

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

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

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

yceruto commented

src/Symfony/Component/Messenger/composer.json Outdated

		"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",

Copy link

MemberAuthor

ycerutoApr 13, 2018•
edited
Loading

Choose a reason for hiding this comment

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

Removing as duplicated entry (unrelated change)

ogizanagi reviewed

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

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

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

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

Copy link

Contributor

Choose a reason for hiding this comment

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

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.

Copy link

Contributor

Choose a reason for hiding this comment

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

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

Copy link

Contributor

Choose a reason for hiding this comment

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

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

Copy link

MemberAuthor

ycerutoApr 13, 2018

Choose a reason for hiding this comment

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

I'll try this again soon.

sroze added the Messenger label

sroze suggested changes

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

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

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

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

Copy link

Contributor

srozeApr 13, 2018

Choose a reason for hiding this comment

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

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

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

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

		(new LoggingMiddleware($logger))->handle($message, $next);

Copy link

Contributor

srozeApr 13, 2018

Choose a reason for hiding this comment

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

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

Copy link

MemberAuthor

ycerutoApr 13, 2018

Choose a reason for hiding this comment

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

Done.

carsonbot added Status: Needs Work and removed Status: Needs Review labels

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

yceruto force-pushed themessenger/logging_middleware branch 2 times, most recently from462ca65 todafd241Compare

April 13, 2018 14:22

Copy link

MemberAuthor

yceruto commentedApr 13, 2018•
edited
Loading

@ogizanagi thetests fail again, I guessLoggerPass is processed afterMessengerPass? or not?

Line 90 in6bbb5bc

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

Any suggestions?

Copy link

Contributor

ogizanagi commentedApr 13, 2018

@yceruto : IIUC, the FrameworkExtensionTest does not build the FrameworkBundle, so compiler passes like theLoggerPass 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 force-pushed themessenger/logging_middleware branch 7 times, most recently from45ca387 toecdb38cCompare

April 13, 2018 17:09

carsonbot removed the Status: Needs Work label

ogizanagi approved these changes

carsonbot added the Status: Reviewed label

yceruto commented

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">

Copy link

MemberAuthor

ycerutoApr 13, 2018

Choose a reason for hiding this comment

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

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

Copy link

Contributor

ogizanagiApr 14, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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 added this to the4.1 milestone

nicolas-grekas approved these changes

•

edited

carsonbot removed the Status: Reviewed label

nicolas-grekas previously requested changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

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

carsonbot added the Status: Needs Work label

yceruto force-pushed themessenger/logging_middleware branch fromecdb38c toeedad4cCompare

April 14, 2018 17:36

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

eedad4c

Copy link

MemberAuthor

yceruto commentedApr 14, 2018

Done! (failures are unrelated)

Status: Needs Review

carsonbot added Status: Needs Review and removed Status: Needs Work labels

sroze approved these changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

sroze dismissednicolas-grekas’sstale review

April 15, 2018 21:28

Changes done

Copy link

Contributor

sroze commentedApr 15, 2018

Thank you@yceruto.

sroze merged commiteedad4c intosymfony:master

sroze added a commit that referenced this pull request