[Notifier] Inject Mailer instead of service locator for FakeSms and FakeChat #40739

OskarStark · 2021-04-08T11:35:30Z

Q	A
Branch?	5.x
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fixes #40731
License	MIT
Doc PR	symfony/symfony-docs#15206
Recipe PR	symfony/recipes#930

Until now the locator was not injected and therefore not working.

We decided to make the transport name configurable instead of the service_id.

How is it working?

Todos

add tests
test in a real project

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

OskarStark · 2021-04-08T12:25:46Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -2417,6 +2417,16 @@ private function registerNotifierConfiguration(array $config, ContainerBuilder $
            $container->removeDefinition($classToServices[MercureTransportFactory::class]);
        }

+        if (ContainerBuilder::willBeAvailable('symfony/fake-chat-notifier', FakeChatTransportFactory::class, array_merge($parentPackages, ['symfony/mailer']))) {


Not sure this line is correct 🤔

I only want to run this code if fake-chat-notifier with FakeChatTransportFactory::class and symfony/mailer is installed. Am I using willBeAvailable method correct?

Friendly ping @stof

The part I'm not sure is the array_merge. I don't remember whether passing multiple parent packages requires all of them or at least one of them. /cc @nicolas-grekas

Regarding this code:

symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php

Lines 1469 to 1495 in ddb48bb

/**

* Checks whether a class is available and will remain available in the "no-dev" mode of Composer.

*

* When parent packages are provided and if any of them is in dev-only mode,

* the class will be considered available even if it is also in dev-only mode.

*/

final public static function willBeAvailable(string $package, string $class, array $parentPackages): bool

{

if (!class_exists($class) && !interface_exists($class, false) && !trait_exists($class, false)) {

return false;

}

if (!class_exists(InstalledVersions::class) || !InstalledVersions::isInstalled($package) || InstalledVersions::isInstalled($package, false)) {

return true;

}

// the package is installed but in dev-mode only, check if this applies to one of the parent packages too

$rootPackage = InstalledVersions::getRootPackage()['name'] ?? '';

foreach ($parentPackages as $parentPackage) {

if ($rootPackage === $parentPackage || (InstalledVersions::isInstalled($parentPackage) && !InstalledVersions::isInstalled($parentPackage, false))) {

return true;

}

}

return false;

}

I would say it just checks if at least one of them is available 🤔

meaning that if you want to ensure that several parent packages are all installed, you should do that as ContainerBuilder::willBeAvailable(..., $parent1) && ContainerBuilder::willBeAvailable(..., $parent2) 😄

Yes, but in this case I am wondering if the previous used one by mercure is working as expected and needs to do the call twice too 🤔

cc @mtarld

Yes indeed, the call is done twice for mercure

Yes but you are using $parentPackages, but I think you need to achieve, that both, notifier and framework-bundle need to be available, right? But this would not work.

To be more clear, I think we need to change your code to:

diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php index 4d4749a2f1..f36702d254 100644 --- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php +++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php @@ -2410,10 +2410,16 @@ class FrameworkExtension extends Extension } } - if (ContainerBuilder::willBeAvailable('symfony/mercure-notifier', MercureTransportFactory::class, $parentPackages) && ContainerBuilder::willBeAvailable('symfony/mercure-bundle', MercureBundle::class, $parentPackages)) { + if (ContainerBuilder::willBeAvailable('symfony/mercure-notifier', MercureTransportFactory::class, ['symfony/framework-bundle']) + && ContainerBuilder::willBeAvailable('symfony/mercure-notifier', MercureTransportFactory::class, ['symfony/notifier']) + && ContainerBuilder::willBeAvailable('symfony/mercure-bundle', MercureBundle::class, ['symfony/framework-bundle']) + && ContainerBuilder::willBeAvailable('symfony/mercure-bundle', MercureBundle::class, ['symfony/notifier']) + ) { $container->getDefinition($classToServices[MercureTransportFactory::class]) ->replaceArgument('$registry', new Reference(HubRegistry::class)); - } elseif (ContainerBuilder::willBeAvailable('symfony/mercure-notifier', MercureTransportFactory::class, $parentPackages)) { + } elseif (ContainerBuilder::willBeAvailable('symfony/mercure-notifier', MercureTransportFactory::class, ['symfony/framework-bundle']) + && ContainerBuilder::willBeAvailable('symfony/mercure-notifier', MercureTransportFactory::class, ['symfony/notifier']) + ) { $container->removeDefinition($classToServices[MercureTransportFactory::class]); :

if yes maybe it makes sense to either extend the willBeAvailable method to a 4th parameter bool $all = false or create a allWillBeAvailable and deprecated $parentPackages to be an array in willBeAvailable method.

cc @nicolas-grekas

PS: I am unsure because I use this method the first time 😸

src/Symfony/Component/Notifier/Bridge/FakeChat/FakeChatEmailTransport.php

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

OskarStark · 2021-04-09T07:47:47Z

@Nyholm any idea what's wrong here?

OskarStark · 2021-04-09T11:14:17Z

I tested this in my project with the following command:

<?php

declare(strict_types=1);

namespace App\Command;

use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\Notifier\ChatterInterface;
use Symfony\Component\Notifier\Message\ChatMessage;
use Symfony\Component\Notifier\Message\SmsMessage;
use Symfony\Component\Notifier\Tests\Fixtures\TestOptions;
use Symfony\Component\Notifier\TexterInterface;

final class TestFakeBridgesCommand extends Command
{
    protected static $defaultName = 'test:fake-bridges';

    public function __construct(
        private ChatterInterface $chatter,
        private TexterInterface $texter,
    ) {
        parent::__construct();
    }

    protected function configure(): void
    {
        $this
            ->setDescription('Test FakeSms and FakeChat bridge');
    }

    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        // SMS
        $message = new SmsMessage('1234567', 'Hi Symfony');
        $this->texter->send($message);

        // ChatMessage without recipient
        $message = new ChatMessage('Hi Symfony!');
        $this->chatter->send($message);

        // Chat Message with recipient
        $message = new ChatMessage('Hi Symfony!', new TestOptions(['recipient_id' => 'Oskar']));
        $this->chatter->send($message);

        return Command::SUCCESS;
    }
}

Results 👍

OskarStark · 2021-04-09T11:19:48Z

src/Symfony/Component/Notifier/Tests/Fixtures/TestOptions.php

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Notifier\Tests\Fixtures;


Namespace correct?

OskarStark · 2021-04-09T11:20:10Z

src/Symfony/Component/Notifier/Tests/Mailer/DummyMailer.php

+/**
+ * @author Oskar Stark <oskarstark@googlemail.com>
+ */
+class DummyMailer implements MailerInterface


Or is there sth similar available? 🤔

Do you have a proposal for a better name?

Nyholm · 2021-04-09T11:25:01Z

Yes, you are missing .gitignore

OskarStark · 2021-04-09T11:55:59Z

Oh I overlooked this, thanks .gitignore added

chalasr

Thanks

…akeChat

This PR was merged into the 5.3-dev branch. Discussion ---------- [Notifier] [FakeSms] [FakeChat] Update DSN Follows symfony/symfony#40739 Commits ------- 3833bba [Notifier] [FakeSms] [FakeChat] Update DSN

OskarStark self-assigned this Apr 8, 2021

carsonbot added Status: Needs Review Bug Notifier labels Apr 8, 2021

OskarStark commented Apr 8, 2021

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Outdated Show resolved Hide resolved

OskarStark commented Apr 8, 2021

View reviewed changes

chalasr reviewed Apr 8, 2021

View reviewed changes

src/Symfony/Component/Notifier/Bridge/FakeChat/FakeChatEmailTransport.php Show resolved Hide resolved

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Outdated Show resolved Hide resolved

This was referenced Apr 8, 2021

[Notifier] [FakeSms] [FakeChat] Update DSN symfony/symfony-docs#15206

Merged

[Notifier] [FakeSms] [FakeChat] Fix DSN symfony/recipes#930

Merged

chalasr added this to the 5.x milestone Apr 8, 2021

OskarStark force-pushed the fix/service-id branch 2 times, most recently from 035a59e to 3c6a2ef Compare April 9, 2021 10:58

OskarStark force-pushed the fix/service-id branch 2 times, most recently from 3fb8a3f to 7fcaed7 Compare April 9, 2021 11:15

OskarStark changed the title ~~[Notifier] Fix service locator for fake-chat/sms~~ [Notifier] Inject Mailer instead of service locator for FakeSms and FakeCha Apr 9, 2021

OskarStark requested review from chalasr and Nyholm April 9, 2021 11:16

OskarStark commented Apr 9, 2021

View reviewed changes

OskarStark force-pushed the fix/service-id branch from 7fcaed7 to a29908e Compare April 9, 2021 11:55

OskarStark changed the title ~~[Notifier] Inject Mailer instead of service locator for FakeSms and FakeCha~~ [Notifier] Inject Mailer instead of service locator for FakeSms and FakeChat Apr 9, 2021

stof approved these changes Apr 9, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 9, 2021

chalasr approved these changes Apr 10, 2021

View reviewed changes

[Notifier] Inject Mailer instead of service locator for FakeSms and F…

0f6d507

…akeChat

OskarStark force-pushed the fix/service-id branch from a29908e to 0f6d507 Compare April 12, 2021 09:05

OskarStark merged commit 9092d5b into symfony:5.x Apr 12, 2021

	/**
	* Checks whether a class is available and will remain available in the "no-dev" mode of Composer.
	*
	* When parent packages are provided and if any of them is in dev-only mode,
	* the class will be considered available even if it is also in dev-only mode.
	*/
	final public static function willBeAvailable(string $package, string $class, array $parentPackages): bool
	{
	if (!class_exists($class) && !interface_exists($class, false) && !trait_exists($class, false)) {
	return false;
	}

	if (!class_exists(InstalledVersions::class) \|\| !InstalledVersions::isInstalled($package) \|\| InstalledVersions::isInstalled($package, false)) {
	return true;
	}

	// the package is installed but in dev-mode only, check if this applies to one of the parent packages too

	$rootPackage = InstalledVersions::getRootPackage()['name'] ?? '';
	foreach ($parentPackages as $parentPackage) {
	if ($rootPackage === $parentPackage \|\| (InstalledVersions::isInstalled($parentPackage) && !InstalledVersions::isInstalled($parentPackage, false))) {
	return true;
	}
	}

	return false;
	}

Uh oh!

[Notifier] Inject Mailer instead of service locator for FakeSms and FakeChat #40739

[Notifier] Inject Mailer instead of service locator for FakeSms and FakeChat #40739

Uh oh!

Conversation

OskarStark commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todos

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

OskarStark commented Apr 9, 2021

Uh oh!

OskarStark commented Apr 9, 2021

Results 👍

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OskarStark Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nyholm commented Apr 9, 2021

Uh oh!

OskarStark commented Apr 9, 2021

Uh oh!

chalasr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

OskarStark commented Apr 8, 2021 •

edited

Loading

OskarStark Apr 9, 2021 •

edited

Loading