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

Add an adapter to support the Symfony Mailer component and deprecate the Swiftmailer integration #102

Merged
merged 1 commit into from
Jun 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 31 additions & 2 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -66,5 +66,34 @@ jobs:
run: composer test

-
name: Run lint container
run: (cd src/Bundle/test && bin/console lint:container)
name: Run lint container (with all services)
run: |
rm -rf src/Bundle/test/var/cache
(cd src/Bundle/test && bin/console lint:container)

-
name: Run lint container (with Swift Mailer)
run: |
rm -rf src/Bundle/test/var/cache
mv src/Bundle/test/config/packages/mailer.yaml mailer.yaml
composer remove symfony/mailer --dev
(cd src/Bundle/test && bin/console lint:container)
Copy link
Member

Choose a reason for hiding this comment

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

if we keep this test suite this way for a moment, I think we have to add composer require --dev symfony/mailer at the end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m not following what it is you’re suggesting to do. The order of operations for all the Composer commands is fine as is, there is no point in doing a full reset to the starting state at the end of each lint step (everything a step is doing is building off the state the previous step left things in).

Copy link
Member

Choose a reason for hiding this comment

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

I think depending on a previous one is a little bit risky. On resource bundle, we do that : https://github.com/Sylius/SyliusResourceBundle/blob/1.10/.github/workflows/build.yml#L106

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By that same logic, you can’t rely on the previous steps to have set up the environment correctly either 😉 (if these were parallel or separately running jobs it’d be another story, but it’s a single threaded build).

The only difference in workflows is where certain commands are run, and running less Composer commands overall since each step builds on the previous.


-
name: Run lint container (with symfony/mailer)
run: |
rm -rf src/Bundle/test/var/cache
mv mailer.yaml src/Bundle/test/config/packages/mailer.yaml
mv src/Bundle/test/config/bundles.php src/Bundle/test/config/bundles_swift.php
mv src/Bundle/test/config/bundles_no_swift.php src/Bundle/test/config/bundles.php
composer remove symfony/swiftmailer-bundle --dev
composer require symfony/mailer --dev
loic425 marked this conversation as resolved.
Show resolved Hide resolved
(cd src/Bundle/test && bin/console lint:container)
loic425 marked this conversation as resolved.
Show resolved Hide resolved

-
name: Run lint container (with no mailers)
run: |
rm -rf src/Bundle/test/var/cache
rm -f src/Bundle/test/config/packages/mailer.yaml
composer remove symfony/mailer --dev
loic425 marked this conversation as resolved.
Show resolved Hide resolved
(cd src/Bundle/test && bin/console lint:container)
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,5 @@

/composer.phar
/composer.lock

/.phpunit.result.cache
3 changes: 3 additions & 0 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
"php": "^8.0",
"sylius-labs/polyfill-symfony-event-dispatcher": "^1.0",
"symfony/config": "^5.4",
"symfony/deprecation-contracts": "^2.1 || ^3.0",
"symfony/dependency-injection": "^5.4",
"symfony/framework-bundle": "^5.4",
"symfony/http-kernel": "^5.4",
Expand All @@ -46,6 +47,8 @@
"sylius-labs/coding-standard": "^4.0",
"symfony/console": "^5.4",
"symfony/dotenv": "^5.4",
"symfony/event-dispatcher": "^5.4",
"symfony/mailer": "^5.4",
"symfony/swiftmailer-bundle": "^3.1",
"symfony/twig-bundle": "^5.4",
"vimeo/psalm": "^4.22",
Expand Down
1 change: 1 addition & 0 deletions phpstan.neon
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,5 @@ parameters:

- '/Parameter \#1 \$event of method Symfony\\Contracts\\EventDispatcher\\EventDispatcherInterface::dispatch\(\) expects object, string given\./'
- '/Method Symfony\\Contracts\\EventDispatcher\\EventDispatcherInterface::dispatch\(\) invoked with 2 parameters, 1 required\./'
- '/Cannot call method has\(\) on object\|null/'
- '/Property Sylius\\Component\\Mailer\\Model\\Email\:\:\$id is never written\, only read\./'
107 changes: 53 additions & 54 deletions src/Bundle/DependencyInjection/SyliusMailerExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,90 +13,89 @@

namespace Sylius\Bundle\MailerBundle\DependencyInjection;

