Skip to content
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

Jump to bottom

[FrameworkBundle][Notifier] Allow to configure or disable the message bus to use #39353

Open
wants to merge 2 commits into
base: 6.2
Choose a base branch
from
Open

[FrameworkBundle][Notifier] Allow to configure or disable the message bus to use #39353

wants to merge 2 commits into from

Conversation

jschaedl
Copy link
Contributor

@jschaedl jschaedl commented Dec 6, 2020 •
edited by OskarStark

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR tbd

For the mailer integration there is already a possibility to disable or configure the message_bus to use. See: #34633 and #34648

This PR introduces a similar config option notifier.message_bus for the notifier integration.

Things done:

  • Add a config option notifier.message_bus for the notifier configuration
  • Adjust FrameworkExtension considering the new message_bus config option
  • Add tests for the new config option

@carsonbot carsonbot changed the title [Frameworkbundle][Notifier] Allow to configure or disable the message bus to use [FrameworkBundle] [Frameworkbundle][Notifier] Allow to configure or disable the message bus to use Dec 6, 2020
@carsonbot carsonbot changed the title [FrameworkBundle] [Frameworkbundle][Notifier] Allow to configure or disable the message bus to use [FrameworkBundle][Notifier] [Frameworkbundle] Allow to configure or disable the message bus to use Dec 6, 2020
@jschaedl jschaedl force-pushed the notifier-disable-message-bus branch 2 times, most recently from 4e9a72c to d7f207a Compare Dec 6, 2020
@jderusse jderusse added this to the 5.x milestone Dec 6, 2020
@OskarStark
Copy link
Contributor

OskarStark commented Dec 10, 2020

It looks like your commit is not associated with your GitHub account:
image

And FrameworkBundle is twice in the PR header 🧐

@nicolas-grekas nicolas-grekas changed the title [FrameworkBundle][Notifier] [Frameworkbundle] Allow to configure or disable the message bus to use [FrameworkBundle][Notifier] Allow to configure or disable the message bus to use Dec 10, 2020
@OskarStark
Copy link
Contributor

OskarStark commented Jan 30, 2021

Anything left todo here @jschaedl ?

@jschaedl
Copy link
Contributor Author

jschaedl commented Jan 31, 2021 •
edited

Hi @OskarStark
Implementation is done. But I have no idea why tests are failing on PHP 7.4 🤔

@OskarStark
Copy link
Contributor

OskarStark commented Jan 31, 2021

Can you please rebase and see if it's still failing ? Thanks

@jschaedl jschaedl force-pushed the notifier-disable-message-bus branch 2 times, most recently from 0952835 to 13a1266 Compare Jan 31, 2021
@OskarStark
Copy link
Contributor

OskarStark commented Apr 6, 2021

@jschaedl can you please check the failing tests?

Thanks

@jschaedl jschaedl force-pushed the notifier-disable-message-bus branch 2 times, most recently from fe8b2ac to 2e8cd93 Compare Apr 6, 2021
@jschaedl
Copy link
Contributor Author

jschaedl commented Apr 6, 2021

@OskarStark I rebased the branch. Tests are still failing on PHP 8.0 but this seems to be unrelated with my changes.

@OskarStark OskarStark requested a review from jderusse Apr 6, 2021
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 13, 2021

The failures are related I believe.

@jschaedl jschaedl force-pushed the notifier-disable-message-bus branch 2 times, most recently from 61acd5c to 3e800b6 Compare Apr 17, 2021
@OskarStark OskarStark requested a review from Nyholm Apr 17, 2021
@jschaedl jschaedl force-pushed the notifier-disable-message-bus branch 2 times, most recently from 1667610 to 804185f Compare Apr 18, 2021
@jschaedl jschaedl force-pushed the notifier-disable-message-bus branch from 804185f to 63a5546 Compare May 2, 2021
@jschaedl
Copy link
Contributor Author

jschaedl commented May 3, 2021

Rebased and ready for another review :-)

@jschaedl jschaedl mentioned this pull request May 13, 2021
Closed
@OskarStark
Copy link
Contributor

OskarStark commented Jun 17, 2021

Ready to merge @nicolas-grekas ?

@OskarStark OskarStark requested a review from derrabus Aug 4, 2021
@OskarStark
Copy link
Contributor

OskarStark commented Aug 4, 2021

Please rebase to get rid of Travis, thanks

@jschaedl jschaedl force-pushed the notifier-disable-message-bus branch from 728afd7 to 616cdd6 Compare Aug 17, 2021
@jschaedl
Copy link
Contributor Author

jschaedl commented Aug 17, 2021

@OskarStark Rebase done.

jderusse
jderusse reviewed Aug 17, 2021
View changes
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Outdated
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))
Copy link
Member

@jderusse jderusse Aug 17, 2021

Choose a reason for hiding this comment

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

$messageBus ? ... can be removed because we are in the else part.

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Outdated Show resolved Hide resolved
@chalasr
Copy link
Member

chalasr commented Aug 19, 2021

Some tests are failing

chalasr
chalasr reviewed Aug 19, 2021
View changes
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Outdated
@@ -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']) {
Copy link
Member

@chalasr chalasr Aug 19, 2021

Choose a reason for hiding this comment

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

should we throw an exception if one tries to configure message_bus while messenger is not installed/enabled?

@fabpot
Copy link
Member

fabpot commented Oct 19, 2021

@jschaedl Can you have a look at the failing tests?

@fabpot fabpot modified the milestones: 5.4, 6.1 Nov 16, 2021
@fabpot fabpot modified the milestones: 6.1, 6.2 May 20, 2022
@fabpot fabpot force-pushed the notifier-disable-message-bus branch from 77db22d to 95718bb Compare Jul 21, 2022
@fabpot
Copy link
Member

fabpot commented Jul 21, 2022

@jschaedl I've rebased on current 6.2 and fixed the tests. Can you review my commit before I merge?

fabpot
fabpot reviewed Jul 21, 2022
View changes
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Outdated
foreach ($servicesWithBusArgument as $serviceId) {
if ($container->hasDefinition($serviceId)) {
$container->getDefinition($serviceId)
->setArgument(0, null)
Copy link
Member

@fabpot fabpot Jul 21, 2022

Choose a reason for hiding this comment

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

I don't get why you are removing the transports here. We do need the transports in all cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot

@jderusse jderusse

@OskarStark OskarStark

@ogizanagi ogizanagi

@chalasr chalasr

@Nyholm Nyholm

@nicolas-grekas nicolas-grekas

@derrabus derrabus

Assignees
No one assigned
Labels
Feature FrameworkBundle Notifier Status: Needs Review
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@jschaedl @OskarStark @nicolas-grekas @chalasr @fabpot @jderusse @ogizanagi @carsonbot