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

silverbackdan · Nov 23, 2022

Q	A
Branch?	6.2
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	Fix #47993
License	MIT
Doc PR	n/a

When running tests in 6.1 I am checking the message context has been set correctly. The email that is collected in the logger is updated after rendering. This line doesn't seem necessary, but if it is, perhaps we should be looking here and to why the message, when cloned still has the context modified in the profiler.
https://github.com/symfony/symfony/blob/6.2/src/Symfony/Component/Mailer/Mailer.php#L54

Thoughts?

//cc @fabpot as this change was a commit by him. Happy to discuss.

Thank you

carsonbot · Nov 23, 2022

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 6.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

carsonbot · Nov 23, 2022

Hey!

Thanks for your PR. You are targeting branch "6.2" but it seems your PR description refers to branch "6.2 for bug fixes".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

silverbackdan · Nov 23, 2022

Oh I see, the cloning of the email and envelope happens first.
Another solution could be here:
https://github.com/symfony/symfony/blob/6.2/src/Symfony/Component/Mailer/EventListener/MessageLoggerListener.php#L40

public function onMessage(MessageEvent $event): void
    {
        $event->setMessage(clone $event->getMessage());
        $this->events->add($event);
    }

Once the correct fix is established I could update the failing tests.

Edit: Hmm.. sorry this fix wouldn't work. Just tried it. Probably have to be what I've tried in the PR.

fabpot · Nov 24, 2022

src/Symfony/Bridge/Twig/Mime/BodyRenderer.php

@@ -67,8 +67,6 @@ public function render(Message $message): void
            $message->htmlTemplate(null);
        }

-        $message->context([]);


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

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

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.

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?

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

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.

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

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.

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?

silverbackdan · Nov 24, 2022

I guess my questions would be:

Is it the responsibility of the renderer to prepare the message appropriately for the possibility of sending via messenger/async?
Could we do this perhaps in Symfony\Component\Mailer\Messenger\SendEmailMessage by checking if it's an instance of TemplatedEmail instead and clreating a clone there and clearing context?
Is it useful/necessary to have the context still for debugging and testing purposes and would this constitute a BC break?

silverbackdan · Nov 24, 2022

Just doing some checks to reproduce again, and having some bugs with Mime\Email::ensureBodyValid for an email which is valid too.
But context is either showing now, or only when I'm testing with messenger transport enabled. Tired head so may be silly mistakes. Leave it with me I'll do some digging on what's going on.

silverbackdan · Nov 24, 2022

Interestingly enough I've adjusted my application's functional tests to use the in-memory transport with serialization enabled and my doctrine entity does serialize. But I'm able to trace through this process now to see where might be best to remove the context before serializing in any case.

carsonbot · Nov 24, 2022

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 6.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

silverbackdan · Nov 24, 2022

How about this as an update/fix?

I have set it to clear the context just before sending the message to the bus if it is a TemplatedEmail and added a test to ensure the context is empty for the message bus.

But this should not affect the message dispatched to the profiler as far as I can see.

silverbackdan · Nov 24, 2022

I still wonder whether the email in the profiler that is not queued would have the context, but I also wonder whether that's a problem or not as usually async and serialization is disabled during tests and for the profiler there would show a queued message with the context still.. I think.

silverbackdan · Nov 25, 2022

I've noticed the new 6.3 branch pop up today.
This would be great to resolve before the official 6.2 release and I'm happy to work today on any further adjustments you may deem necessary to avoid this breaking tests on the 6.2 release
I know release days are pretty intense, so appreciate your time if you're able to give any to resolving this one :)

Thanks!

fabpot · Nov 26, 2022

To be honest, I'm not sure there is anything to fix here. I could argue that testing emails should be done on the rendered email and not on the context passed to it. If you want to test the context, you can add a listener on Symfony\Component\Mailer\Event\MessageEvent that triggers early on (at least before the renderer).

In any case, the only place where to clear the context is just after having rendered the email, anywhere else won't work.

silverbackdan · Nov 26, 2022

I agree that the tests could be done on the result of the context, also in a unit test to ensure the context is being set correctly. As there seems to be no alternative to where to clear the context, and as it's deemed necessary to do so, I'll adjust my tests accordingly. Thanks for your time in considering this in any case.

carsonbot added Status: Needs Review Bug Mailer labels Nov 23, 2022

carsonbot added this to the 6.2 milestone Nov 23, 2022

fabpot reviewed Nov 24, 2022

View reviewed changes

silverbackdan marked this pull request as draft November 24, 2022 19:32

silverbackdan closed this Nov 24, 2022

silverbackdan force-pushed the 6.2 branch from 645c724 to 19940ff Compare November 24, 2022 19:38

Attempt context clearing for message sent to bus

e3a7411

silverbackdan reopened this Nov 24, 2022

silverbackdan marked this pull request as ready for review November 24, 2022 19:51

silverbackdan added 2 commits November 25, 2022 11:08

Harden test to ensure messenger message can be serialized

bd5b6b1

Refactor test

29ad664

silverbackdan changed the base branch from 6.3 to 6.2 November 25, 2022 11:18

silverbackdan added 2 commits November 25, 2022 11:35

Adjust test name for further clarification of test goals

fecc3a3

Add comment on clearing context in code

33d927d

silverbackdan closed this Nov 26, 2022

fabpot mentioned this pull request May 3, 2023

[Mime] Keep context after rendering emails #50233

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

silverbackdan commented Nov 23, 2022 •

edited

Loading

carsonbot commented Nov 23, 2022

carsonbot commented Nov 23, 2022

silverbackdan commented Nov 23, 2022 •

edited

Loading

fabpot Nov 24, 2022

stof Nov 24, 2022

silverbackdan Nov 24, 2022

silverbackdan Nov 24, 2022

stof Nov 24, 2022

silverbackdan Nov 24, 2022 •

edited

Loading

silverbackdan Nov 24, 2022

fabpot Nov 25, 2022

silverbackdan Nov 25, 2022

silverbackdan commented Nov 24, 2022

silverbackdan commented Nov 24, 2022

silverbackdan commented Nov 24, 2022

carsonbot commented Nov 24, 2022

silverbackdan commented Nov 24, 2022

silverbackdan commented Nov 24, 2022

silverbackdan commented Nov 25, 2022

fabpot commented Nov 26, 2022

silverbackdan commented Nov 26, 2022

Search code, repositories, users, issues, pull requests...

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

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

Conversation

silverbackdan commented Nov 23, 2022 • edited Loading

carsonbot commented Nov 23, 2022

carsonbot commented Nov 23, 2022

silverbackdan commented Nov 23, 2022 • edited Loading

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

silverbackdan Nov 24, 2022 • edited Loading

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

silverbackdan commented Nov 24, 2022

silverbackdan commented Nov 24, 2022

silverbackdan commented Nov 24, 2022

carsonbot commented Nov 24, 2022

silverbackdan commented Nov 24, 2022

silverbackdan commented Nov 24, 2022

silverbackdan commented Nov 25, 2022

fabpot commented Nov 26, 2022

silverbackdan commented Nov 26, 2022

silverbackdan commented Nov 23, 2022 •

edited

Loading

silverbackdan commented Nov 23, 2022 •

edited

Loading

silverbackdan Nov 24, 2022 •

edited

Loading