use Sylius\Bundle\MailerBundle\Renderer\Adapter\EmailTwigAdapter;
use Sylius\Bundle\MailerBundle\Sender\Adapter\SwiftMailerAdapter;
use Symfony\Component\Config\FileLocator;
use Symfony\Component\DependencyInjection\ChildDefinition;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Extension\Extension;
use Symfony\Component\DependencyInjection\Loader\XmlFileLoader;
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\HttpKernel\DependencyInjection\ConfigurableExtension;
use Symfony\Component\Mailer\MailerInterface;
use Twig\Environment;

final class SyliusMailerExtension extends Extension
final class SyliusMailerExtension extends ConfigurableExtension
{
/**
* {@inheritdoc}
*/
public function load(array $configs, ContainerBuilder $container): void
protected function loadInternal(array $mergedConfig, ContainerBuilder $container): void
{
$config = $this->processConfiguration($this->getConfiguration([], $container), $configs);
$loader = new XmlFileLoader($container, new FileLocator(__DIR__ . '/../Resources/config'));
$loader->load('services.xml');

$configFiles = [
'services.xml',
];

foreach ($configFiles as $configFile) {
$loader->load($configFile);
}
$this->configureSenderAdapter($mergedConfig, $container);
$this->configureRendererAdapter($mergedConfig, $container);

$this->configureSenderAdapter($container, $config);
$this->configureRendererAdapter($container, $config);
$container->setParameter('sylius.mailer.sender_name', $mergedConfig['sender']['name']);
$container->setParameter('sylius.mailer.sender_address', $mergedConfig['sender']['address']);

$container->setParameter('sylius.mailer.sender_name', $config['sender']['name']);
$container->setParameter('sylius.mailer.sender_address', $config['sender']['address']);
$templates = $mergedConfig['templates'] ?? ['Default' => '@SyliusMailer/default.html.twig'];

$templates = $config['templates'] ?? ['Default' => '@SyliusMailer/default.html.twig'];

$container->setParameter('sylius.mailer.emails', $config['emails']);
$container->setParameter('sylius.mailer.emails', $mergedConfig['emails']);
$container->setParameter('sylius.mailer.templates', $templates);
}

/**
* {@inheritdoc}
*/
public function getConfiguration(array $config, ContainerBuilder $container): Configuration
private function configureSenderAdapter(array $mergedConfig, ContainerBuilder $container): void
{
$configuration = new Configuration();
if (!ContainerBuilder::willBeAvailable('swiftmailer/swiftmailer', \Swift::class, ['symfony/swiftmailer-bundle'])) {
$container->removeDefinition('sylius.email_sender.adapter.swiftmailer');
}

$container->addObjectResource($configuration);
if (!ContainerBuilder::willBeAvailable('symfony/mailer', MailerInterface::class, ['symfony/framework-bundle'])) {
$container->removeDefinition('sylius.email_sender.adapter.symfony_mailer');
}

return $configuration;
}
if (isset($mergedConfig['sender_adapter'])) {
$container->setAlias('sylius.email_sender.adapter', $mergedConfig['sender_adapter']);

private function configureSenderAdapter(ContainerBuilder $container, array $config): void
{
$bundles = (array) $container->getParameter('kernel.bundles');
return;
}

$defaultSenderAdapter = 'sylius.email_sender.adapter.default';
if (array_key_exists('SwiftmailerBundle', $bundles)) {
$swiftmailerAdapter = new ChildDefinition('sylius.email_sender.adapter.abstract');
$swiftmailerAdapter->setClass(SwiftMailerAdapter::class);
$swiftmailerAdapter->setArguments([new Reference('mailer'), new Reference('event_dispatcher')]);
$swiftmailerAdapter->setPublic(true);
$services = [
'sylius.email_sender.adapter.swiftmailer',
'sylius.email_sender.adapter.symfony_mailer',
'sylius.email_sender.adapter.default',
];

$container->setDefinition('sylius.email_sender.adapter.swiftmailer', $swiftmailerAdapter);
$defaultSenderAdapter = 'sylius.email_sender.adapter.swiftmailer';
}
foreach ($services as $service) {
if ($container->hasDefinition($service)) {
$container->setAlias('sylius.email_sender.adapter', $service);

$container->setAlias('sylius.email_sender.adapter', $config['sender_adapter'] ?? $defaultSenderAdapter);
return;
}
}
}

private function configureRendererAdapter(ContainerBuilder $container, array $config): void
private function configureRendererAdapter(array $mergedConfig, ContainerBuilder $container): void
{
$bundles = (array) $container->getParameter('kernel.bundles');
if (!ContainerBuilder::willBeAvailable('twig/twig', Environment::class, ['symfony/twig-bundle'])) {
$container->removeDefinition('sylius.email_renderer.adapter.twig');
}

$defaultRendererAdapter = 'sylius.email_renderer.adapter.default';
if (array_key_exists('TwigBundle', $bundles)) {
$twigAdapter = new ChildDefinition('sylius.email_renderer.adapter.abstract');
$twigAdapter->setClass(EmailTwigAdapter::class);
$twigAdapter->setArguments([new Reference('twig'), new Reference('event_dispatcher')]);
$twigAdapter->setPublic(true);
if (isset($mergedConfig['renderer_adapter'])) {
$container->setAlias('sylius.email_renderer.adapter', $mergedConfig['renderer_adapter']);

$container->setDefinition('sylius.email_renderer.adapter.twig', $twigAdapter);
$defaultRendererAdapter = 'sylius.email_renderer.adapter.twig';
return;
}

$container->setAlias('sylius.email_renderer.adapter', $config['renderer_adapter'] ?? $defaultRendererAdapter);
$services = [
'sylius.email_renderer.adapter.twig',
'sylius.email_renderer.adapter.default',
];

foreach ($services as $service) {
if ($container->hasDefinition($service)) {
$container->setAlias('sylius.email_renderer.adapter', $service);

return;
}
}
}
}
31 changes: 29 additions & 2 deletions src/Bundle/Resources/config/services.xml
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@

<service id="sylius.email_renderer.adapter.abstract" class="Sylius\Component\Mailer\Renderer\Adapter\AbstractAdapter" abstract="true">
<call method="setEventDispatcher">
<argument type="service" id="event_dispatcher" />
<argument type="service" id="event_dispatcher" on-invalid="ignore" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This (and the change immediately below) ends up being a bug fix. The event dispatcher is an optional dependency in the classes that support it, yet compiling the container would fail hard if the dispatcher service isn't there. This ensures that the dispatcher stays optional.

Copy link
Member

@loic425 loic425 May 12, 2022

Choose a reason for hiding this comment

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

isn't on-invalid="null"? is this different?

I don't find any documentation about this value.
https://symfony.com/doc/current/service_container/optional_dependencies.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It’s the “ignoring missing dependencies” section on that page.

The null behavior is valid if you’re working with a nullable argument. The $dispatcher argument on the abstract adapter’s setEventDispatcher() method is not nullable, so the invalid behavior is the right behavior for this case (which means it just won’t be called).

Copy link
Member

Choose a reason for hiding this comment

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

oh yes, I just had not seen it was with a call method, excuxe me.

</call>
</service>
<service
Expand All @@ -49,10 +49,19 @@
parent="sylius.email_renderer.adapter.abstract"
public="true"
/>
<service
id="sylius.email_renderer.adapter.twig"
class="Sylius\Bundle\MailerBundle\Renderer\Adapter\EmailTwigAdapter"
parent="sylius.email_renderer.adapter.abstract"
public="true"
>
<argument type="service" id="twig" />
<argument type="service" id="event_dispatcher" on-invalid="null" />
</service>

<service id="sylius.email_sender.adapter.abstract" class="Sylius\Component\Mailer\Sender\Adapter\AbstractAdapter" abstract="true">
<call method="setEventDispatcher">
<argument type="service" id="event_dispatcher" />
<argument type="service" id="event_dispatcher" on-invalid="ignore" />
</call>
</service>
<service
Expand All @@ -61,5 +70,23 @@
parent="sylius.email_sender.adapter.abstract"
public="true"
/>
<service
id="sylius.email_sender.adapter.swiftmailer"
class="Sylius\Bundle\MailerBundle\Sender\Adapter\SwiftMailerAdapter"
parent="sylius.email_sender.adapter.abstract"
public="true"
>
<deprecated package="sylius/mailer-bundle" version="1.8">The "%service_id%" service is deprecated and will be removed in 2.0, use the Symfony Mailer integration instead.</deprecated>
<argument type="service" id="swiftmailer.mailer.default" />
<argument type="service" id="event_dispatcher" on-invalid="null" />
</service>
<service
id="sylius.email_sender.adapter.symfony_mailer"
class="Sylius\Bundle\MailerBundle\Sender\Adapter\SymfonyMailerAdapter"
parent="sylius.email_sender.adapter.abstract"
public="true"
>
<argument type="service" id="mailer.mailer" />
</service>
</services>
</container>
4 changes: 2 additions & 2 deletions src/Bundle/Sender/Adapter/DefaultAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ public function send(
array $replyTo = []
): void {
throw new \RuntimeException(sprintf(
'You need to configure an adapter to send the email. Take a look at %s (requires "symfony/swiftmailer-bundle" library).',
SwiftMailerAdapter::class
'You need to configure an adapter to send the email. Take a look at %s (requires "symfony/mailer" library).',
SymfonyMailerAdapter::class
));
}
}
9 changes: 9 additions & 0 deletions src/Bundle/Sender/Adapter/SwiftMailerAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
use Sylius\Component\Mailer\SyliusMailerEvents;
use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;

