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

CommentFileSizeAuthor
#33 mailsystem_theme_mail_test-2498265-33.patch19.49 KBtduong
#33 interdiff-2498265-31-33.txt658 bytestduong
#31 mailsystem_theme_mail_test-2498265-31.patch19.49 KBtduong
#31 interdiff-2498265-29-31.txt3.73 KBtduong
#29 mailsystem_theme_mail_test-2498265-29.patch19.36 KBtduong
#29 interdiff-2498265-24-29.txt5.95 KBtduong
#24 mailsystem_theme_mail_test-2498265-24.patch18.91 KBtduong
#24 interdiff-2498265-22-24.txt393 bytestduong
#22 mailsystem_theme_mail_test-2498265-22.patch18.45 KBtduong
#22 interdiff-2498265-18-22.txt1.72 KBtduong
#18 mailsystem_theme_mail_test-2498265-18-applied_patch.patch18.39 KBtduong
#18 mailsystem_theme_mail_test-2498265-18.patch6.89 KBtduong
#18 interdiff-2498265-15-18.txt5.7 KBtduong
#18 mailsystem_theme_mail_test-2498265-18-applied_patch.patch18.35 KBtduong
#15 mailsystem_theme_mail_test-2498265-15.patch5.4 KBtduong
#13 mailsystem_theme_mail_test-2498265-13.patch5.87 KBtduong
#12 mailsystem-theme-mail-2498265-10-interdiff.txt1.72 KBBerdir
#12 mailsystem-theme-mail-2498265-10.patch11.92 KBBerdir
#10 theme-registry-2640962-13-interdiff.txt4.17 KBBerdir
#10 theme-registry-2640962-13.patch12.46 KBBerdir
#4 mailsystem-theme-mail-2498265-4.patch11.67 KBBerdir
#2 mail_system_theme_stuff-2498265-2.patch6.35 KBgiancarlosotelo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mbovan’s picture

Version: 8.x-2.x-dev » 8.x-4.x-dev
giancarlosotelo’s picture

Status: Active » Needs review
FileSize
6.35 KB

A 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.

Status: Needs review » Needs work

The last submitted patch, 2: mail_system_theme_stuff-2498265-2.patch, failed testing.

Berdir’s picture

Title: Fix mail theme stuff » Reimplement mail theme functionality
Component: Module compatibility » Code
Priority: Normal » Critical
Status: Needs work » Needs review
FileSize
11.67 KB

Updating 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.

Berdir’s picture

Also created #2640962: Theme Registry does not support switching the active theme, which would allow to simplify the code here and make it more reliable.

Berdir’s picture

Issue tags: +Needs tests

Tests 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.

mbaynton’s picture

@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.

podarok’s picture

