When using image lazyloader and the "Exclude images based on their image style" function i got this error.
Error: "Compilation failed: range out of order in character class at offset 121". This is because in lazyloader_excluded_by_image_style function

This is because the regex in lazyloader_excluded_by_image_style will fail if Image style name contains certain non escaped characters, in my case a hyphen "-".
Looking closer at the code there was more stuff going on...for example the enabled image style names were compared to the current image url - but we have access to the current image style name through $variables['style_name'].

see patch attached.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wejoko created an issue. See original summary.

office@w3k.at’s picture

Issue summary: View changes
FileSize
1.52 KB
office@w3k.at’s picture

Issue summary: View changes
legolasbo’s picture

Status: Active » Needs work

Thanks for the patch. Please see my comments below

  1. +++ b/lazyloader.module
    @@ -256,22 +256,25 @@ function _lazyloader_excluded_by_filename($variables) {
    -  $excluded_styles = _lazyloader_filter_selected_values(variable_get('lazyloader_image_styles', array()));
    -  // If no image styles are selected we have nothing to exclude.
    -  if (empty($excluded_styles)) {
    +  $enabled_styles = _lazyloader_filter_selected_values(variable_get('lazyloader_image_styles', array()));
    +
    +  // If no image styles are selected we do nothing.
    +  if (empty($enabled_styles)) {
    

    I like this change, but it requires an upgrade path.

  2. +++ b/lazyloader.module
    @@ -256,22 +256,25 @@ function _lazyloader_excluded_by_filename($variables) {
    + ¶
    +  // If the image does not use a style but an image style is selected we disable lazyload. ¶
    

    Extraneous whitespace should be removed.

  3. +++ b/lazyloader.module
    @@ -256,22 +256,25 @@ function _lazyloader_excluded_by_filename($variables) {
    +	  return TRUE;
    

    Wrong indentation

  4. +++ b/lazyloader.module
    @@ -256,22 +256,25 @@ function _lazyloader_excluded_by_filename($variables) {
    +  ¶
    +  // If current image style is not one of the enabled image styles we disable lazyload. ¶
    

    Extraneous whitespace should be removed

  5. +++ b/lazyloader.module
    @@ -256,22 +256,25 @@ function _lazyloader_excluded_by_filename($variables) {
    +	  return TRUE;
    

    Wrong indentation.

  6. +++ b/lazyloader.module
    @@ -256,22 +256,25 @@ function _lazyloader_excluded_by_filename($variables) {
    +  ¶
    

    More whitespace

adrien.felipe’s picture

Patch #2 works for me, thanks. Sorry I can't apply the review to a new patch now.

Christopher Riley’s picture

Patch 2 cleaned up my issue, thanks!

rubymuse’s picture

I'm no regexp expert, but I think it should be
return !preg_match('/styles\/(' . $excluded_styles . ')/', $variables['path']);

instead of
return !preg_match('/styles\/[' . $excluded_styles . ']/', $variables['path']);

(parens instead of square brackets)

adrien.felipe’s picture

@rubymuse
[] will match only characters included in $excluded_styles
while I believe we need to match all styles names in $excluded_styles
which () will do as long as they are separated by|

$excluded_styles should be something like "style1|style2|style3"
to have a final regex: preg_match('/styles\/(style1|style2|style3)/'

I'm just guessing. But I think it's correct, or at least [] would be incorrect

adrien.felipe’s picture

@rubymuse, I read it the other way around. You're absolutely right.

jenlampton’s picture

I like this change, but it requires an upgrade path.

Are you sure about that? The label on the field in the UI says `Enabled for image styles`. It looks like the patch treats them as the user expects.

I've fixed the whitespace issues with the patch. Marking NR.

rmajed’s picture

patch of comment #2 didnt apply for my branch and so thats a new patch for it

Status: Needs review » Needs work

The last submitted patch, 11: exclude-images-based-on-their-image-style-regex-error-2649260-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

rmajed’s picture

extending patch #11 to include the echo version library to be able to detect it and use it for other modules as lazyloader_filter and other fix to prevent migration failure

jenlampton’s picture

Re-roll of patch with code standards fix. Not sure why that test is failing. Testing again.

jenlampton’s picture

Hm, that patch seemed to contain more than intended.... trying again.

jenlampton’s picture

I've been having some trouble with the latest versions of patches here. I'm currently getting a lot of errors in my logs that resemble this:

PHP Warning: preg_match(): Compilation failed: range out of order in character class

Since I'm not a fan of including another library without very good cause, so I'm rolling back to the patch in #10, which worked (and still applies cleanly to the 2.x branch). re uploading here for sanity.

aquaphenix’s picture

Re-roll of patch #16 as it doesn't apply correctly with other patches.