[Mime] use isRendered method to ensure we can avoid rendering an email twice
#59596
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
f83820d to
7ed10bd
Compare
288c4c0 to
477343b
Compare
|
Would there be a way to test this? |
|
@nicolas-grekas I need to take a closer look to it. |
isRendered method to ensure we can avoid rendering an email twice
c6c75a6 to
234c657
Compare
|
Hey 👋 I reviewed the existing tests and found that the Since the test below is very similar to public function testEmailIsNotRenderedTwice()
{
$twig = new Environment(new ArrayLoader([
'text' => 'Initial Text',
'html' => '<b>Initial HTML</b>',
]));
$renderer = new BodyRenderer($twig);
$email = new TemplatedEmail();
$email->textTemplate('text');
$email->htmlTemplate('html');
// First render
$renderer->render($email);
$this->assertTrue($email->isRendered());
$this->assertEquals('Initial Text', $email->getTextBody());
$this->assertEquals('<b>Initial HTML</b>', $email->getHtmlBody());
// Manually setting the text and HTML content to simulate a previous render
$email->text('Manually Set Text');
$email->html('Manually Set HTML');
// Second render should not override manually set values
$renderer->render($email);
$this->assertEquals('Manually Set Text', $email->getTextBody());
$this->assertEquals('Manually Set HTML', $email->getHtmlBody());
} |
234c657 to
2d37c79
Compare
|
Thank you @walva. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
After upgrading my project from Symfony 5.4 to 6.4, due this change, I encountered an issue where asynchronous email sending failed due to serialization problems with
TemplatedEmail. According to the Symfony documentation, when sending an email asynchronously, the email instance must be serializable. While Mailer instances are inherently serializable,TemplatedEmailrequires that its context be serializable. If the context contains non-serializable variables, such as Doctrine entities, it's recommended to either replace them with serializable variables or render the email before calling$mailer->send($email).To address this, I decorated the Symfony
Mailerto force the rendering of emails using theBodyRendererInterface::render(Message $message): voidmethod. However, this approach led to an exception indicating that the context was empty. The method successfully rendered the email, clearing its context, but the sameTemplatedEmailobject was rendered again later in the execution process, specifically in the MessageListener.To prevent this double rendering, I utilized the
TemplatedEmail::isRendered(): boolmethod, as seen in the AbstractTransport. This ensures that if an email is already rendered, it won't be rendered again, preserving the context and preventing serialization issues.Solution
The proposed fix involves checking if the
TemplatedEmailhas already been rendered before attempting to render it again. By using theisRendered()method, we can avoid unnecessary re-rendering, which can lead to empty contexts and serialization problems during asynchronous email sending.Impact
This correction allows classes extending
TemplatedEmailto be sent asynchronously without encountering errors related to double rendering or context loss due to serialization issues, as outlined in the documentation.References
Symfony Mailer Documentation
MessageListener Implementation
AbstractTransport Implementation
This pull request aims to enhance the robustness of asynchronous email handling in Symfony by ensuring that TemplatedEmail instances are not rendered multiple times, thereby maintaining their context and serializability.
Original message:
Following up my comment on https://github.com/symfony/symfony/pull/47075/files.
I would recommend using the method TemplatedEmail::isRendered() the same way it is used in AbstractTransport.php.
Keeping like this prevents subclass of TemplatedEmail with computed html template to not be rendered twice, such as NotificationEmail.
This change will allow developers to create a subclass of template emails and render them before sending them async, without rendering them twice.