Problem

I stumbled accross this issue lately after some rework on my theme, everything showing fine in my aggregated css file but not in my .emimages.css file.

The problem seems to be there :

$contents = preg_replace_callback('/[^}]+{[^{}]+(url\([\'"]?'. preg_quote(base_path(), '/') .'([^\'")]+)[\'"]?\))[^{}]*}/i', '_css_emimage_replace', $contents);

As the php documentation says, if there's an error while processing the preg_replace_command, it returns NULL, and you end up with an empty file.

Solution(s)

Handle NULL return

One of the most obvious solution would be to check if $content == NULL. If it does, then a safe solution would be not to replace in $styles and keep the file as is, and to log an error in drupal log mentionning the failure, along with preg_last_error() value.

It is still good to generate the (empty) .emimages.css file since it will avoid spaming the log with this issue.

Avoid this error

Looks like most of the time, the error may happen due to a backtrack limit issue with preg library.
A solution would be to mention, both in the module documentation and in the log message, that increasing this value (adding php_value preg.backtrack_limit 500000 to .htaccess) may solve the problem.

Sorry I don't post a patch, I don't really have any "dev" environment here, wouldn't be able to test it properly.

CommentFileSizeAuthor
#2 backtrack_limit.patch2.49 KBcoutud
#1 backtrack_limit.patch3.69 KBcoutud

Comments

coutud’s picture

coutud commented
Status: Active » Needs review
StatusFileSize
new backtrack_limit.patch3.69 KB

After further investigation, the problem appears when there are already big css blocks in the file, for example when there are already big url(data: blocks.

In my case, it appeared with a font definition using base64 encoded font file.

The attached patch :

  • Changes the regex, adding a } (or beginning of the file) as the first matching character, so that pcre won't try to start match unless a previous css expression has stopped. That's the main thing to avoid the backtrack error.
  • Assume there won't be any slashes in a css declaration block, only in a url() block. This way, the pcre engine can "reset" its backtrack status if there's one /, which happens frequently in any base64 encoded block. So instead of keeping in memory (and hitting backtrack limit) the full base64 encoded string, it will "reset" its backtrack at the latest / at most
  • Only replaces urls ending with .gif, .png or .jpg. This ensure that the modules doesn't try to embed other files, like .svg, .ttf or .eot that are used with web-fonts css declarations.
  • Display a meaningful error in the watchdog if the regular expression replacement failed anyway, and output the non-processed content to the .emimages.css file, to avoid empty css files.

This is tested on my machine. As you can see, I had to add a match in the regex to have the css block without the starting }, hence some renumbering in the _css_emimage_replace function.

coutud’s picture

coutud commented
StatusFileSize
new backtrack_limit.patch2.49 KB

Attached is a better patch, with a smarter regex.
The starting } isn't included anymore (so matches don't change), which actually solves an issue where two consecutive css rules have a url() block in them.

.class1{background:url('blah.gif');}.class2{background:url('blah.gif')}

Would have the first } included in the first match, hence the second expression wouldn't match.

jcarnett’s picture

jcarnett commented
Status: Needs review » Patch (to be ported)

The patch appears to work well when tested against a few relatively complex themes. I went ahead and committed it to 6.x-1.x-dev: http://drupal.org/cvs?commit=455550.

I'll port to 6.x-2.x then push new releases and likely make 2.x the recommended release at the same time.

Thanks for your help.

jcarnett’s picture

jcarnett commented
Status: Patch (to be ported) » Needs work

I actually did end up finding an issue with this patch. The exclusion of slashes from property values causes declarations like the following to be skipped and not embed the image:

.example {
  font: 12px/170% Verdana, sans-serif;
  background: url(/example-bg.jpg);
}

I spent some time looking for a fix but I don't have one yet so if you have any ideas let me know. For now I'll partially revert the patch in 1.x-dev to the previous pattern, but keep the logging and fallback on error. If that ends up being the only improvement here, I guess I'd be satisfied.