template_preprocess_mimemail_message() seems to be built to avoid lines in CSS files that are longer than 700 lines. It does so by using wordwrap(). However that often fails because we try to use wordwrap on optimized CSS files:

  • wordwrap() only works on whitespace.
  • Drupal's CSS optimization removes all whitespace (except if it's syntactically needed).

One consequence of this bug is that gmail previews don't work because the html-content is base64 encoded.

Here is the relevant code snippet:

  $css = '';
  if (isset($styles)) {
    // Process each style sheet.
    foreach (explode("\n", $styles) as $style) {
      if (!empty($style)) {
        $css .= drupal_load_stylesheet($style, TRUE);
      }
    }

    // Wordwrap to adhere to RFC821
    $css = wordwrap($css, 700);
  }
CommentFileSizeAuthor
#10 mimemail-2765387-wrap-css-10.patch1.87 KBtorotil
#7 2765387-wrap-css-7.patch1.89 KBtorotil
#3 2765387-wrap-css-3.patch1.87 KBtorotil
#2 2765387-remove-css-optimization-2.patch471 bytestorotil

Comments

torotil created an issue. See original summary.

torotil’s picture

torotil commented
Status: Active » Needs review
StatusFileSize
new 2765387-remove-css-optimization-2.patch471 bytes

To mitigate that issue I propose to remove the CSS optimizations as long as there is no wordwrap() implementation suitable for CSS (ie. allow breaks after any of these chars (outside of quoted strings): {};:.)

Here is a patch removing the optimizations.

torotil’s picture

torotil commented
StatusFileSize
new 2765387-wrap-css-3.patch1.87 KB

Here is a first attempt at wrapping CSS content.

pivica’s picture

pivica commented

We have a problem with default wordwrap function that works on optimized stylesheet. Because of long lines wordwrap would randomly break CSS around 700 chars. The problem happens when wordwrap break line inside of a CSS rule with quotes, you got something like this:

...font-family:Georgia,'Times New
Roman',Times,serif;font-size:12px;color:#303030;}...

Notice line break inside of font-family 'Times New Roman' string. This is not valid CSS anymore and some mail/web clients will render HTML with errors then (for example thunderbird, firefox, etc).

I've tested both of patches in comments #2 and #3 and i can confirm that with them applied CSS is generated correctly. Also patch from comment #3 works nicely and produce much better CSS then with old code; however I didn't test this a lot and didn't check code from patch so I can't say that it will work for any CSS combination.

Status: Needs review » Needs work

The last submitted patch, 3: 2765387-wrap-css-3.patch, failed testing. View results

tr’s picture

tr commented

Patch fails in PHP 5.3 because you used short array syntax, which is not supported for PHP 5.3.

The patch also introduces 7 coding standards violations.

Please re-roll the patch with these fixes.

torotil’s picture

torotil commented
Status: Needs work » Needs review
StatusFileSize
new 2765387-wrap-css-7.patch1.89 KB

Re-rolled as requested.

tr’s picture

tr commented
Issue tags: +Needs tests

I'm working on this module again because I would like to make a new D7 release, and I would like to get the CSS attachment functionality ported to D8. This seems like something that should be fixed for both.

I'm wary about making a change like this because we have over 75 thousand users for the D7 version of this module. If this introduces a new problem for even a fraction of those long-running sites, this could be a disaster. I assume that existing sites have already worked around this problem by not using optimized CSS, but any wrapping change here could potentially break those currently-working sites.

So to make me more confident, I would like to have a test case to make sure the wrapping doesn't mangle the CSS. Core Drupal has a similar test case for the CSS optimization functionality, so I think we could copy that and modify it slightly to work here.

Specifically, in core modules/simpletest/tests/common.test there's a test case called testLoadCssBasic() which loads some CSS, optimizes, then ensures the output was what we expected. We could do the same with a sample already-optimized CSS file that fails with the current wrapping (this would have to be supplied as part of the test - we need a case that is known to fail with the current code).

Having a test case that shows the new wrapping works properly for a known-problem optimized CSS will give me confidence, and will also provide a test that people can use to demonstrate and quickly fix any problems that arise from this change - all they need to do is post a new CSS file that fails to wrap properly and we can instantly confirm the problem and instantly confirm that a fix works without breaking our existing CSS test

So @torotil, @pivica, can one of you provide some CSS that fails with the current wrapping code? I can write the test, but I want to get examples from you because you are the ones that are experiencing the problem.

tr’s picture

tr commented
Related issues: +#2511524: Add a setting to turn on / off css "optimizing"
torotil’s picture

torotil commented
StatusFileSize
new mimemail-2765387-wrap-css-10.patch1.87 KB

Re-roll for the 7.x-1.2 release.

tr’s picture

tr commented
Status: Needs review » Needs work

You can't use short array syntax in Drupal 7.