Skip to content

[TwigBridge] Render email once #39733

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

Jump to bottom
Merged
merged 1 commit into from
Mar 5, 2021
Merged

[TwigBridge] Render email once #39733

merged 1 commit into from
Mar 5, 2021

Conversation

jderusse
Copy link
Member

@jderusse jderusse commented Jan 5, 2021 •
edited
Loading

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #39718
License MIT
Doc PR -

When \Symfony\Component\Mailer\Mailer send an email via the Bus (async) it dispatches an MessageEvent, then the consumer call the \Symfony\Component\Mailer\Transport\AbstractTransport::send method which also dispatches an MessageEvent.

This event is listened by \Symfony\Bridge\Twig\Mime\BodyRenderer::render which rendered twice an email.

I'm not sure why the event is send twice, and if we could safely remove one of them (or maybe deprecating the MessageEvent, in favor of SendMessageEvent + AsyncMessageEvent)

This PR store a flag in the Message to avoid rendering it twice.

@jderusse jderusse force-pushed the twig-render-email-once branch from 3917970 to b0f4188 Compare January 5, 2021 22:30
@Nyholm
Copy link
Member

Nyholm commented Jan 6, 2021

@carsonbot do you know who could review this?

@Nyholm
Copy link
Member

Nyholm commented Jan 6, 2021

@carsonbot please find me a reviewer

@carsonbot
Copy link

@pupaxxo could maybe review this PR?

Nyholm
Nyholm approved these changes Jan 6, 2021
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you

@nicolas-grekas nicolas-grekas added this to the 5.x milestone Jan 7, 2021
@carsonbot carsonbot changed the title Render email once [TwigBridge] Render email once Jan 8, 2021
@ajgarlag
Copy link
Contributor

ajgarlag commented Feb 5, 2021

I think that this PR is not a feature, but a bug fix and that it should be merged into the 4.4 branch.

My problem is that when my app sends a templated email to the async transport, it is rerenderd in an HTTP context and stored with the URLs pointing to the correct hostname, but when the message is consumed from the console message:consume command it is rendered again in a CLI context, and the URL finally points to localhost.

I know that it can be fixed by setting the router.request_context.host parameter, but it is not so easy in a multitenant environment, and it's annoying to see that the email body in the profiler differs from the email body really sent to my address.

@jderusse
Copy link
Member Author

jderusse commented Feb 5, 2021

I know that it can be fixed by setting the router.request_context.host parameter, but it is not so easy in a multitenant environment, and it's annoying to see that the email body in the profiler differs from the email body really sent to my address.

This issue is addressed by another of my PR 😛 #39688

@ajgarlag
Copy link
Contributor

ajgarlag commented Feb 8, 2021

This issue is addressed by another of my PR stuck_out_tongue #39688

That is a great new feature.

But IMO this PR is a bug fix and shoud be merged into v4.4 to fix #39718.

@jderusse jderusse changed the base branch from 5.x to 4.4 February 8, 2021 22:22
@jderusse jderusse force-pushed the twig-render-email-once branch from b0f4188 to 186ea59 Compare February 8, 2021 22:22
@jderusse jderusse modified the milestones: 5.x, 4.4 Feb 8, 2021
@jderusse
Copy link
Member Author

jderusse commented Feb 8, 2021

rebased on 4.4

@ajgarlag
Copy link
Contributor

ajgarlag commented Mar 5, 2021

I though this PR would be merged before releasing version 4.4.20. Can I help to get it merged soon?

Thanks

@derrabus
Copy link
Member

derrabus commented Mar 5, 2021

Thank you Jérémy.

@derrabus derrabus merged commit 13055b6 into symfony:4.4 Mar 5, 2021
@fabpot fabpot mentioned this pull request Mar 10, 2021
Merged
@PhilETaylor
Copy link
Contributor

This has caused a b/c break :-( details incoming...

@PhilETaylor PhilETaylor mentioned this pull request Mar 11, 2021
Closed
@PhilETaylor
Copy link
Contributor

Serialization of 'Closure' is not allowed #40445

@jderusse jderusse deleted the twig-render-email-once branch March 11, 2021 13:38
@fabpot fabpot mentioned this pull request Mar 29, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz approved these changes

@Nyholm Nyholm Nyholm approved these changes

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
Feature Status: Reviewed TwigBridge
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.

TemplatedEmail are rendered two times
8 participants
@jderusse @Nyholm @carsonbot @ajgarlag @derrabus @PhilETaylor @javiereguiluz @nicolas-grekas