Dispatch Event after email sent + split Sender and Renderer #2482
Conversation
amenophis
force-pushed
the
mailer-event
branch
3 times, most recently
from
ad34588
to
ad62222
Compare
| */ | ||
| public function __construct(DefaultSettingsProviderInterface $defaultSettingsProvider) | ||
| public function setDefaultSettingsProvider(DefaultSettingsProviderInterface $defaultSettingsProvider) |
There was a problem hiding this comment.
Why changing it to setter injection?
There was a problem hiding this comment.
It is the recommended way for abstract service.
There was a problem hiding this comment.
|
@pjedrzejewski got an idea about decoupling render and sender. will try to add it tonight. |
|
@pjedrzejewski also see that renderBlock could not be called as it is not in the interface. what do you think ? |
amenophis
force-pushed
the
mailer-event
branch
from
c19e53b
to
70072fe
Compare
|
@pjedrzejewski I introduce Renderer and Sender separation. What do you think about it ? |
| /** | ||
| * @author Jérémy Leherpeur <jeremy@leherpeur.net> | ||
| */ | ||
| abstract class AbstractAdapter |
There was a problem hiding this comment.
This class is totally useless...
There was a problem hiding this comment.
Yes for the moment. Maybe i will inject the event_dispatcher.
amenophis
force-pushed
the
mailer-event
branch
4 times, most recently
from
d6e6052
to
2659182
Compare
amenophis
changed the title
|
Sounds cool, I will take a look at the code soonish. |
|
OK nice ! wait for your review ! |
| /** | ||
| * @author Jérémy Leherpeur <jeremy@leherpeur.net> | ||
| */ | ||
| class EmailRenderedEvent extends Event |
There was a problem hiding this comment.
Why having two event classes that holds almost same data instead of one, and some optional parameter while using "recipients"?
There was a problem hiding this comment.
Done ! Just merge the two in one Event class
|
You should fix CS & rebase code. |
amenophis
force-pushed
the
mailer-event
branch
3 times, most recently
from
27f6561
to
6fb3d09
Compare
| <service id="sylius.email_sender.adapter.twig_swiftmailer" class="%sylius.email_sender.adapter.twig_swiftmailer.class%"> | ||
| <argument type="service" id="sylius.mailer.default_settings_provider" /> | ||
| <argument type="service" id="mailer" /> | ||
| <service id="sylius.email_renderer.adapter.abstract" class="%sylius.email_renderer.adapter.abstract.class%" abstract="true"> |
There was a problem hiding this comment.
I'm not sure that all those new services should be public as they will be only used internally by Sender class...
There was a problem hiding this comment.
It can be used by another bundle to create a new adapter
|
Fixed, Rebased and squashed. |
|
@pjedrzejewski ping ;) |
|
@stloyd thanks for pinging :) |
|
@amenophis I will try to review it today, sorry! |
| /** @var EmailEvent $event */ | ||
| $event = $this->dispatcher->dispatch(self::EVENT_EMAIL_RENDERED, new EmailEvent($subject, $body)); | ||
|
|
||
| return array( |
There was a problem hiding this comment.
I believe we should introduce an object here, not array, like "RenderedEmail" class.
|
Looks cool, please apply my comments if you agree and I will be happy to merge it. :) Please remember to add event dispatcher to the deps! |
|
OK, will add it ASAP. |
|
@pjedrzejewski done. OK ? |
|
@stloyd @pjedrzejewski I'm not really expert with phpSpec, could you help with the fix for Specs ? |
|
|
||
| $adapter->send($email, array('jonh@example.com'), array('foo' => 2))->shouldBeCalled(); | ||
| $rendererAdapter->render($email, array('foo' => 2))->shouldBeCalled(); | ||
| $senderAdapter->send(array('jonh@example.com'), null, null, null)->shouldBeCalled(); | ||
|
|
||
| $this->send('bar', array('jonh@example.com'), array('foo' => 2)); | ||
| } |
There was a problem hiding this comment.
This should be:
function it_sends_an_email_through_the_adapter(
$rendererAdapter,
$senderAdapter,
$provider,
EmailInterface $email,
RenderedEmail $renderedEmail
) {
$provider->getEmail('bar')->shouldBeCalled()->willReturn($email);
$email->isEnabled()->shouldBeCalled()->willReturn(true);
$email->getSenderAddress()->shouldBeCalled();
$email->getSenderName()->shouldBeCalled();
$rendererAdapter->render($email, array('foo' => 2))->willReturn($renderedEmail);
$senderAdapter->send(array('jonh@example.com'), null, null, $renderedEmail)->shouldBeCalled();
$this->send('bar', array('jonh@example.com'), array('foo' => 2));
}
amenophis
force-pushed
the
mailer-event
branch
2 times, most recently
from
c6efaed
to
f6aba81
Compare
Introduce Email Renderer
|
@stloyd done @pjedrzejewski Time to merge ? |
|
@pjedrzejewski Is there anything blocking this? Review was done few times already... |
Dispatch Event after email sent + split Sender and Renderer
|
Thank you Jeremy! 👍 Great work. |
As disscussed here: #2479
This is a first implementation.
@pjedrzejewski can you feedback ?