Skip to content

[Mailer] Fix: do not clear context of message after rendering #48300

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.

Sign up for GitHub

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

Closed
silverbackdan wants to merge 5 commits into from
Closed

[Mailer] Fix: do not clear context of message after rendering #48300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e3a7411
Attempt context clearing for message sent to bus
silverbackdan Nov 24, 2022
bd5b6b1
Harden test to ensure messenger message can be serialized
silverbackdan Nov 25, 2022
29ad664
Refactor test
silverbackdan Nov 25, 2022
fecc3a3
Adjust test name for further clarification of test goals
silverbackdan Nov 25, 2022
33d927d
Add comment on clearing context in code
silverbackdan Nov 25, 2022
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
2 changes: 0 additions & 2 deletions src/Symfony/Bridge/Twig/Mime/BodyRenderer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@ public function render(Message $message): void
$message->htmlTemplate(null);
}

$message->context([]);
Copy link
Member

@fabpot fabpot Nov 24, 2022

Choose a reason for hiding this comment

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

That's useful, see #47075 (comment) for the explanation of why we need to empty the context.

Copy link
Member

@stof stof Nov 24, 2022

Choose a reason for hiding this comment

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

We should probably have a test covering that use case, to avoid regressions on that use case

Copy link
Author

@silverbackdan silverbackdan Nov 24, 2022

Choose a reason for hiding this comment

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

Interesting, thanks for clarifying, I hadn't seen that.
I wonder whether by doing this we end up removing the context from the debug profiler as well though. I wonder if there is a way to keep this information in the logs/profiler.

Copy link
Author

@silverbackdan silverbackdan Nov 24, 2022

Choose a reason for hiding this comment

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

Would it perhaps be preferable to clear the context at the time of serializing for the messenger dispatch, perhaps by cloning the message and clearing?

Copy link
Member

@stof stof Nov 24, 2022

Choose a reason for hiding this comment

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

Omitting the context when serializing a TemplatedEmail that is already rendered might indeed be an option.

Copy link
Author

@silverbackdan silverbackdan Nov 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

OK cool, as I'm working in test environment for behat functional testing in my project, I don't have a test which triggers the PhpSerializer to serialize the envelope. I'll try and work out how to make a failing test in this PR when the context is not cleared first and then where (that would make sense) to hook in the logic to check for a TemplatedEmail before the serializer is called.

Copy link
Author

@silverbackdan silverbackdan Nov 24, 2022

Choose a reason for hiding this comment

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

I've updated the PR request to clear context before sending to the bus instead as an alternative option.

Copy link
Member

@fabpot fabpot Nov 25, 2022

Choose a reason for hiding this comment

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

Your change would break valid use cases. There are 2 supported use cases: rendering emails before sending them to the bus and rendering them when consumed from the queue. So, we can only clear the context when the email is rendered, which is why the current code is here.

Copy link
Author

@silverbackdan silverbackdan Nov 25, 2022

Choose a reason for hiding this comment

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

Is there another alternative? This code change would break tests so would the advice to anyone upgrade from 6.1 to 6.2 be to adjust their tests and perhaps am I wrong in thinking that by clearing the context the profiler would not longer be able to output the context set in the email too?
Would the change in RC1 as it stands not constitute a breaking change?


// if text body is empty, compute one from the HTML body
if (!$message->getTextBody() && null !== $html = $message->getHtmlBody()) {
$text = $this->converter->convert(\is_resource($html) ? stream_get_contents($html) : $html, $message->getHtmlCharset());
Expand Down
6 changes: 6 additions & 0 deletions src/Symfony/Component/Mailer/Mailer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\Mailer;

use Psr\EventDispatcher\EventDispatcherInterface;
use Symfony\Bridge\Twig\Mime\TemplatedEmail;
use Symfony\Component\Mailer\Event\MessageEvent;
use Symfony\Component\Mailer\Exception\TransportExceptionInterface;
use Symfony\Component\Mailer\Messenger\SendEmailMessage;
Expand Down Expand Up @@ -59,6 +60,11 @@ public function send(RawMessage $message, Envelope $envelope = null): void
}

