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

mbabker · 2021-12-04T01:53:02Z

Replaces #18

Zales0123

Hello @mbabker, we can finally move forward with this one 🎉 I've left some comments, please, take a look at them. Do you think you would have some time to work on this PR in the nearest future, or should I take it over and finish the work? Thank you for your contribution! 🖖 🏅

src/Bundle/DependencyInjection/SyliusMailerExtension.php

src/Bundle/spec/Sender/Adapter/SymfonyMailerAdapterSpec.php

mbabker · 2022-05-10T01:23:08Z

composer.json

@@ -45,6 +45,8 @@
        "phpunit/phpunit": "^9.4",
        "sylius-labs/coding-standard": "^4.0",
        "symfony/console": "^4.4 || ^5.4",
+        "symfony/event-dispatcher": "^4.4 || ^5.4",


The event dispatcher is used in a few classes, but even for development, it was only installed through transient dependencies.

mbabker · 2022-05-10T01:25:18Z

src/Bundle/Resources/config/services.xml

@@ -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" />


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.

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

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).

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

Zales0123

If you rebase with #128 it would be easier to tell whether the container is building properly or not 🖖 I also believe we should run the lint:container with the symfony/mailer enabled or disabled to see if everything works 💃 Thank you, one more time, for your work @mbabker 🎉

src/Bundle/spec/Sender/Adapter/SymfonyMailerAdapterSpec.php

Zales0123 · 2022-05-11T11:49:43Z

I believe it still has some problems :D Let's take a look at this build (this PR enhanced with additional steps in the build): https://github.com/Zales0123/SyliusMailerBundle/runs/6384899456?check_suite_focus=true

As I said, we need a few different cases covered with the test application:

no adapter configured
swiftmailer adapter configured (with or without symfony/mailer in composer.json)
symfony mailer adapter configured (with or without symfony/swiftmailer-bundle in composer.json)
Maybe even something more :D

The alternative is to require symfony/mailer and symfony/swiftmailer-bundle not in dev but in normal requirements - it would lower the number of test cases. But we definitely need to cover some test cases to be sure it works 🖖

cc @loic425 @lchrusciel I need you opinions :)

mbabker · 2022-05-12T00:00:53Z

The build setup you used won't work. The sylius.email_sender.adapter.swiftmailer and sylius.email_sender.adapter.symfony_mailer services were not created until the compiler pass runs, and only if a service is not explicitly set in the configuration (neither of those services were defined in the services.xml file previously).

To simplify testing, I've reworked things again to use feature detection though ContainerBuilder::willBeAvailable() inside the container extension (yes, it's static; no, wrapping it into something that can be mocked is not practical, see Sylius/SyliusGridBundle#227 (comment) for more on that).

On a side note, trying to do the entire CI in one build is rather limiting. By my count, there should be at least four separate workflows (Psalm and PHPStan as separate jobs in one workflow, code style checks in another workflow, PHPSpec in a 3rd workflow, and PHPUnit in a 4th). This would ensure that the entirety of the CI suite runs on every build instead of half of it not even running because of a code style issue somewhere in the package. You also wouldn't need things like the four separate lint:container calls after running a bunch of Composer commands to get everything in the desired state, you'd do the Composer setup one time and account for the optional things being removed in the test code (at least that's how I'm smoke testing the PagerfantaBundle with Twig fully removed).

Zales0123 · 2022-05-12T11:18:57Z

Thank you for the clarification, I agree 👍 I've checked a few cases with the current state of PR on Sylius (I assumed swiftmailer is always installed via composer, as it's provided by Sylius itself):

Symfony/mailer not installed, no adapter configured - works, swiftmailer used ✅
Symfony/mailer not installed, symfony adapter configured - fails and it should ✅
Symfony/mailer not installed, swiftmailer adapter configured - fails, but it shouldn’t 🚫
Symfony/mailer installed, no adapter configured - works, swiftmailer used ✅
Symfony/mailer installed, symfony adapter configured - works, symfony mailer used ✅
Symfony/mailer installed, swiftmailer adapter configured - works, swiftmailer used ✅

Can you please check out scenario 3? it can happen when one has a Sylius application with swiftmailer adapter configured (even though it's the default one), updates the mailer bundle and sees the exception (as there is a service created that has a dependency to Symfony Mailer probably).

cc @loic425 @lchrusciel I still need your eye on it as well 😄

Regarding the test suite - I totally agree, that we should refactor it to multiple builds 👍 The proposed suite was just for now 🖖

loic425 · 2022-05-12T12:14:03Z

.github/workflows/build.yml

+                    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)


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

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).

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

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.

.github/workflows/build.yml

…the Swiftmailer integration Co-authored-by: Michael Babker <[email protected]>

mbabker · 2022-05-13T00:28:58Z

I've taken the feature detection around the services as far as I can sanely take it.

Zales0123

I've also checked all scenarios described here with this implementation and it seems to work like a charm ✨ 🎉

Zales0123 · 2022-06-09T13:03:47Z

