Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

sroze
Copy link
Contributor

@sroze sroze commented Oct 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.

@sroze sroze force-pushed the messenger-partial-parent-middleware-arguments branch from c9b4d87 to c9ab7ba Compare October 30, 2018 21:15
@sroze sroze requested review from nicolas-grekas, stof and ogizanagi and removed request for stof October 30, 2018 21:15
@nicolas-grekas
Copy link
Member

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

@nicolas-grekas nicolas-grekas added this to the 4.2 milestone Oct 30, 2018
ogizanagi
ogizanagi previously approved these changes Oct 30, 2018
Copy link
Contributor

@ogizanagi 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.

@@ -494,12 +494,14 @@ public function testRegistersMiddlewareFromServices()
$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)

$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

@nicolas-grekas
Copy link
Member

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

@ogizanagi
Copy link
Contributor

ogizanagi commented Oct 30, 2018

@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.yaml
framework:
    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
Copy link
Contributor

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 use foo(),
either you need to change a value, i.e $baz and use foo('foo', 5), not foo($baz=5). And it's fine to me here.

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

@ogizanagi ogizanagi dismissed their stale review October 31, 2018 07:13

Cf previous comment

@nicolas-grekas
Copy link
Member

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

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Oct 31, 2018

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

nope: my suggestion is to make doctrine_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.

@nicolas-grekas
Copy link
Member

Already fixed in #29010 (comment) by removing special edge cases, instead of adding more ;)

@sroze sroze closed this Oct 31, 2018
@sroze sroze deleted the messenger-partial-parent-middleware-arguments branch October 31, 2018 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants