-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
[Messenger] Allow to override only a few of the abstract middleware definition #29034
Conversation
c9b4d87
to
c9ab7ba
Compare
I think there is a better way, see https://github.com/symfony/symfony/pull/29010/files#r229492831 |
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.
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); |
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.
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' |
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.
parent arguments are overwritten by index, and next ones appended
@ogizanagi see my comment linked above. I don't think we should follow this path. |
@nicolas-grekas : Continuing the discussion here, as it's seems more scoped here.
The current logic typically allows this: https://symfony.com/doc/current/messenger.html#using-middleware-factories # 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. |
But actually, I'm not fan of this PR allowing to replace any argument by index through the config, just for fixing this issue. function foo($bar = 'foo', $baz = 3, $qux = null); either you're satisfied with defaults and use So about this issue, I'd suggest a more pragmatic approach: #29038 |
👎 let's kill these workarounds on top of hacks. We need to play by the rules of the definition inheritance, not against. |
nope: my suggestion is to make |
Already fixed in #29010 (comment) by removing special edge cases, instead of adding more ;) |
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.