NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Messenger] Allow to override only a few of the abstract middleware definition#29034

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

sroze wants to merge1 commit intosymfony:masterfromsroze:messenger-partial-parent-middleware-arguments

Closed

[Messenger] Allow to override only a few of the abstract middleware definition#29034

sroze wants to merge1 commit intosymfony:masterfromsroze:messenger-partial-parent-middleware-arguments

Conversation

Copy link

Contributor

sroze commentedOct 30, 2018

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

The recent PR about the traceable middleware introduced an issue. This PR fixes it by enabling us to override only some parameters of the parent service.

carsonbot added Status: Needs Review Messenger Bug labels

Oct 30, 2018

Allow to override only a few of the abstract middleware definition

c9ab7ba

sroze force-pushed themessenger-partial-parent-middleware-arguments branch fromc9b4d87 toc9ab7baCompare

October 30, 2018 21:15

sroze requested review fromnicolas-grekas,ogizanagi andstof and removed request forstof

October 30, 2018 21:15

Copy link

Member

nicolas-grekas commentedOct 30, 2018

I think there is a better way, seehttps://github.com/symfony/symfony/pull/29010/files#r229492831

nicolas-grekas added this to the4.2 milestone

Oct 30, 2018

ogizanagi previously approved these changes

Oct 30, 2018

View reviewed changes

Copy link

Contributor

ogizanagi left a comment

Choose a reason for hiding this comment

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

Good catch. Just really minor comments.

src/Symfony/Component/Messenger/Tests/DependencyInjection/MessengerPassTest.php

		$container =$this->getContainerBuilder($fooBusId ='messenger.bus.foo');
		$container->register('middleware_with_factory', UselessMiddleware::class)->addArgument('some_default')->setAbstract(true);
		$container->register('middleware_with_factory_using_default', UselessMiddleware::class)->addArgument('some_default')->setAbstract(true);
		$container->register('middleware_with_factory_using_a_lot_of_defaults', UselessMiddleware::class)->setArguments(array('one','two','three'))->setAbstract(true);

Copy link

Contributor

Choose a reason for hiding this comment

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

middleware_with_factory_replacing_defaults_by_index ? 😅 ("Using a lot of defaults" doesn't really convey the intent here)

src/Symfony/Component/Messenger/Tests/DependencyInjection/MessengerPassTest.php

		$this->assertEquals(
		array('one','2','three','four'),
		$container->getDefinition($factoryWithALotOfDefaultsChildMiddlewareId)->getArguments(),
		'parent arguments are not overwritten'

Copy link

Contributor

Choose a reason for hiding this comment

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

parent arguments are overwritten by index, and next ones appended

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

Oct 30, 2018

Copy link

Member

nicolas-grekas commentedOct 30, 2018

@ogizanagi see my comment linked above. I don't think we should follow this path.

Copy link

Contributor

ogizanagi commentedOct 30, 2018•
edited
Loading

@nicolas-grekas : Continuing the discussion here, as it's seems more scoped here.

all this logic should be removed: ChildDefinition already have their merging logic. Overwriting an existing parent argument is done by using index_0 as a key. Let's make things simpler by reusing existing knowledge.

The current logic typically allows this:https://symfony.com/doc/current/messenger.html#using-middleware-factories
Unless I miss something, your suggestion will require writing:

# config/packages/messenger.yamlframework:messenger:buses:command_bus:middleware:                    -doctrine_transaction_middleware:['index_0' => 'custom']

to change the entity manager name.

The idea of such factories was to expose simple config for middlewares. Not leaking Symfony's DIC specificities into it.

ogizanagi mentioned this pull request

Oct 31, 2018

[Messenger][FrameworkBundle] Fix bus name on traceable middleware#29038

Closed

Copy link

Contributor

ogizanagi commentedOct 31, 2018

But actually, I'm not fan of this PR allowing to replace any argument by index through the config, just for fixing this issue.
The middleware factory args mimic defaults in a PHP function. I.e, given:

function foo($bar ='foo',$baz =3,$qux =null);

either you're satisfied with defaults and usefoo(),
either you need to change a value, i.e$baz and usefoo('foo', 5), notfoo($baz=5). And it's fine to me here.

So about this issue, I'd suggest a more pragmatic approach:#29038

ogizanagi dismissed theirstale review

October 31, 2018 07:13

Cf previous comment

Copy link

Member

nicolas-grekas commentedOct 31, 2018

👎 let's kill these workarounds on top of hacks. We need to play by the rules of the definition inheritance, not against.

Copy link

Member

nicolas-grekas commentedOct 31, 2018•
edited
Loading

doctrine_transaction_middleware: ['index_0' => 'custom']

nope: my suggestion is to makedoctrine_transaction_middleware: ['custom'] work.
This can easily be done by playing the rules of definition inheritance: the bus name should be undefined in the parent definition, and be the first next argument in the constructor definition.