[FrameworkBundle][Notifier] Allow to configure or disable the message bus to use #39353
Conversation
4e9a72c to
d7f207a
Compare
|
It looks like your commit is not associated with your GitHub account: And FrameworkBundle is twice in the PR header 🧐 |
|
Anything left todo here @jschaedl ? |
|
Hi @OskarStark |
|
Can you please rebase and see if it's still failing ? Thanks |
0952835 to
13a1266
Compare
|
@jschaedl can you please check the failing tests? Thanks |
fe8b2ac to
2e8cd93
Compare
|
@OskarStark I rebased the branch. Tests are still failing on PHP 8.0 but this seems to be unrelated with my changes. |
|
The failures are related I believe. |
61acd5c to
3e800b6
Compare
|
Please rebase to get rid of Travis, thanks |
728afd7 to
616cdd6
Compare
|
@OskarStark Rebase done. |
| if ($container->hasDefinition($serviceId)) { | ||
| $container->getDefinition($serviceId) | ||
| ->setArgument(0, null) | ||
| ->replaceArgument(1, $messageBus ? new Reference($messageBus) : new Reference('messenger.default_bus', ContainerInterface::NULL_ON_INVALID_REFERENCE)) |
There was a problem hiding this comment.
$messageBus ? ... can be removed because we are in the else part.
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
|
Some tests are failing |
| @@ -2390,6 +2390,24 @@ private function registerNotifierConfiguration(array $config, ContainerBuilder $ | |||
| $container->removeDefinition('notifier.channel.email'); | |||
| } | |||
|
|
|||
| $servicesWithBusArgument = ['texter', 'chatter', 'notifier.channel.chat', 'notifier.channel.email', 'notifier.channel.sms']; | |||
| if (false === $messageBus = $config['message_bus']) { | |||
There was a problem hiding this comment.
should we throw an exception if one tries to configure message_bus while messenger is not installed/enabled?
There was a problem hiding this comment.
The logic is used here for setting the messageBus argument on the mailer.mailer service in the same way. I thought using ContainerInterface::NULL_ON_INVALID_REFERENCE is enough?
|
@jschaedl Can you have a look at the failing tests? |
77db22d to
95718bb
Compare
|
@jschaedl I've rebased on current 6.2 and fixed the tests. Can you review my commit before I merge? |
| foreach ($servicesWithBusArgument as $serviceId) { | ||
| if ($container->hasDefinition($serviceId)) { | ||
| $container->getDefinition($serviceId) | ||
| ->setArgument(0, null) |
There was a problem hiding this comment.
I don't get why you are removing the transports here. We do need the transports in all cases.
There was a problem hiding this comment.
Not sure what you mean, but I am not removing the transports. I am adding the message bus service to every service with a messageBus argument, in case the message_bus config is filled.
|
Friendly ping @jschaedl |
68f31ac to
fc7aaa6
Compare
|
I rebased again. Looks good to me now. |
|
Thank you @jschaedl. |
For the mailer integration there is already a possibility to disable or configure the
message_busto use. See: #34633 and #34648This PR introduces a similar config option
notifier.message_busfor the notifier integration.Things done:
notifier.message_busfor the notifier configurationmessage_busconfig option