Reimplement mail theme functionality

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.

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.

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.

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.

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

+++ 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

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.

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.

Wrong files.

Starting codes for mailsystem theme test.

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

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?

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.

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.

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.

Done.

Missing mailsystem.services.yml file.

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

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.

+++ 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

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.

Fixes based on comment #26.

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

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

Forgot to change order also in MailsystemServiceProvider.

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 }}

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.

Excellent.