8000 [Messenger] Templated email messages fails when sending async (thru messenger transport) by ronnylt · Pull Request #39458 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Messenger] Templated email messages fails when sending async (thru messenger transport) #39458

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
Closed
wants to merge 2 commits into from
Closed

[Messenger] Templated email messages fails when sending async (thru messenger transport) #39458

wants to merge 2 commits into from

Conversation

ronnylt
Copy link
Contributor
@ronnylt ronnylt commented Dec 11, 2020
Q A
Branch? 5.x
Bug fix? yes
New feature? no
Tickets Fix #39190
License MIT

@ronnylt ronnylt mentioned this pull request Dec 11, 2020
Closed
@ronnylt
Copy link
Contributor Author
ronnylt commented Dec 11, 2020

What is the intended behaviour when queueing template messages, doing the rendering before enqueuing the message for async sending, OR just before actually sending the message to the mailer transport?

In other words, is rendering intended to be done before dispatching to the messenger's bus?

I think this clarification is important for a fix.

@ronnylt ronnylt force-pushed the failing-test-templaed-queued-emails branch from 6af42cd to fa6e8e7 Compare December 11, 2020 12:31
@jderusse
Copy link
Member
jderusse commented Dec 11, 2020 •
edited
Loading

One benefit of rendering the template BEFORE queuing, is: the extensions have access to the request context.

  • can render absolute links with scheme and domain
  • have access to the logged user

When switching to async, you have to provide the needed context (ie. framework.router.default_uri, or passing context in the render method).

Rendering template is fast. I'm not sure adding such complexity and bugs worth it.

@jderusse jderusse added this to the 5.x milestone Dec 11, 2020
@carsonbot carsonbot changed t 8000 he title Templated email messages fails when sending async (thru messenger transport) [Messenger] Templated email messages fails when sending async (thru messenger transport) Dec 11, 2020
@ronnylt ronnylt force-pushed the failing-test-templaed-queued-emails branch from 859598c to 9cf10d5 Compare December 12, 2020 07:11
@ronnylt ronnylt force-pushed the failing-test-templaed-queued-emails branch from 9cf10d5 to 3c3c39f Compare December 12, 2020 07:12
@ronnylt
Copy link
Contributor Author
ronnylt commented Dec 12, 2020

@jderusse +1 on rendering before queuing, this also allows to have a minimalist worker application that only takes care of sending ready-to-send emails.

The proposed fix removes the cloning of the message before sending it to the EventDispatcherInterface. This way the message that is transformed by body renderers, etc... is the same that is actually enqueued.

Not sure if the cloning is needed or if removing this can have any other side-effects.

Any ideas? Thanks.

@ronnylt
Copy link
Contributor Author
ronnylt commented Dec 21, 2020

Hi! Can anyone provide any updates or guidance on how to proceed?

Thanks!

@mburtscher
Copy link

Really looking forward to this fix! The change actually revert's @fabpot's intention to not use the same variable name as changed in commit 829566c. The actual bug was introduced later in commit fc4be48 with removing the second event dispatched.

@perebusquets
Copy link

Also looking forward to implement this fix, any idea on when can we expect this to be fixed?

@OskarStark OskarStark requested a review from fabpot January 30, 2021 16:30
@fabpot fabpot modified the milestones: 5.4, 6.1 Nov 16, 2021
@fabpot fabpot modified the milestones: 6.1, 6.2 May 20, 2022
@fabpot
Copy link
Member
fabpot commented Jul 24, 2022

Closing as this fix is not valid.
See #47052 and #47049 for more explanations.

@fabpot fabpot closed this Jul 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot Awaiting requested review from fabpot

Assignees
No one assigned
Labels
Bug Messenger Status: Needs Review
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.

Templated email messages fails when sending async
6 participants
@ronnylt @jderusse @mburtscher @perebusquets @fabpot @carsonbot
0