Skip to content
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.

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

[Mailer] Redundant MessageEvent dispatch #34972

Closed
raneomik opened this issue Dec 13, 2019 · 10 comments
Closed

[Mailer] Redundant MessageEvent dispatch #34972

raneomik opened this issue Dec 13, 2019 · 10 comments

Comments

@raneomik
Copy link

Symfony version(s) affected: 4.4.1

Description

When only one email is sent in a Symfony application having also the Messenger component, two MessageEvents are actually dispatched.
Not sure if it is a bug or it is done on purpose, but it brings some confusion in the Web profiler where it shows a queued email in a "[main]" transport :

image

Seen also in functional tests using non-mocked, by default injected dependencies and checking sent emails count through dispatched events.

How to reproduce

In a Symfony 4.4 application in development mode (web profiler and debug activated), having Messenger component, and an implemented Mailer.
When an email is sent and Mailer is configured for 1 dsn, it should show 2 emails in web profiler, one sent by the actual configured dsn, and one queued in a "[main]" transport.

Possible Solution
Remove the MessageEvent dispatch in Symfony\Component\Mailer\Mailer:send and keep only the Messenger's MessageBus dispatch.

Additional context
The 2 MessageEvents come from :

  • dispatched in Symfony\Component\Mailer\Transport\AbstractTransport:send
  • in Symfony\Component\Mailer\Mailer:send - if MessageBus is not null (the dispatched transport in MessageEvent is an instance of Symfony\Component\Mailer\Transport\Transports)
raneomik pushed a commit to raneomik/symfony that referenced this issue Dec 13, 2019
raneomik pushed a commit to raneomik/symfony that referenced this issue Dec 13, 2019
raneomik pushed a commit to raneomik/symfony that referenced this issue Dec 16, 2019
raneomik pushed a commit to raneomik/symfony that referenced this issue Dec 16, 2019
raneomik pushed a commit to raneomik/symfony that referenced this issue Jan 10, 2020
@fbourigault
Copy link
Contributor

I confirm this bug. In tests it make MailerAssertionsTrait::assertEmailCount seeing twice more messages than expected.

@mpoiriert
Copy link

One of the MessageEvent has the $queued property set to true so they are "different".

Problem is that the MessageLoggerListener should check this attribute before adding it to the list.

This issue is applicable on other listener.

If we take the Symfony\Component\Mailer\EventListener\MessageListener the event will be process everytime, causing the renderer to be call twice (for example the Symfony\Bridge\Twig\Mime\BodyRenderer).

I was trying to ignore some event form the listener base on $event->isQueued() but there is a problem of object reference. The message from the event and the message for the envelope is not the same on the the $event->isQueued() === false, which is not the case on the first event.

This is causing problem if we add a recipient on the message since the AbtractTransport will check the recipient from the envelope which will have a reference on the wrong message.

@fabpot
Copy link
Member

fabpot commented Apr 13, 2020

As mentioned already, the 2 events are different. One if for the message that is queued and the other is from when the message is sent.

#36445 fixes the web profiler to make things clearer and less confusing.

Regarding test assertions, I've just tested and it works:

        $this->assertEmailCount(1);
        $this->assertQueuedEmailCount(1);

@fabpot fabpot closed this as completed Apr 13, 2020
fabpot added a commit that referenced this issue Apr 13, 2020
…nd sent emails (fabpot)

This PR was merged into the 5.1-dev branch.

Discussion
----------

[WebProfilerBundle] Make a difference between queued and sent emails

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes-ish
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Refs #34972 <!-- prefix each issue number with "Fix #", if any -->
| License       | MIT
| Doc PR        | n/a

The current profiler panel for Mailer is confusing when emails are sent synchronously.

Before:

![image](https://user-images.githubusercontent.com/47313/79122064-19867f80-7d97-11ea-9371-672f1b6f2998.png)

After:

![image](https://user-images.githubusercontent.com/47313/79121986-e8a64a80-7d96-11ea-8a41-b50f1d160f5f.png)

The new version makes a difference between queued and sent messages. The goes goes for the web debug toolbar.

I've also fixed an HTML markup issue and made the display more compact.

Commits
-------

0ee98a1 [WebProfilerBundle] Make a difference between queued and sent emails
@mpoiriert
Copy link

mpoiriert commented Apr 13, 2020

Yes but the problem is that they are handle twice by the Symfony\Bridge\Twig\Mime\BodyRenderer the $event->isQueued() is never checked... So probably there is a lot of listener that process both event, that should not be the case. Having two different event would solve the issue, otherwise all listener need to be double checked and an issue open directly in each component.

@fabpot fabpot reopened this Apr 13, 2020
@weaverryan
Copy link
Member

Yes but the problem is that they are handle twice by the Symfony\Bridge\Twig\Mime\BodyRenderer

A side-effect of this is that a TemplatedEmail is rendered twice. If you're using Encore to load CSS inside those templates, then you need to do a manual autowire EntrypointLookupInterface and call reset on it.

I believe the setup to repeat is this:

A) Install mailer AND Messenger
B) But do not route email messages to a transport (so, have them still be handled sync).
C) Send an email - it will be rendered twice. That's normally maybe wasteful, but not problematic. But with EncoreBundle, the second (I think "actual" email) is sent without styles.

Cheers!

@fabpot
Copy link
Member

fabpot commented Apr 21, 2020

@weaverryan I don't get the issue. The message is cloned, so we are indeed rendering the email twice, but that's from 2 different instances (which means that I will close the issue again as there is no "bug" here).

But, back to Encore. Does that mean that you cannot call encore_entry_link_tags twice? So, if I send 2 different emails during the rendering of a page, that would be a problem as well?

Wait, I've just tried to find out. So, if you call encore_entry_link_tags in the main HTML template for the HTTP Response AND use it in an email that is sent on the same page, I get the issue as well.

It means that you can only call encore_entry_link_tags once per Request, right? If that's the case, it's not related to this issue and a more general problem with Encore (sorry, I haven't look at Encore code to see why there is a state in Encore).

@mpoiriert
Copy link

But why would we need to render the email twice ? It doesn't make any sens... I understand that the even is dipatched twice (with the isQueued returning true on the first event) but the rendering will be executed twice once for the queued email (that is not sent) and again when the email is sent (via the messenger). Should be open a issue in the twiw-bridge component to make sure the email is justrendered when "isQueued !== false" ?

@fabpot
Copy link
Member

fabpot commented Apr 21, 2020

The queued message should be rendered for the profiler. I don't see any issue here.

@mpoiriert
Copy link

Make sens for the profiler but this behaviour should not be triggered when profiler is not aviable (aka in production) but it will

@fabpot
Copy link
Member

fabpot commented Apr 22, 2020

The profiler was just an example. Tests is another one. In any case, we want to run all listeners on both messages to get the same end result (the fact that the email is sent or not is not relevant here). You might have some listeners that could do things differently depending on the queue status though, but I don't think this should be the case for the built-in ones.

@fabpot fabpot closed this as completed Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants