Mail System breaks theme registry

Not 100% why mailsystem builds the mail theme registry when it's not the active theme but I won't question that and instead will give you this patch... I'm not convinced it's the completely proper way to fix this but it works for me with an Omega based subtheme.

Patch in #1 has a typo in, doesn't appear to affect anything except a PHP notice but here's a fixed patch.

Patch #2 solved same issue on a omega 4.x subtheme.
Thanks @mjpa

Patch #2 is working here, I'd RTBC it but I needed and additional fix for a related issue: my custom theme uses some functions from the base theme, which is currently loaded after the child causing a "function not found" fatal error to be thrown. If the interdiff looks good we can RTBC this.

Patch #3 fixed "Undefined variable: row_attributes" in "views-view-unformatted.tpl.php" bug here.
We also use an Omega based subtheme here while facing this problem.
Thanks all.

I can confirm this works.

The patch is RTBC indeed, but it should definitely not be needed.

I would say there is a deeper bug within mail system, which is circumvented here.

Committed #4 to 7.x-3.x-dev, which isn't to say that there isn't room to revisit the entire thing.

Leaving this open for the 2.x branch, which I haven't touched.

Wow, very evil.
I just spent hours to figure out why flushing the cache sometimes leads to a broken frontend theme. Turns out flushing the cache in the back-end triggers if ($mailsystem_theme != $theme_key) { and thus this bug (frontend theme is set as mail theme atm.).

I really want to see this patch in and keep it on RTBC but here some nitpicks I've found:

+++ b/mailsystem.theme.inc
@@ -26,6 +26,10 @@ function mailsystem_theme_theme_registry_alter(&$theme_registry) {
+        // Swap to the mailsystem theme

@@ -75,8 +79,46 @@ function mailsystem_theme_theme_registry_alter(&$theme_registry) {
+        // Swap back to the original theme
...
+  // Make sure the theme exists
...
+  // Both theme/theme_key are set to the new theme
...
+  // Create the base_theme_info
...
+  // Some other theme related globals
...
+  // We need to reset the drupal_alter and module_implements statics

Inline comments must end in full-stops, exclamation marks, or question marks.

2 hours on the debug train myself and a few days before that going WTF and removing modules and comparing old db schemas.

Please commit to 2.x-dev
RTBC++

A way to change the theme to "admin-theme" if a newsletter is sent (by cron or by drush-cron).

First: it is not possible to detect a cron-run in hook_custom_theme() because on drush-cron there is no 'cron_key'.

Idea:

• in /admin/config/system/mailsystem I define my admin-theme as mail-theme (the place where my templates are).
• in the first template (maybe simplenews-newsletter.body.tpl.php) I call my_newsletter_set_admintheme()

That's the function:

/**
 * Set the theme to "admin-theme" - this is a brute-force-attack :-)
 * Called from within the first newsletter-template...
 *
 * Based on: https://www.drupal.org/node/2051135#comment-8650973
 */
function my_newsletter_set_admintheme() {
  global $theme, $theme_key, $base_theme_info;
  if ($theme != variable_get('admin_theme')) { // do nothing if its the right theme...
    $themes = list_themes();
    $theme_key = $theme = variable_get('admin_theme');

    // Find all our ancestor themes and put them in an array.
    $base_theme = array();
    $ancestor = $theme;
    while ($ancestor && isset($themes[$ancestor]->base_theme)) {
      $ancestor = $themes[$ancestor]->base_theme;
      $base_theme[] = $themes[$ancestor];
    }
    $base_theme_info = array_reverse($base_theme);

    // Some other theme related globals
    global $theme_engine, $theme_info, $theme_path;
    $theme_engine = $themes[$theme]->engine;
    $theme_info = $themes[$theme];
    $theme_path = dirname($themes[$theme]->filename);


    // We need to reset the theme_get_registry, drupal_alter and module_implements statics
    drupal_static_reset('theme_get_registry'); // important !!
    drupal_static_reset('drupal_alter');  // maybe - see https://www.drupal.org/node/2051135#comment-8650973
    drupal_static_reset('module_implements');

    _drupal_theme_initialize($themes[$theme], array_reverse($base_theme));

  }
}

Here's a reroll of #4 with the comments in #10.

What do we need for this to get in to 2.x? Since nothing has been done on the 2.x branch in two years, does this mean that changes are only going in to 3.x?

Hello!
#13 doesn't solve the problem described here

+1 for #14.

This warning occured during sending email via Rules and Message after editing an order:

Warning: Invalid argument supplied for foreach() in alpha_invoke() (line 99 of /home/store/public_html/profiles/commerce_kickstart/themes/omega/alpha/includes/alpha.inc).
Warning: Invalid argument supplied for foreach() in alpha_invoke() (line 99 of /home/store/public_html/profiles/commerce_kickstart/themes/omega/alpha/includes/alpha.inc).
Unable to send e-mail. Contact the site administrator if the problem persists.

And this report event from message_notify:
Could not send message using @title to user ID 141.

Environment:

I use Drupal Commerce kickstart (2x) with this related modules:

• Mailsystem 3.x (with this patch + mailsystem-use-caching-for-mailsystem_get_classes-1905544-13 + mailsystem.1534706.6 + mailsystem.1359482.5)
• Memcache
• Mime Mail
• SMTP
• My theme is a subtheme of the default kickstart theme

Trying to disable parent themes:

If (line 63 with this patch) I comment out this line, all warning disappears but email is not sent (Could not send message using @title...):

// line 63 into the foreach()
//$cache[$name]['includes'][] = drupal_get_path('theme', $base->name) . '/template.php';

...but mail not sent:

"dpm($theme_registry)" called line 82:

@comexpertise, this patch is for the 7.x-2.x branch. It was already committed to 7.x-3.x (#8).

That being said, I am leaning more towards agreeing with @Fabianx (#7). This module is doing some really nasty stuff in how it's altering the theme registry. It's very hackish at best, which can ultimately cause many different issues with other modules/themes when they expect the theme registry to be relatively stable. This module should find a different, more stable, way to output emails in the correct theme.

This begs the question though, does this module even theme any mail messages?? I have been looking through the repo and cannot find any place it actually uses theme(). The only thing I can find is in Mime Mail and HTML Mail.

This is where any "theme switching" should probably happen, right before any theme() is invoked and then switched back the the current theme after it's called.

P.S. it would be nice to get some maintainer feedback. What is trying to be accomplished with these alterations exactly? The comments and code aren't exactly self-explanatory.

@Mark Carver the recommended release is still at 2, this needs a backport to 2.x as long as that is the case.

I agree we should entertain the idea that it doesn't need the theme switching support but first, it would prevent a lot of heartache if the patch was backported. That bug was SOOO nasty.

EDIT: want to echo:

it would be nice to get some maintainer feedback. What is trying to be accomplished with these alterations exactly? The comments and code aren't exactly self-explanatory.

The reason I moved it to 7.x-3.x is because even with the patch that was committed in #8, this "solution" isn't really a solution at all.. it's just a temporary hack to get things relatively "working". This is also why I changed the title, since this implementation of the hook_theme_registry_alter() is potentially breaking everything. Once something more stable (or a different approach entirely) has been figured out, it should be committed to both branches, so I'm leaving the tag you just added.

All emails are successfully sent with rules fired from front-office but not working from back-office.

Tested with 7.x-2 and 7.x-3.x-dev.

Mails are successfully sent if "Mimemail" is used for sending mail into "Site-wide default" option. But SMTP is not used...

And these errors are still there of course:

Warning: Invalid argument supplied for foreach() in alpha_invoke() (line 99 of /home/store/public_html/profiles/commerce_kickstart/themes/omega/alpha/includes/alpha.inc).
Warning: Invalid argument supplied for foreach() in alpha_invoke() (line 99 of /home/store/public_html/profiles/commerce_kickstart/themes/omega/alpha/includes/alpha.inc).

Content of my (disabled) MimeMailSystem__SmtpMailSystem class:

class MimeMailSystem__SmtpMailSystem implements MailSystemInterface {
  protected $formatClass;
  protected $mailClass;
  public function __construct() {
    if (drupal_autoload_class('MimeMailSystem')) {
      $this->formatClass = new MimeMailSystem;
    }
    else {
      $this->formatClass = new DefaultMailSystem;
    }
    if (drupal_autoload_class('SmtpMailSystem')) {
      $this->mailClass = new SmtpMailSystem;
    }
    else {
      $this->mailClass = new DefaultMailSystem;
    }
  }
  public function format(array $message) {
    return $this->formatClass->format($message);
  }
  public function mail(array $message) {
    return $this->mailClass->mail($message);
  }
}

@comexpertise, yes it is nice to have answers. Unfortunately, there are no real answers yet. You will have to be patient until either a) someone has time to really debug what's going on here (I do not have time ATM) and/or b) we get some feedback from the maintainers as to why hook_theme_registry_alter() is being used in this fashion. Regarding the omega/bootstrap errors that this particular implementation has created, they will get resolved when this one does. As far as why the first class in #22 isn't working, I am not sure and is likely an entirely separate issue from this one.

You're right my comment # 22 does not apply to this issue, that's another problem in itself.

I took a look:

a) The patch should just be committed by Les Lim to 7.x-2.x.

I took a look at the history and 3.x is around 10 commits farther than 2.x, so this is a just get it in and get a new stable release out.

It fixes a critical. Someone should contact him directly on it.

b) I took a good look and I don't even see why the theme registry alter is needed at all.

Did someone try what happens if you completely remove it?

I assume the reason is that you cannot render something in a different theme while another theme is running.

However, for this a new API module should be created whose sole purpose is to "switch_theme" and then mail_system ported to use it.

Changing the registry is not the right way, even if the intention was good.

I've contacted Les through d.o. we can wait and see.

I have to admit to not understanding the use of hook_theme_registry_alter() here, either -- that work pre-dates my involvement and it's never come into play for my own use cases.

Based on the git logs, the original work on this was developed in #1382036: Add a common way to configure the theme that will render the email. Miro or Berdir might be able to provide more insight, and both are also co-maintainers.

Thanks for the feedback @Les Lim, any chance you can commit the stop-gap fix to 2.x as well and we can spin off the removal/refactoring of that theme swapping in another issue?

It seems this doesn't happen anymore after an upgrade to Drupal 7.34.

Patch (#3 or #14) still needed in Drupal 7.34 with Omega based subtheme.

+1 for #30.

Notice: The path do not resolve all related issues: https://www.drupal.org/node/1763362#comment-6790982

I've re-opened: #1763362-27: Omega does not work with Mail System module. Please direct your comments there.

I still think fixing this on a base-theme level is wrong and shouldn't be needed at all. The patch here is still a "hack" (since Omega still has an issue), but I'm not touching the status of this issue in hopes that even this temporarily solution can be committed to the 7.x-2.x branch.

This module should re-examine why/how it's altering the theme registry to accomplish something that (seemingly) isn't used by any other contrib module in the first place. Once a final solution has been determined, it should be committed to both 7.x-2.x and 7.x-3.x.

Patch #13 solved the problem after upgrading to 7.34 core.

This is pretty bad bug, which affects Bootstrap sub-theme. Patch #13 fixed the issue.

I agree with #7 and #18. But this bug present in recommended stable version since 2012 year. Please push at least this hotfix for now.

Thanks for the patch #13 - this fixed it for us. We can confirm this causes a lot of problems with Omega, including breaking the theme after changes are made through admin (eg saving a content type) - it was also intermittently intercepting saving changes via views.

BREAKS Bootstrap. PLEASE release hotfix with this patch! FYI - only using this module for mandrill support. Patch #13 fixes it. Note that this is a pretty hard bug to track down.

Berdir should have a look at it before commit. See #27 and #25.

I'm having the same problems, but with the Bootstrap theme. I know it's a hack and putting it in a stable release channel feels wrong, but it's definitely worse when your sites primary theme blows up (really badly).

I've patched my site and it fixes the the problem for me. I'd recommend marking this issue "Needs work" but also committing the patch in #13 and releasing as soon as possible.

@angry-dan - completely agree. The patch may not be elegant but IMO a mysterious broken theme due to mail is far worse.

We had the issue that our omega-based theme broke (e.g. nearly all classes vanished from frontend markup) after the theme registry was rebuild from within the admin theme (e.g. clear all caches or add menu item).

We applied the patch from #13 and it seemed to work at first. But later we noticed that again some things vanish from the markup, like the viewport settings (from omega theme settings).

Finally we found out that there is a much simpler solution than the patch from #13. Simply calling cache_clear_all('theme_registry', 'cache', TRUE); resolved all our issues. I have attached the change as a patch.

Hello,
I can confirm that provided patch in #40 worked for me.

Thank you,
abarpetia

A duplicate of this issue was in bootstrap theme (https://www.drupal.org/node/2615978)

I can confirm that #13 works for that issue, and 40 does not work.

Thanks to all for your work and availability !

@eliosh did you applied the patch against latest dev or against 7.x-2.34 (which is almost 4 years old) ?

I had no luck applying #13 or #40 against 7.x-2.x-dev.

I have the same problem with a bootstrap subtheme, so I have created manually a new patch based in #13 with the last 7.x-2.x-dev version.

This patch cleanly apply and works.

Patch in #40 doesn't solve the problem, maybe this is related with another bug.

The patch in #44 works great for us!

Just a nitpick about a trailing tab. ;)

+++ b/mailsystem.theme.inc
@@ -75,8 +79,46 @@ function mailsystem_theme_theme_registry_alter(&$theme_registry) {
+        mailsystem_theme_swap_theme($old_theme);        ¶

I've re-rolled patch at #44 to remove the offending white space. Hopefully it will pass testing now.

The tests failed with the response "No valid tests were specified" because of a bug in DrupalCI: #2645590: Ensure that simpletest job doesn't "fail" testing if no tests are present

The patch in #48 works great for us, thank you very much!

Not really maintaining the 7.x branches but looks like nobody else is either ;)

Code looks pretty scary but has been tested by lots of people, so, committed to 7.x-2.x.

Such a wierd bug. Tested on currently 7.x-2.x-dev version and issue is fixed - no clear cache is needed now.

Great! Would it be possible to have a new official release including this fix please?

Thank you!

The patch at #40 shouldn't be used as the impact on performance is crazy. (Can be made worse though at clearing all the cache)
Using Omega 4.4, things are fine again at simply using the dev version at that date.

This said, having a bug in the theme layout, where the cause is to be found in the mailsystem module, is very very nasty.

Thanks to the bkonetzny who spend time going to debug that.

Confirming that the patch in #44 solves the issue.

See my comment #11 on #2637832: Undefined function bootstrap_setting() for a full explanation of my testing and reproduction procedures.

Chased this bug for too long as well, any change we can get a new release, #2692985: Create a new stable release of 7.x-2.x is marked RTBC

Eagerly awaiting the release, thank you!!

Thanks for the work :) I wait also for the release.

What's the deal with line 46?

@@ -42,10 +46,10 @@ function mailsystem_theme_theme_registry_alter(&$theme_registry) {
 
         // Include template files to let _theme_load_registry add preprocess
         // functions.
-        include_once drupal_get_path('theme', $theme->name) . '/template.php';
         foreach ($base_theme as $base) {
           include_once drupal_get_path('theme', $base->name) . '/template.php';
         }
+        include_once drupal_get_path('theme', $theme->name) . '/template.php';
 
         // Get the theme_registry cache.

This causes a rejection failure during the patch.

I'm not a patch expert, but from where I sit it's re-adding the exact same line it's supposed to remove.

Can I get some clarification on what's supposed to happen there? I just deleted the lines below 46, but after thinking about it, I'm wondering if those are supposed to be there and the failure is strictly coming from nothing being changed which means I can completely ignore the error.

Running against 2.34 stable (as opposed to dev which yes, I know has been committed to dev.)

Any help from someone with better knowledge on patches would be nice.

#64: For some reason brackets after include_once have been added in the current stable release, and therefore the patch no longer applies. I had the same problem, so I rerolled #48 for and shared (only for your convenience ;)

Re: #46:

The reason that line is being moved (which is why it looks like it's being removed and then added back) is because if a theme relies on code from a base theme (before the base theme's template.php file has been loaded), it can cause a fatal. So the solution is to load the theme's template.php file after all the base themes have been loaded.

Thanks Tom & Mark :-)

#66 works fine :-)
Thanks

Drupal 7.57 + Bootstrap 7.x-3.19

Too much time spent on debuging due to an non updated module ... Thank you for the 2.35 release !