try {
// Many users will have issues if they include unserializable items in their context of a TemplatedEmail
// Therefore to improve usability we will clear the context of this email which will have already been rendered
if ($message instanceof TemplatedEmail) {
$message->context([]);
}
$this->bus->dispatch(new SendEmailMessage($message, $envelope), $stamps);
} catch (HandlerFailedException $e) {
foreach ($e->getNestedExceptions() as $nested) {
Expand Down
57 changes: 51 additions & 6 deletions src/Symfony/Component/Mailer/Tests/MailerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@

use PHPUnit\Framework\TestCase;
use Psr\EventDispatcher\EventDispatcherInterface;
use Symfony\Bridge\Twig\Mime\TemplatedEmail;
use Symfony\Component\HttpFoundation\File\File;
use Symfony\Component\Mailer\Event\MessageEvent;
use Symfony\Component\Mailer\Exception\LogicException;
use Symfony\Component\Mailer\Mailer;
Expand All @@ -21,6 +23,7 @@
use Symfony\Component\Messenger\Envelope;
use Symfony\Component\Messenger\MessageBusInterface;
use Symfony\Component\Messenger\Stamp\StampInterface;
use Symfony\Component\Messenger\Transport\Serialization\PhpSerializer;
use Symfony\Component\Mime\Email;
use Symfony\Component\Mime\RawMessage;

Expand All @@ -34,23 +37,26 @@ public function testSendingRawMessages()
$transport->send(new RawMessage('Some raw email message'));
}

public function testSendMessageToBus()
private static function createDumyMailerBus()
{
$bus = new class() implements MessageBusInterface {
return new class() implements MessageBusInterface {
public $messages = [];
public $envelopes = [];
public $stamps = [];

public function dispatch($message, array $stamps = []): Envelope
{
$this->messages[] = $message;
$this->stamps = $stamps;

return new Envelope($message, $stamps);
$envelope = new Envelope($message, $stamps);
$this->envelopes[] = $envelope;
return $envelope;
}
};
}

$stamp = $this->createMock(StampInterface::class);

private function createMockDispatcher(StampInterface $stamp)
{
$dispatcher = $this->createMock(EventDispatcherInterface::class);
$dispatcher->expects($this->once())
->method('dispatch')
Expand All @@ -61,6 +67,14 @@ public function dispatch($message, array $stamps = []): Envelope
}))
->willReturnArgument(0)
;
return $dispatcher;
}

public function testSendMessageToBus()
{
$bus = self::createDumyMailerBus();
$stamp = $this->createMock(StampInterface::class);
$dispatcher = $this->createMockDispatcher($stamp);

$mailer = new Mailer(new NullTransport($dispatcher), $bus, $dispatcher);

Expand All @@ -78,4 +92,35 @@ public function dispatch($message, array $stamps = []): Envelope
self::assertCount(1, $bus->stamps);
self::assertSame([$stamp], $bus->stamps);
}

public function testSendTemplatedEmailWithUnserializableContextToBus()
{
$bus = self::createDumyMailerBus();
$stamp = $this->createMock(StampInterface::class);
$dispatcher = $this->createMockDispatcher($stamp);

$mailer = new Mailer(new NullTransport($dispatcher), $bus, $dispatcher);

$email = (new TemplatedEmail())
->from('hello@example.com')
->to('you@example.com')
->subject('Time for Symfony Mailer!')
->text('Sending emails is fun again!')
->context([
'unserializable' => new File('/anything.jpg', false)
]);

$mailer->send($email);

$serializer = new PhpSerializer();

self::assertCount(1, $bus->messages);
self::assertCount(1, $bus->envelopes);

$templatedEmail = $bus->messages[0]->getMessage();
self::assertInstanceOf(TemplatedEmail::class, $templatedEmail);
self::assertSame($email, $templatedEmail);
self::assertArrayHasKey('body', $serializer->encode($bus->envelopes[0]));
self::assertSame([], $templatedEmail->getContext());
}
}