/**
* @deprecated The Swift Mailer integration is deprecated since sylius/mailer-bundle 1.8. Use the Symfony Mailer integration instead.
*/
class SwiftMailerAdapter extends AbstractAdapter
{
/** @var \Swift_Mailer */
Expand All @@ -30,6 +33,12 @@ class SwiftMailerAdapter extends AbstractAdapter

public function __construct(\Swift_Mailer $mailer, ?EventDispatcherInterface $dispatcher = null)
{
trigger_deprecation(
'sylius/mailer-bundle',
'1.8',
'The Swift Mailer integration is deprecated and will be removed in 2.0. Use the Symfony Mailer integration instead.'
);

$this->mailer = $mailer;
$this->dispatcher = $dispatcher;
}
Expand Down
66 changes: 66 additions & 0 deletions src/Bundle/Sender/Adapter/SymfonyMailerAdapter.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
<?php

/*
* This file is part of the Sylius package.
*
* (c) Paweł Jędrzejewski
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace Sylius\Bundle\MailerBundle\Sender\Adapter;

use Sylius\Component\Mailer\Event\EmailSendEvent;
use Sylius\Component\Mailer\Model\EmailInterface;
use Sylius\Component\Mailer\Renderer\RenderedEmail;
use Sylius\Component\Mailer\Sender\Adapter\AbstractAdapter;
use Sylius\Component\Mailer\SyliusMailerEvents;
use Symfony\Component\Mailer\MailerInterface;
use Symfony\Component\Mime\Address;
use Symfony\Component\Mime\Email;

final class SymfonyMailerAdapter extends AbstractAdapter
{
private MailerInterface $mailer;

public function __construct(MailerInterface $mailer)
{
$this->mailer = $mailer;
}

/**
* {@inheritdoc}
*/
public function send(
array $recipients,
string $senderAddress,
string $senderName,
RenderedEmail $renderedEmail,
EmailInterface $email,
array $data,
array $attachments = [],
array $replyTo = []
): void {
$message = (new Email())
->subject($renderedEmail->getSubject())
->from(new Address($senderAddress, $senderName))
->to(...$recipients)
->replyTo(...$replyTo)
->html($renderedEmail->getBody());

foreach ($attachments as $attachment) {
$message->attachFromPath($attachment);
}

$emailSendEvent = new EmailSendEvent($message, $email, $data, $recipients, $replyTo);

$this->dispatcher?->dispatch($emailSendEvent, SyliusMailerEvents::EMAIL_PRE_SEND);

$this->mailer->send($message);

$this->dispatcher?->dispatch($emailSendEvent, SyliusMailerEvents::EMAIL_POST_SEND);
}
}
6 changes: 3 additions & 3 deletions src/Bundle/spec/Sender/Adapter/DefaultAdapterSpec.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
namespace spec\Sylius\Bundle\MailerBundle\Sender\Adapter;

use PhpSpec\ObjectBehavior;
use Sylius\Bundle\MailerBundle\Sender\Adapter\SwiftMailerAdapter;
use Sylius\Bundle\MailerBundle\Sender\Adapter\SymfonyMailerAdapter;
use Sylius\Component\Mailer\Model\EmailInterface;
use Sylius\Component\Mailer\Renderer\RenderedEmail;
use Sylius\Component\Mailer\Sender\Adapter\AbstractAdapter;
Expand All @@ -32,8 +32,8 @@ function it_throws_an_exception_about_not_configured_email_sender_adapter(
): void {
$this
->shouldThrow(new \RuntimeException(sprintf(
'You need to configure an adapter to send the email. Take a look at %s (requires "symfony/swiftmailer-bundle" library).',
SwiftMailerAdapter::class
'You need to configure an adapter to send the email. Take a look at %s (requires "symfony/mailer" library).',
SymfonyMailerAdapter::class
)))
->during('send', [['[email protected]'], '[email protected]', 'arnaud', $renderedEmail, $email, []])
;
Expand Down
Loading