Status: Needs review » Needs work
+++ b/src/MailsystemManager.php
@@ -38,11 +40,59 @@ class MailsystemManager extends MailManager {
+    $message = parent::mail($module, $key, $to, $langcode, $params, $reply, $send);

If this part fails, site becomes viewable in different theme. Right?
Let's think how we can avoid possible fail at frontend level

Berdir’s picture

If 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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
12.46 KB
4.17 KB

Added 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.

Status: Needs review » Needs work

The last submitted patch, 10: theme-registry-2640962-13.patch, failed testing.

Berdir’s picture

tduong’s picture

Starting codes for mailsystem theme test.

iainH’s picture

Still not theming per /config/system/mailsystem

After having applied the patch (mailsystem-theme-mail-2498265-10.patch against Mail 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() in MailEntity.php returns markup themed by classy as evidenced by twig debug output and the fact that preprocess functions are not invoked in the theme identified by function mailsystem_get_mail_theme()
  • This occurs whether I send test or final newsletters
tduong’s picture

Uploaded patch:

  • improved MailsystemTestController
  • moved and improved mailsystem_test.module
  • start code for test coverage

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?

Status: Needs review » Needs work

The last submitted patch, 15: mailsystem_theme_mail_test-2498265-15.patch, failed testing.

iainH’s picture

Further diagnostics of mailsystem-theme-mail-2498265-10.patch against Mail 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 in modules/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.

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'simplenews_newsletter_body' -->
<!-- FILE NAME SUGGESTIONS:
   * simplenews-newsletter-body--councillors--email-html.html.twig
   * simplenews-newsletter-body--email-html.html.twig
   * simplenews-newsletter-body--councillors.html.twig
   x simplenews-newsletter-body.html.twig
-->
<!-- BEGIN OUTPUT from 'modules/simplenews/theme/simplenews-newsletter-body.html.twig' -->

even when simplenews-newsletter-body--councillors--email-html.html.twig is in the module's AND the themes template directories.

tduong’s picture

Uploaded patch:

  • improved test coverage
  • created missing files:mailsystem_test_theme.info.yml and mailsystem_test.info.yml
  • moved and renamed mailsystem.html.twig as username.html.twig

From the patch in #12:

  • replaced $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.

The last submitted patch, 18: mailsystem_theme_mail_test-2498265-18.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 18: mailsystem_theme_mail_test-2498265-18-applied_patch.patch, failed testing.

Berdir’s picture

  1. +++ b/src/MailsystemManager.php
    @@ -38,11 +40,65 @@ class MailsystemManager extends MailManager {
    +    $mail_theme = $this->getMailTheme();
    +    $current_active_theme = $this->themeManager->getActiveTheme();
    +    if ($mail_theme != $current_active_theme->getName()) {
    +      $this->themeManager->setActiveTheme(\Drupal::service('theme.initialization')->initTheme('mailsystem_test_theme'));
    

    you hardcoded it to the test theme now. That's not correct.

    We should also inject the theme.initialization service, just like the others.

  2. +++ b/src/MailsystemManager.php
    @@ -38,11 +40,65 @@ class MailsystemManager extends MailManager {
    +      if ($mail_theme != $current_active_theme) {
    

    this also needs to be updated like the if above.

tduong’s picture

Status: Needs review » Needs work

The last submitted patch, 22: mailsystem_theme_mail_test-2498265-22.patch, failed testing.

tduong’s picture

Status: Needs work » Needs review
FileSize
393 bytes
18.91 KB

Missing mailsystem.services.yml file.

iainH’s picture

mailsystem_theme_mail_test-2498265-24.patch looking good so far. Will exercise it a bit more later today.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests
  1. +++ b/src/MailsystemManager.php
    @@ -38,11 +40,65 @@ class MailsystemManager extends MailManager {
    +    if ($mail_theme != $current_active_theme->getName()) {
    +      $this->themeManager->setActiveTheme(\Drupal::service('theme.initialization')->initTheme($mail_theme));
    

    This 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.

  2. +++ b/src/Tests/MailsystemTestThemeTest.php
    @@ -0,0 +1,77 @@
    +  /**
    +   * Tests the mailsystem theme.
    +   */
    +  public function testMailsystemTheme() {
    

    Tests the mail theme. Similar for the method name.

  3. +++ b/src/Tests/MailsystemTestThemeTest.php
    @@ -0,0 +1,77 @@
    +    // Manually configure the test mail collector implementation to prevent
    +    // tests from sending out emails and collect them in state instead.
    +    // While this should be enforced via settings.php prior to installation,
    +    // some tests expect to be able to test mail system implementations.
    

    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().

  4. +++ b/src/Tests/MailsystemTestThemeTest.php
    @@ -0,0 +1,77 @@
    +    // Send an email.
    +    $this->drupalGet('/mailsystem-test/theme');
    +    $mails = $this->drupalGetMails();
    

    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.

podarok’s picture

+++ b/src/MailsystemManager.php
@@ -78,18 +78,24 @@ class MailsystemManager extends MailManager {
+    finally {

It 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

Berdir’s picture

That 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.

tduong’s picture

Fixes based on comment #26.

Berdir’s picture

+++ b/src/MailsystemManager.php
@@ -55,14 +56,22 @@
    */
-  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler, ConfigFactoryInterface $config_factory, LoggerChannelFactoryInterface $logger_factory, TranslationInterface $string_translation, ThemeManagerInterface $theme_manager, Registry $default_theme_registry, Registry $mail_theme_registry) {
+  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler, ConfigFactoryInterface $config_factory, LoggerChannelFactoryInterface $logger_factory, TranslationInterface $string_translation, ThemeManagerInterface $theme_manager, Registry $default_theme_registry, Registry $mail_theme_registry, ThemeInitializationInterface $theme_initialization) {
     parent::__construct($namespaces, $cache_backend, $module_handler, $config_factory, $logger_factory, $string_translation);
     $this->mailsystemConfig = $config_factory->get('mailsystem.settings');
     $this->themeManager = $theme_manager;
     $this->defaultThemeRegistry = $default_theme_registry;
     $this->mailThemeRegistry = $mail_theme_registry;
+    $this->themeInitialization = $theme_initialization;
   }

Please 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.

tduong’s picture

Changed order for less problematic future changes and added variables in MailsystemManagerTest (consistency).

Status: Needs review » Needs work

The last submitted patch, 31: mailsystem_theme_mail_test-2498265-31.patch, failed testing.

tduong’s picture

Status: Needs work » Needs review
FileSize
658 bytes
19.49 KB

Forgot to change order also in MailsystemServiceProvider.

iainH’s picture

Yes, the correct theme is now being used by simplenews for both test and final emails; so as far as I can tell mailsystem 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 the simplenews issue queue.

Exception: Object of type Drupal\simplenews\Entity\Subscriber cannot be printed. in Drupal\Core\Template\TwigExtension->escapeFilter() (line 441 of core/lib/Drupal/Core/Template/TwigExtension.php).
__TwigTemplate_e2c3a14d88928850e88b4334213bfdcdede06b63d70122ea695fab6978fa7f39->doDisplay(Array, Array)
Twig_Template->displayWithErrorHandling(Array, Array)
Twig_Template->display(Array)
Twig_Template->render(Array)
twig_render_template('themes/pellucid_monoset/templates/simplenews-newsletter-body--councillors--email-html.html.twig', Array)
Drupal\Core\Theme\ThemeManager->render('simplenews_newsletter_body', Array)
Drupal\Core\Render\Renderer->doRender(Array, 1)
Drupal\Core\Render\Renderer->render(Array, 1)
Drupal\Core\Render\Renderer->Drupal\Core\Render\{closure}()
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object)
Drupal\Core\Render\Renderer->renderPlain(Array)
Drupal\simplenews\Mail\MailEntity->buildBody('html')
...

in themes/pellucid_monoset/templates/simplenews-newsletter-body--councillors--email-html.html.twig I have:

<h2>{{ title }}</h2>
<p><strong>{{ simplenews_subscriber }}</strong></p>
{{ build }}

  • Berdir committed b8f339b on
    Issue #2498265 by tduong, Berdir, giancarlosotelo: Reimplement mail...
Berdir’s picture

Status: Needs review » Fixed

Uhm. 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.

iainH’s picture

Excellent.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.