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

Cannot modify email headers in SendNewsletterEvent #6189

Open
fritzmg opened this issue Jul 4, 2023 · 7 comments
Open

Cannot modify email headers in SendNewsletterEvent #6189

fritzmg opened this issue Jul 4, 2023 · 7 comments
Labels
Milestone

Comments

@fritzmg
Copy link
Contributor

fritzmg commented Jul 4, 2023

Affected version(s)

4.13, 5.0+

Description

In #3489 we introduced the SendNewsletterEvent as a replacement for the sendNewsletter hook. Unfortunately we overlooked that the new event is much more limited compared to the hook as the event does not provide the actual email instance and thus one cannot modify other email meta data of the newsletter email anymore - like Reply-To or other headers.

/cc @SeverinGloeckle

@fritzmg fritzmg added this to the 4.13 milestone Jul 4, 2023
@SeverinGloeckle
Copy link
Contributor

The SendNewsletterEvent is aimed at keeping the editable options independent of the concrete email implementation. I think that the argumentation outlined in #3489 (comment) still holds up.

You are right, however, that the event now lacks potentially relevant editing options.

The following options are already available in the event:

  • set recipient address
  • set text content
  • set html content
  • skip sending
  • enable/disable html
  • access recipient data
  • set recipient data (which was mainly relevant for further processing in the old hook in Contao 4.13)
  • access newsletter data
  • set newsletter data (which is only relevant in between multiple event handlers)

For modifying email data, it would be conceivable to implement one of the following:

  1. provide the Contao email object via the event
  2. provide the actual email object ($objMessage in the Contao email).
    • The actual object can currently be a Swift_Message or a Symfony email.
    • In this case, Contao's Email must also be modified to provide direct access to $objMessage.
  3. provide an internal interface for the actual email object
    • This interface maps the most important editing capabilities of the email (provided by both Swift_Message and Symfony's email).
    • In this case, Contao's Email must also be modified to provide direct access to $objMessage.
  4. provide defined access options in the event and processing them in the newsletter class (e.g. setCc, setReplyTo, ...)

From my point of view:

  • Variant 1 is not sustainable, as it is too tied to the Contao email.
  • Variant 2 is not sustainable, as it is too tied to the concrete email implementation.
  • Variant 3 is possible, but may require an intermediate class in the future if new email implementations do not provide certain methods.
  • Variant 4 is the easiest way, especially for a limited amount of options (e.g. only CC, BCC, Reply-To), but requires some redundant code (get/set for each value in the event, apply each value in the newsletter class).

If we can agree on a variant or if someone has a more effective solution, I can take care of the implementation.

@fritzmg
Copy link
Contributor Author

fritzmg commented Jul 5, 2023

imo variant 1 is the only viable option.

  1. We haven't deprecated Contao\Email.
  2. We can change it to Symfony\Component\Mime\Email in Contao 6 (and only then).
  3. Variants 3 and 4 are unnecessary overhead and work.

@m-vo
Copy link
Member

m-vo commented Jul 5, 2023

We have not deprecated the Email class (yet?), but putting it in the event would make this somewhat impossible. Even changing the namespace would be a break. I'd favor variant 4 I think.

@fritzmg
Copy link
Contributor Author

fritzmg commented Jul 5, 2023

We have not deprecated the Email class (yet?), but putting it in the event would make this somewhat impossible.

Why? We need to support it for the remainder of Contao 5's lifetime and in Contao 6 the event's getMessage() class will then return a different instance.

@SeverinGloeckle
Copy link
Contributor

SeverinGloeckle commented Jul 6, 2023

There is also a variant 1.1 in which the Contao email object is provided via an interface (e.g. Contao\Mailer\ContaoEmail), in which irrelevant parts like hasFailures() are not included. However, since the class includes a hidden logic for the recipients - compileRecipients() - which goes beyond an actual email message and should not be relied on implicitly, this way doesn't seem ideal either.

The question is how much work and thought is actually required in this limited case (-> sendNewsletterEvent) and what is better discussed elsewhere (how to deal with Contao\Email and legacy classes in general).

A very strict solution might also be to explicitly exclude the modification of the e-mail message using the event. The user may still implement this via a custom message and transmission and deactivate the normal logic with setSkipSending(). The relevant data of the newsletter and the recipient are already included in the event.

@fritzmg
Copy link
Contributor Author

fritzmg commented Jul 6, 2023

There is also a variant 1.1 in which the Contao email object is provided via an interface (e.g. Contao\Mailer\ContaoEmail), in which irrelevant parts like hasFailures() are not included.

What purpose would that have? i.e. why should we implement an additional layer?

A very strict solution might also be to explicitly exclude the modification of the e-mail message using the event.

Why would we want to restrict that?

@leofeyer leofeyer added up for discussion Issues and PRs which will be discussed in our monthly Mumble calls. and removed unconfirmed labels Aug 16, 2023
@fritzmg
Copy link
Contributor Author

fritzmg commented Aug 24, 2023

As discussed in the call we want to refactor the newsletter bundle to not use Contao\Email anymore and instead use symfony/mime and symfony/mailer directly. This shows us a way forward in getting rid of Contao\Email altogether and we'll introduce stamping or other identifiers to the message/envelope which can be processed in mailer events.

@leofeyer leofeyer removed the up for discussion Issues and PRs which will be discussed in our monthly Mumble calls. label Aug 24, 2023
@fritzmg fritzmg modified the milestones: 4.13, 5.3 Aug 25, 2023
@leofeyer leofeyer added feature and removed bug labels Nov 8, 2023
@leofeyer leofeyer modified the milestones: 5.3, 5.4 Jan 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants