Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Mail system theming stuff is not working. Most of it is commented out.
Proposed resolution
Fix it.
Remaining tasks
User interface changes
API changes
Comment | File | Size | Author |
---|---|---|---|
#33 | mailsystem_theme_mail_test-2498265-33.patch | 19.49 KB | tduong |
| |||
#33 | interdiff-2498265-31-33.txt | 658 bytes | tduong |
#18 | mailsystem_theme_mail_test-2498265-18.patch | 6.89 KB | tduong |
#12 | mailsystem-theme-mail-2498265-10-interdiff.txt | 1.72 KB | Berdir |
#12 | mailsystem-theme-mail-2498265-10.patch | 11.92 KB | Berdir |
|
Comments
Comment #1
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedComment #2
giancarlosotelo CreditAttribution: giancarlosotelo at MD Systems GmbH commentedA bit tricky , so I uncommented the line in the .module and in the theme part I added some conditionals to verify that the path exist and I also replace the old theme registry cache call.
It seems fine but I also added a test to verify that the theme is set in the mail with SimpleNews and I am getting an error, It sent the mail but apparently the Simplenews theme is not set correctly.
Comment #4
BerdirUpdating title, this is a blocker for a stable release.
This is a completely new approach and largely untested. This is based on discussions in #2600936: Test messages send using admin theme which I will close as a duplicate.
Comment #5
BerdirAlso created #2640962: Theme Registry does not support switching the active theme, which would allow to simplify the code here and make it more reliable.
Comment #6
BerdirTests would be great, but I'm considering to commit this after manual testing since writing a test isn't a trivial task for this. We need a test module that sends a mail, uses a template/render array in there for that and a test theme that would override that template so we can assert the output.
Comment #7
mbaynton@Berdir thanks for working on this! I will try it out as soon as I can...we send newsletters at the end of the month so probably either next few days or next month.
Comment #8
podarokIf this part fails, site becomes viewable in different theme. Right?
Let's think how we can avoid possible fail at frontend level
Comment #9
BerdirIf there is an exception, yes, good point. We should add a try/finally to make sure it's always called, even in case of an exception.
Comment #10
BerdirAdded a finally block. Also, the core issue is RTBC and will maybe get in soon. In that case, I'll remove the hacks as they shouldn't be required anymore. Possibly in a follow-up, depending on when this gets in.
Comment #12
BerdirWrong files.
Comment #13
tduong CreditAttribution: tduong at MD Systems GmbH commentedStarting codes for mailsystem theme test.
Comment #14
iainH CreditAttribution: iainH as a volunteer commentedStill not theming per
/config/system/mailsystem
After having applied the patch
(mailsystem-theme-mail-2498265-10.patch
againstMail System 8.x-4.x-dev (2015-Dec-16
)):function mailsystem_get_mail_theme()
correctly identifies the theme - at least in cases where I told/config/system/mailsystem
the <actual theme name> and "default"But ...
protected function buildBody()
inMailEntity.php
returns markup themed byclassy
as evidenced by twig debug output and the fact that preprocess functions are not invoked in the theme identified byfunction mailsystem_get_mail_theme()
Comment #15
tduong CreditAttribution: tduong at MD Systems GmbH commentedUploaded patch:
No interdiff, since is bigger than the patch.
I'm not sure how to proceed and what kind of email should be sent (as 'simplenews.mailer' ?). Do you have any suggestion?
Comment #17
iainH CreditAttribution: iainH as a volunteer commentedFurther diagnostics of
mailsystem-theme-mail-2498265-10.patch
againstMail System 8.x-4.x-dev (2015-Dec-16)
modules/simplenews/theme
templates are used but theme template suggestions are not, neither when the templates are placed alongside others inmodules/simplenews/theme
nor in<current theme>/templates
where<current theme>
is specified in/config/system/mailsystem
.For example: the following theme suggestion is not honoured.
even when
simplenews-newsletter-body--councillors--email-html.html.twig
is in the module's AND the themes template directories.Comment #18
tduong CreditAttribution: tduong at MD Systems GmbH commentedUploaded patch:
From the patch in #12:
$mail_theme
with\Drupal::service('theme.initialization')->initTheme('mailsystem_test_theme')
(\Drupal\mailsystem\MailsystemManager line 76)The "*18-applied_patch.patch" should work and be the final one.
Comment #21
Berdiryou hardcoded it to the test theme now. That's not correct.
We should also inject the theme.initialization service, just like the others.
this also needs to be updated like the if above.
Comment #22
tduong CreditAttribution: tduong at MD Systems GmbH commentedDone.
Comment #24
tduong CreditAttribution: tduong at MD Systems GmbH commentedMissing mailsystem.services.yml file.
Comment #25
iainH CreditAttribution: iainH as a volunteer commentedmailsystem_theme_mail_test-2498265-24.patch
looking good so far. Will exercise it a bit more later today.Comment #26
BerdirThis is not properly injected yet. You added the argument, but you also need to define it in the constructor and call it with $this->themeInitialization instead of calling out to \Drupal.
You will also need to update the unit test below similar to the existing changes.
Tests the mail theme. Similar for the method name.
We copied this from WebTestBase. I think it makes more sense to document why we have to duplicate it here. Something like:
// mailsystem uses its own configuration for the used mail plugins. Use the mail collector just like WebTestBase::initConfig().
Improve the comments here and below a bit.
Explain that the first call is with the default setting and should *not* use our test theme.
Then below, explain that we don't just install the test theme but also set it as the mail theme. And then send another module to ensure that our test theme is used and its implementation of the username template.
@ianH: Yeah, the previous patch couldn't possibly have worked. That's what happens when you don't properly test your code :) Once the review above is addressed and you can confirm that this works for you, I'll commit this and release a first alpha version for mailsystem.
Comment #27
podarokIt would be nice to catch an exception via watchdog_exception for making administrators or module developers a knowledge what was going on.
Otherwise there is a possible issue when mail is not sending, but nobody knows why it so
Comment #28
BerdirThat is a finally statement. We are not catching any exception, it bubbles up unchanged. It just means we run that code, even if an exception is thrown.
Comment #29
tduong CreditAttribution: tduong at MD Systems GmbH commentedFixes based on comment #26.
Comment #30
BerdirPlease change the order of arguments so that the two theme_registry variables are at the end.
I hope to remove them again once the referenced core issue is done, and when they are at the end, it's less problematic if the class is updated to no longer receive them.
Comment #31
tduong CreditAttribution: tduong at MD Systems GmbH commentedChanged order for less problematic future changes and added variables in MailsystemManagerTest (consistency).
Comment #33
tduong CreditAttribution: tduong at MD Systems GmbH commentedForgot to change order also in MailsystemServiceProvider.
Comment #34
iainH CreditAttribution: iainH as a volunteer commentedYes, the correct theme is now being used by
simplenews
for both test and final emails; so as far as I can tellmailsystem
is doing its job.But rendering some variables (not all) causes an exception.
This doesn't look like a
mailsystem
problem so will report it on thesimplenews
issue queue.in
themes/pellucid_monoset/templates/simplenews-newsletter-body--councillors--email-html.html.twig
I have:Comment #36
BerdirUhm. Yes, of course not :) that's an entity, you can't print an entity. that's not a bug anywhere except in your template code. So the only issue you could open there is a support request and ask how to do what you are trying to. And Drupal Answers might be a better place for that.
Anyway, thanks for the feedback. Committing this now.
Comment #37
iainH CreditAttribution: iainH as a volunteer commentedExcellent.