src/Bundle/spec/Sender/Adapter/SymfonyMailerAdapterSpec.php

+            ->shouldBeCalled()
+        ;
+
+        $mailer->send(Argument::type(Email::class))->shouldBeCalled();


We could work a little bit at this prophecy to check the Email content as well 🖖

Zales0123 · 2022-06-09T13:14:17Z

What an adventure! 🎉 Thank you, @mbabker, for this amazing work 🥇

mbabker force-pushed the feature/symfony-mailer branch 3 times, most recently from d99600f to 628ca55 Compare December 4, 2021 02:15

Roshyo approved these changes Dec 4, 2021

View reviewed changes

vvasiloi mentioned this pull request Jan 10, 2022

Add handling CC and BCC to Sender #11

Closed

Zales0123 added the Feature New feature proposals. label Feb 4, 2022

Zales0123 mentioned this pull request Feb 4, 2022

Add symfony mailer adapter #18

Closed

stloyd mentioned this pull request Feb 10, 2022

Support for newest Symfony mailer #5

Closed

Zales0123 mentioned this pull request Feb 25, 2022

Fix sending emails when storing invoices in S3 Sylius/InvoicingPlugin#242

Open

loic425 mentioned this pull request Apr 20, 2022

Symfony 6 support Sylius/Sylius#13274

Closed

Zales0123 reviewed May 9, 2022

View reviewed changes

src/Bundle/DependencyInjection/SyliusMailerExtension.php Outdated Show resolved Hide resolved

src/Bundle/spec/Sender/Adapter/SymfonyMailerAdapterSpec.php Outdated Show resolved Hide resolved

src/Bundle/spec/Sender/Adapter/SymfonyMailerAdapterSpec.php Show resolved Hide resolved

mbabker force-pushed the feature/symfony-mailer branch from 0d6a673 to 4dd60bb Compare May 10, 2022 00:11

mbabker requested a review from a team as a code owner May 10, 2022 00:11

mbabker force-pushed the feature/symfony-mailer branch from 4dd60bb to c18092a Compare May 10, 2022 01:22

mbabker commented May 10, 2022

View reviewed changes

mbabker force-pushed the feature/symfony-mailer branch 3 times, most recently from 6b7a3cb to 22a6c57 Compare May 10, 2022 01:34

Zales0123 reviewed May 10, 2022

View reviewed changes

src/Bundle/spec/Sender/Adapter/SymfonyMailerAdapterSpec.php Outdated Show resolved Hide resolved

mbabker force-pushed the feature/symfony-mailer branch 2 times, most recently from 0594f68 to ad5eaf1 Compare May 10, 2022 23:00

mbabker force-pushed the feature/symfony-mailer branch 7 times, most recently from d2c5925 to 26d2ccc Compare May 11, 2022 23:55

mbabker force-pushed the feature/symfony-mailer branch from 26d2ccc to df91db8 Compare May 11, 2022 23:59

loic425 reviewed May 12, 2022

View reviewed changes

.github/workflows/build.yml Show resolved Hide resolved

.github/workflows/build.yml Show resolved Hide resolved

loic425 reviewed May 12, 2022

View reviewed changes

.github/workflows/build.yml Show resolved Hide resolved

Add an adapter to support the Symfony Mailer component and deprecate …

e9862b2

…the Swiftmailer integration Co-authored-by: Michael Babker <[email protected]>

mbabker force-pushed the feature/symfony-mailer branch from df91db8 to e9862b2 Compare May 13, 2022 00:28

Zales0123 approved these changes Jun 9, 2022

View reviewed changes

Zales0123 merged commit bfeab0b into Sylius:master Jun 9, 2022

mbabker deleted the feature/symfony-mailer branch June 9, 2022 13:24

mbabker mentioned this pull request Nov 21, 2022

[Maintenance] Fix removing Swiftmailer adapter #181

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mbabker commented Dec 4, 2021

Zales0123 left a comment

mbabker May 10, 2022

mbabker May 10, 2022

loic425 May 12, 2022 •

edited

Loading

mbabker May 12, 2022

loic425 May 12, 2022

Zales0123 left a comment

Zales0123 commented May 11, 2022 •

edited

Loading

mbabker commented May 12, 2022

Zales0123 commented May 12, 2022

loic425 May 12, 2022

mbabker May 12, 2022

loic425 May 12, 2022

mbabker May 12, 2022

mbabker commented May 13, 2022

Zales0123 left a comment •

edited

Loading

Zales0123 Jun 9, 2022

Zales0123 commented Jun 9, 2022

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

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

Conversation

mbabker commented Dec 4, 2021

Zales0123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loic425 May 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zales0123 left a comment

Choose a reason for hiding this comment

Zales0123 commented May 11, 2022 • edited Loading

mbabker commented May 12, 2022

Zales0123 commented May 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbabker commented May 13, 2022

Zales0123 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zales0123 commented Jun 9, 2022

loic425 May 12, 2022 •

edited

Loading

Zales0123 commented May 11, 2022 •

edited

Loading

Zales0123 left a comment •

edited

Loading