LegacyEmailBuilder calls hook_mail() too early

I think I facing the same issue. when using “resend receipt” from backend, I could not find a mailer policy config to use the custom twig in my custom template.

I'm having the same issue. When sending an order confirmation from checkout it uses the template from my custom theme (which is good!) but when resending the receipt from the admin section, it uses the template from my admin theme.

In my Symfony Mailer policy i've added a policy for commerce which says always to use the template of my theme.

I am having a real issue with:
commerce-invoice-confirmation.html.twig
it never gets used. so I can't deliver a customized invoice mail.
Strange thing: the
commerce-invoice.html.twig
file from my custom theme IS used....

any help appreciated!

CommerceEmailBuilder was removed in #3276029: Improve email creation, released in alpha8.

So with alpha9, it should work as you expect. The trace from the issue summary suggest the old code hasn't fully been removed. Please make sure you have cleared caches, make sure when you upgrade you deleted the entire directory first (so CommerceEmailBuilder.php is not still left over from earlier versions).

CommerceEmailBuilder will later be added back again in #3271421: Full integration with commerce module - probably as part of an optional submodule symfony_mailer_commerce. In that case it would start working what you call the "wrong" way again. This would be working as designed. In the long term I would like drop the "legacy" templates from each module that created it's only system such as commerce, simplenews. Instead all modules can share the same email_ templates. In simplenews, moving to symfony_mailer saved 2000 lines of code! See #3275129: Use Symfony Mailer API to send mails.

I never installed alpha8 and the right template gets picked up but only when going through the checkout flow.
When resending from the admin section, it uses the template of the admin section and not my theme.

i've added a policy and added the theme (my custom one) but still no luck.

When resending from the admin section, it uses the template of the admin section and not my theme.

When resending from the admin section, I expect you are using the admin theme. Please check your Mailer Policy for "Theme" elements. The default policy for All Mails is "Active with fallback" which means: use the active theme, unless the admin theme is active in which case use the default theme.

I just checked my folder /web/modules/contrib/symfony_mailer/src/Plugin/EmailBuilder
and there is no CommerceEmailBuilder.php
cleared caches couple of times.

When resending from the admin section, I expect you are using the admin theme. Please check your Mailer Policy for "Theme" elements. The default policy for All Mails is "Active with fallback" which means: use the active theme, unless the admin theme is active in which case use the default theme.

I have these exact policy settings and when resending from admin the custom twig files are not used.

I have experienced the same problem.

Updating the ThemeAdjuster in the policy has no effect, since that only switches the theme for rendering the email.html.twig file.
However, at that point the {{ body }} variable in that file, has already been rendered as HTML by the symfony_mailer_bc module.
So you end up with an email that has a "wrapper" rendered by symfony_mailer and the correct theme, containing content rendered in the currently active theme (in case of resend - this is the admin theme).

That variable is rendered via the commerce-order-receipt.html.twig template. This happens in the \Drupal\symfony_mailer_bc\MailManagerReplacement::emailFromArray() method, which gets called way early, and thus uses the currently active theme for rendering.

I tried changing the ThemeEmailAdjuster to react to the init phase, but that seems to late as well.

I have created a new issue fork / MR that has some proof-of-concept code that fixes the issue for me.
Please note this is not a final solution that should be applied on your production site!

This code switches the theme to the current site default theme, before the rendering of the mail body takes place.
I suspect this would only impact the rendering of the mail contents (and only when symfony_mailer_bc is enabled of course). The whole email template could still be adjusted with the ThemeAdjuster, but that is untested.

This change would affect all BC mails, so could potentially break other emails in Drupal, that did not expect this behaviour (e.g. other mails that actually want to use the admin theme).

It would be good if this approach could get some review (on your dev sites!). If it would be deemed a good approach, I guess we could potentially provide a simple configuration page for symfony_mailer_bc, that allows deciding via the UI which theme to use for symfony_mailer_bc email content - kind of like the mailsystem module does.

Obviously the better approach would probably be #3271421: Full integration with commerce module, but my solution could probably provide a fix in the meantime.
I assume we should make the theme switching optional and disabled by default, indicating they should only configure the theme switch if they experience issues or want to use commerce_order_receipt.

Thanks @svendecabooter that makes things clear. So this is a real bug with LegacyEmailBuilder which should be fixed. Even after #3271421: Full integration with commerce module, the problem could be seen with another module.

I suggest that the correct fix is in LegacyEmailBuilder.

1. In fromArray() call only newModuleEmail().
2. Move the rest of the code into preRender() - which is after the theme switching occurs in Mailer::doSend() around line 172.

I have updated the merge request with the feedback by AdamPS, and removed my temporary fix.
This does indeed fix the Commerce admin receipt emails, while still working for other legacy mails, in my testing.

cool. fixed the issue in my local dev.

Great thanks

This appears to call MailManagerReplacement::emailFromArray() in the pre-render phase which calls setTo().

Since #3276863: Calculate langcode and account from to address setTo() is only valid in the build phase. Did anyone test with dev / alpha9? I trusted you and didn't test for myself😃.

I tested this with the latest code in the 1,x branch, and did not experience any issues with it, or saw any errors.

Thanks for the clarification Sven. I ran some tests and here's what I found.

MailManagerReplacement::emailFromArray() calls setAddress('To') rather than setTo() which bypasses the validity check. I've just pushed a commit that fixes that because it is a real problem. It means that hook_mail() runs with the wrong langcode and user.

I propose to fix as follows:

1. Call setTo() in new function LegacyEmailBuilder::build().
2. Please use an {@inheritdoc} comment for build() and preRender() for consistency even though it's not strictly true (if we are going to change this please let's change them all together).
3. Remove 'To' from MailManagerReplacement::HEADERS.
4. Move MailManagerReplacement::emailFromArray() to LegacyEmailBuilder::emailFromArray() because it's no longer really a general-purpose function, and it's only called from this one place anyway. We can therefore remove two branches of code: the one referring to EmailInterface::PHASE_INIT and the one with setHtmlBody().
5. LegacyEmailBuilder no longer needs __construct(), create(), ContainerFactoryPluginInterface.
6. Clearly this is very subtle code that is easy to break. Please can we have a test? We can add a hook_mail() implementation in simplenews_test.module and create a test that calls it. It would be great to check that the theme and langcode are correct.

If we can fix this in the next week then I will wait and then create a release including it. If not then I will back this out, and we can fix it in the next release. Many thanks.

RIght now I can see fatal error with message:

The website encountered an unexpected error. Please try again later.<br><br><em class="placeholder">LogicException</em>: setAddress function is only valid in phases 1-1, called in 2. in <em class="placeholder">Drupal\symfony_mailer\Email-&gt;valid()</em> (line <em class="placeholder">542</em> of <em class="placeholder">modules\contrib\symfony_mailer\src\Email.php</em>). <pre class="backtrace">Drupal\symfony_mailer\Email-&gt;setAddress(&#039;To&#039;, Array) (Line: 149)

when using drupal mailManager

$params = [
        'from' => 'info@mydomain.com',
        'replyTo' => $this->currentUser->getEmail(),
        'name' => $data['name'],
        'last_name' => $data['last_name'],
        'to' => $data['to'],
        'short_code' => $data['short_code'],
      ];

      $result = $this->mailManager->mail('app', 'invite_client', $data['to'], $this->currentUser->getPreferredLangcode(), $params, NULL, TRUE);

I do not want to duplicate the answers here #3276863: Calculate langcode and account from to address

Merge request has been updated to account for point 1 to 5.
This should fix the reported error with setAddress()

The automated test would have to be added to symfony_mailer_bc module specifically I think.
Today I no longer have time to look into that... will see if this is possible in the next few days.

Great many thanks. I made some minor comments that are hopefully quick to resolve.

If necessary, I'm willing to commit this without tests provided it has been manually tested. Hopefully we could at least have the tests as a follow-on tasks, because otherwise I feel this can easily get broken in future.

Ok, I did debug research why content is without correct translations. So in my opinion hook_mail is called in incorrect language context so when you get your mail body texts from config, you have always your original language.
Previous, it was working properly. Right now to make it works, you have to load your configs with language override service.

$mailTemplates = \Drupal::languageManager()->getLanguageConfigOverride($message['langcode'], 'app_mails.settings');

@lamp5 where would that code go?
In the LegacyEmailBuilder we are calling hook_mail with the $message['langcode'] set correctly, so that should presumably work as intended, although I have not tested it myself.

It seems you are getting your mail templates through a custom module or implementation, so not sure if the unwanted behaviour is not caused by that specific implementation?

Merge request has been updated to fix the code review remarks.
If we can get some people to do manual tests, I guess it can be committed with a followup task to add the tests later on.

How it looks like in my project:
We have settings form where we define our mail body templates and we can translate those settings using config translations module.
Next, we send email using drupal core mail service, everything works correctly also with swiftmailer and previous with symfony mail, but after #3276863: Calculate langcode and account from to address, it stops working.

example of sending emails.

$result = $this->mailManager->mail('app', 'invite_client', $data['to'], $this->currentUser->getPreferredLangcode(), $params, NULL, TRUE);

example of hook_mail implementation:

function app_mail(){
$mailTemplates = \Drupal::config('app_mails.settings');

...
// prepare mail content, tokens etc.

}

Every email content is in default language right now, but when I change config using languageOverride

$mailTemplates = \Drupal::languageManager()->getLanguageConfigOverride($message['langcode'], 'app_mails.settings');

The mails content is in correct language.

I have now added a functional test for the symfony_mailer_bc module, to check if it uses hook_mail correctly.
Extra logic for langcode and templates still need to be added.

Great many thanks for the test @svendecabooter.

@lamp5 Thanks for all the information. Your code looks good to me. I agree it will fail as you say with the dev release. However MR25 should fix it - have you tested with that?

I am trying to apply MR25 as patch. but always get
Could not apply patch! Skipping. The error was: Cannot apply patch https://git.drupalcode.org/project/symfony_mailer/-/merge_requests/25.diff

Finally, I have updated the test to add test coverage for the logic where Drupal commerce renders the commerce_order_receipt template file within the mail body.

The test case checks both the scenario where the default module template file is used, as well as the scenario where this template is overridden in the (custom) active theme.

@nojj are you applying the patch to the latest 1.x development branch?

@nojj are you applying the patch to the latest 1.x development branch?

both. alpha 9 and latest dev.

after reinstalling latest dev, the patch applies now.
but not with https://www.drupal.org/project/symfony_mailer/issues/3261807#comment-144...
or merge request !22

Many thanks Sven - I hope to review and commit in the next few days, then make a new release.

the patch applies now.

great

but not with

Of course patches on different issues may clash - that's why we like to get them committed😃

or merge request !22

The other MR (!24) has already been committed so should not be applied if using dev.

latest dev containing MR!24 and with
MR !25 fixes my template loading issues.

but - of course - brakes the attachment sending based on MR!22 including WSOD for admin/config/system/mailer.
I posted the error log there .

Great thanks for the updated patch. I found a couple of quick fixes to do then I will commit.

Many thanks @svendecabooter for your time and patience. It's great that we now have tests for this important part of the code.

Please see #3334575: Allow override of LegacyEmailBuilder with configured policy which updates the code written in this patch - still keeping the test working of course. Review or testing appreciated from people here who understand the background.