[FrameworkBundle][Notifier] Fix service registration (MessageBird + TurboSms)

[smnandre]

Q	A
Branch?	5.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	Fix #
License	MIT

Inside the registerNotifierConfiguration method, the $classToServices map (TransportFactoryClass => serviceId) used the Transport class (instead of the TransportFactory) for MessageBird and TurboSms.

The services are registered in Ressources/config/notifier_transports.php (with the correct Factory classes).

This map is then used in this loop:

foreach ($classToServices as $class => $service) {
  $package = substr($service, \strlen('notifier.transport_factory.'));

  if (!ContainerBuilder::willBeAvailable(sprintf('symfony/%s-notifier', $package), $class, $parentPackages, true)) {
      $container->removeDefinition($service);
      $container->removeAlias(str_replace('-', '', $service)); // @deprecated to be removed in 6.0
  }
}

So i'm not sure it's considered a bug as IRL checking the Factory class or the Transport class did not make any difference as i read it.... but it's worth a fix for the future :)

So i target 5.4 with no certitude :)

[smnandre]

(fabbot unrelated)

[OskarStark]

Good catch, to avoid such mistakes we could check inside the loop if class instanceof AbstractFactory, but that would be on runtime, and I don't think its worth

WDYT @nicolas-grekas ?

[smnandre]

Maybe naive but is something like that possible ?

-foreach ($classToServices as $class => $service) {
+foreach ($classToServices as $service) {
    $package = substr($service, \strlen('notifier.transport_factory.'));

-    if (!ContainerBuilder::willBeAvailable(sprintf('symfony/%s-notifier', $package), $class, $parentPackages)) {
+    if (!ContainerBuilder::willBeAvailable(sprintf('symfony/%s-notifier', $package), $container->getDefinition($service)->getClass(), $parentPackages)) {
        $container->removeDefinition($service);
    }
  }

And then keep only the ids (that may be collected thanks to the tag too i guess?)

[nicolas-grekas]

Maybe naive but is something like that possible ?

Possible, can you give it a try on 7.1?

(that may be collected thanks to the tag too i guess?)

That'd require a compiler pass. Which is maybe not a bad idea since it would allow registering external notifiers/etc.
To be confirmed there's a use case for that.

[nicolas-grekas]

Thank you @smnandre.

[smnandre]

Possible, can you give it a try on 7.1?

Yep, i open a PR if that seems ok ?