Add configurable mailer #1052

Wharenn · 2018-10-23T11:43:30Z

I am targeting this branch, because this is a new feature with no breaking change.

Changelog

### Added
- Added an optional `mailer` config option to use a custom mailer implementation

Subject

By default, SonataUserBundle is using the mailer service to send the account password reset email and keep the email building logic in a controller private method. It makes it hard to use a custom implementation like using another mailer than the default one or updating the email creation process.

This PR adds a Mailer class implementing the FOS MailerInterface and doing the same things the AdminResettingController class was. This default class can be replaced by any other user service implementing the MailerInterface by setting corresponding service id uner the mailer config key:

# config/package/sonata.yaml

sonata_user:
    mailer: custom.mailer.service.id

OskarStark · 2018-10-23T12:23:06Z

src/Mailer/Mailer.php

+    public function __construct(RouterInterface $router, EngineInterface $templating, \Swift_Mailer $mailer, string $fromEmail, string $emailTemplate)
+    {
+        $this->router = $router;
+        $this->templating = $templating;


Please use the same order for the const arguments and the member vars

I think I got it. It should be fixed now?

OskarStark · 2018-10-23T12:24:01Z

tests/Mailer/MailerTest.php

+    public function setUp(): void
+    {
+        $this->router = $this->createMock(RouterInterface::class);
+        $this->templating = $this->createMock(EngineInterface::class);


Here the order is correct 🤗👍🏻

src/Controller/AdminResettingController.php

OskarStark

Blocked by #1053

SonataCI · 2018-10-30T12:50:56Z

Could you please rebase your PR and fix merge conflicts?

Wharenn · 2018-11-06T12:17:20Z

This PR has been updated to reflect changes introduced by #1053. We are now using an alias in the DI to point to the configurable mailer.

greg0ire · 2018-11-06T19:02:01Z

docs/reference/installation.rst

+Mailer Configuration
+~~~~~~~~~~~~~~~~~~~~
+
+You can define a custom mailer to send reset password emails. Your mailer will have to implement ``FOS\UserBundle\Mailer\MailerInterface``.


Please comply with the sf docs standards:

Each line should break approximately after the first word that crosses the 72nd character (so most lines end up being 72-78 characters);

greg0ire · 2018-11-06T19:02:48Z

src/DependencyInjection/Configuration.php

@@ -115,6 +116,7 @@ public function getConfigTreeBuilder()
                        ->scalarNode('default_avatar')->defaultValue('bundles/sonatauser/default_avatar.png')->end()
                    ->end()
                ->end()
+                ->scalarNode('mailer')->defaultValue(Mailer::class)->end()


Please add a call to info documenting what this is

greg0ire · 2018-11-06T19:03:47Z

src/Mailer/Mailer.php

+use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
+use Symfony\Component\Routing\RouterInterface;
+
+class Mailer implements MailerInterface


greg0ire · 2018-11-06T19:04:00Z

src/Mailer/Mailer.php

+    /**
+     * @var RouterInterface
+     */
+    protected $router;


Please use private as much as possible

greg0ire · 2018-11-06T19:04:47Z

src/Mailer/Mailer.php

+class Mailer implements MailerInterface
+{
+    /**
+     * @var RouterInterface


Please refactor this to UrlGeneratorInterface, since you are not matching any urls.

src/Resources/config/mailer.xml

greg0ire · 2018-11-06T19:20:35Z

tests/DependencyInjection/SonataUserExtensionTest.php

@@ -226,6 +226,28 @@ public function testConfigureGoogleAuthenticatorEnabled(): void
        $this->assertContainerBuilderHasServiceDefinitionWithArgument('sonata.user.google.authenticator.provider', 0, 'bar');
    }

+    /**
+     * @group legacy
+     * @expectedDeprecation The 'Google\Authenticator' namespace is deprecated in sonata-project/GoogleAuthenticator since version 2.1 and will be removed in 3.0.


It is weird that you need to do this 😅

Yes, it also seems weird to me, but this deprecation is risen everytime the load method is used :/

greg0ire · 2018-11-06T19:20:59Z

tests/Mailer/MailerTest.php

+    /**
+     * @var RouterInterface|\PHPUnit_Framework_MockObject_MockObject
+     */
+    protected $router;


private by default please http://fabien.potencier.org/pragmatism-over-theory-protected-vs-private.html

src/DependencyInjection/SonataUserExtension.php

Hanmac · 2018-11-12T08:29:10Z

I don't know how much this is already uses FOS stuff, but would it make sense to use the FOSUserEvents for password reset too?

and should that be part of your actions or your new mailer?

Wharenn · 2018-11-12T09:09:00Z

@Hanmac we discussed this in #1053 and agreed to use FOS events :) We decided to postpone the implementation to another PR to be able to close the mailer subject before. I already have some wip on the FOS event implementation but I'm waiting for the completion of the mailer subject before pushing it as a PR.

Hanmac · 2018-11-12T09:11:05Z

@Wharenn oh we already did? then it is okay.

i was just wondering where the events should be, in the mailer or in the actions?

Wharenn · 2018-11-12T09:14:41Z

IMO in the actions as they can also be used to hook userland behaviors on every action workflow. This is the way they are implemented in FOSUserBundle. I think the incoming PR will go deeper about the subject.

Wharenn · 2018-11-14T09:06:35Z

Hi @sonata-project/contributors can we move forward on this PR?

kunicmarko20 · 2018-11-14T09:32:09Z

src/DependencyInjection/SonataUserExtension.php

    public function configureTranslationDomain($config, ContainerBuilder $container): void
    {
        $container->setParameter('sonata.user.admin.user.translation_domain', $config['admin']['user']['translation']);
        $container->setParameter('sonata.user.admin.group.translation_domain', $config['admin']['group']['translation']);
    }

-    /**
-     * @param array            $config


you removed this but didn't add the typehint in the signature

kunicmarko20

looks good

core23 · 2018-11-14T11:41:24Z

src/DependencyInjection/SonataUserExtension.php

     */
-    public function fixImpersonating(array $config)
+    public function fixImpersonating(array $config): array


core23 · 2018-11-14T11:41:30Z

src/DependencyInjection/SonataUserExtension.php

-     * @return mixed
-     */
-    public function configureGoogleAuthenticator($config, ContainerBuilder $container)
+    public function configureGoogleAuthenticator(array $config, ContainerBuilder $container): void


core23 · 2018-11-14T11:41:38Z

src/DependencyInjection/SonataUserExtension.php

-     * @param ContainerBuilder $container
-     */
-    public function configureClass($config, ContainerBuilder $container): void
+    public function configureClass(array $config, ContainerBuilder $container): void


core23 · 2018-11-14T11:41:52Z

src/DependencyInjection/SonataUserExtension.php

-     * @param ContainerBuilder $container
-     */
-    public function configureAdminClass($config, ContainerBuilder $container): void
+    public function configureAdminClass(array $config, ContainerBuilder $container): void


core23 · 2018-11-14T11:41:59Z

src/DependencyInjection/SonataUserExtension.php

-     * @param ContainerBuilder $container
-     */
-    public function configureTranslationDomain($config, ContainerBuilder $container): void
+    public function configureTranslationDomain(array $config, ContainerBuilder $container): void


core23 · 2018-11-14T11:42:16Z

src/DependencyInjection/SonataUserExtension.php

-     * @param ContainerBuilder $container
-     */
-    public function configureController($config, ContainerBuilder $container): void
+    public function configureController(array $config, ContainerBuilder $container): void


core23 · 2018-11-14T11:42:33Z

src/DependencyInjection/SonataUserExtension.php

-    /**
-     * @param array $config
-     */
+    public function configureMailer(array $config, ContainerBuilder $container): void


Can you make this private

core23 · 2018-11-14T11:42:50Z

src/DependencyInjection/SonataUserExtension.php

     */
-    protected function aliasManagers(ContainerBuilder $container, $managerType): void
+    protected function aliasManagers(ContainerBuilder $container, string $managerType): void


Wharenn · 2018-11-14T12:18:36Z

@core23 fixed BCs

core23 · 2018-11-14T13:41:18Z

src/DependencyInjection/SonataUserExtension.php

+
+    private function configureMailer(array $config, ContainerBuilder $container): void
+    {
+        $container->setAlias('sonata.user.mailer', $config['mailer']);


Bonuspoint: Make an alias for the MailerInterface too

We are implementing the FOS MailerInterface, this would lead to expose to autowiring the FOS MailerInterface from this bundle. Is this really what we want?

Oh, you're right. Just ignore it 😅

greg0ire · 2018-11-14T15:22:00Z

Thanks @Wharenn !

OskarStark requested changes Oct 23, 2018

View reviewed changes

kunicmarko20 reviewed Oct 23, 2018

View reviewed changes

src/Controller/AdminResettingController.php Outdated Show resolved Hide resolved

Wharenn mentioned this pull request Oct 24, 2018

Use invokable actions #1053

Merged

2 tasks

OskarStark requested changes Oct 29, 2018

View reviewed changes

SonataCI added pending author and removed pending author labels Oct 30, 2018

greg0ire requested changes Nov 6, 2018

View reviewed changes

greg0ire previously approved these changes Nov 7, 2018

View reviewed changes

core23 reviewed Nov 7, 2018

View reviewed changes

src/DependencyInjection/SonataUserExtension.php Outdated Show resolved Hide resolved

OskarStark previously approved these changes Nov 8, 2018

View reviewed changes

core23 previously approved these changes Nov 14, 2018

View reviewed changes

kunicmarko20 reviewed Nov 14, 2018

View reviewed changes

kunicmarko20 previously approved these changes Nov 14, 2018

View reviewed changes

Wharenn dismissed stale reviews from kunicmarko20, core23, OskarStark, and greg0ire via 4a822bd November 14, 2018 09:40

kunicmarko20 previously approved these changes Nov 14, 2018

View reviewed changes

core23 requested changes Nov 14, 2018

View reviewed changes

Wharenn dismissed kunicmarko20’s stale review via 28ae451 November 14, 2018 12:14

Add configurable mailer

7a18002

core23 reviewed Nov 14, 2018

View reviewed changes

core23 approved these changes Nov 14, 2018

View reviewed changes

greg0ire merged commit 55a4450 into sonata-project:4.x Nov 14, 2018

Add configurable mailer #1052

Add configurable mailer #1052

Conversation

Wharenn commented Oct 23, 2018

Changelog

Subject

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OskarStark left a comment

Choose a reason for hiding this comment

SonataCI commented Oct 30, 2018

Wharenn commented Nov 6, 2018 • edited

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

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

Hanmac commented Nov 12, 2018

Wharenn commented Nov 12, 2018 • edited

Hanmac commented Nov 12, 2018

Wharenn commented Nov 12, 2018

Wharenn commented Nov 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunicmarko20 left a comment

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

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

Choose a reason for hiding this comment

Wharenn commented Nov 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Nov 14, 2018

Wharenn commented Nov 6, 2018 •

edited

Wharenn commented Nov 12, 2018 •

edited