The first file_exists() is wrong. Although it happens to return the right value: FALSE. Which means the drupal_get_path() is executed as intended. I suggest you return the if(), but keep the $styles assignment.

Are you working with E_NOTICE turned ON? If not, you probably want to do so because this one you'd get an error in your logs...

Thank you.
Alexis

function template_preprocess_mimemail_message(&$variables) {
  $theme = variable_get('theme_default', NULL);

  // Check for the existence of a mail.css file in the current theme folder
  if (!file_exists($styles)) {   // <- remove this line.
    $styles = drupal_get_path('theme', $theme) .'/mail.css';
  }

...
CommentFileSizeAuthor
#1 mimemail-css_media-6.x.patch1.41 KBAlexisWilke

Comments

AlexisWilke’s picture

AlexisWilke commented
Title: Invalid test of undefined variable in CSS gathering » CSS concatenation problems & solution
StatusFileSize
new mimemail-css_media-6.x.patch1.41 KB

Actually, you select all the CSS from the stylesheets. You may want to define a variable that people can set to the list of CSS media to select.

That way we could have any number of CSS stylesheets depending on different modules, etc. and by setting the media to 'mail' it would be picked up by your system automatically.

As you will see in the patch, at this time I suggest to gather the CSS media set to 'all' and 'screen'. The best could very well be to select 'mail' media only, although no theme current defines it...

Again, this would give other modules a way to offer their own CSS files via the normal Drupal CSS system and not some weird hack.

This being said, it could have some side effects... TBD...

The patch also fixes my first bug report (i.e. Invalid test of undefined variable in CSS gathering).

Thank you.
Alexis Wilke

sgabe’s picture

sgabe commented

Thanks Alexis!

I think the entire template_preprocess_mimemail_message() function needs rework. Since there are several issues it is not easy to do, but you can take a look at it in #743150: Mime Mail 6.x-1.0-beta1? and see what I mean.

Your idea is good, but I am afraid we can implement it only in a future branch and version of Mime Mail, because an important feature of the module is that it looks "almost" like the site and this works out-of-the box. Please, read Allie's comment about this here #108573: Embed CSS images (#6). IMHO when Mime Mail is stable enough I would create a new branch in which we can introduce this change so theme developers can prep with a mail style.

However there are some related issues in the queue, I think right now we should move forward with these:

You can see the whole together in the latest patch in #743150: Mime Mail 6.x-1.0-beta1?. It is out of date now, but I would think something like that.

sgabe’s picture

sgabe commented

Or if you can rework your patch with 'mail' to be optional and fallback to all 'and' screen 'media' that would be nice. Otherwise this issue could be a duplicate of #443964: Skip style sheets with print media with another approach.

AlexisWilke’s picture

AlexisWilke commented

You want to fix the if() shown in #0

And maybe set $css to '' before the for loop.

Then you can close this issue.

Thank you.
Alexis

sgabe’s picture

sgabe commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Needs review » Fixed

This issue is covered by others and the fixes have been committed.

Status: Fixed » Closed (fixed)

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