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.
Comments
Comment #2
office@w3k.at CreditAttribution: office@w3k.at commentedComment #3
office@w3k.at CreditAttribution: office@w3k.at commentedComment #4
legolasboThanks for the patch. Please see my comments below
I like this change, but it requires an upgrade path.
Extraneous whitespace should be removed.
Wrong indentation
Extraneous whitespace should be removed
Wrong indentation.
More whitespace
Comment #5
adrien.felipe CreditAttribution: adrien.felipe commentedPatch #2 works for me, thanks. Sorry I can't apply the review to a new patch now.
Comment #6
Christopher Riley CreditAttribution: Christopher Riley commentedPatch 2 cleaned up my issue, thanks!
Comment #7
rubymuse CreditAttribution: rubymuse commentedI'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)
Comment #8
adrien.felipe CreditAttribution: adrien.felipe commented@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
Comment #9
adrien.felipe CreditAttribution: adrien.felipe commented@rubymuse, I read it the other way around. You're absolutely right.
Comment #10
jenlamptonAre 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.
Comment #11
rmajed CreditAttribution: rmajed as a volunteer commentedpatch of comment #2 didnt apply for my branch and so thats a new patch for it
Comment #13
rmajed CreditAttribution: rmajed as a volunteer commentedextending 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
Comment #14
jenlamptonRe-roll of patch with code standards fix. Not sure why that test is failing. Testing again.
Comment #15
jenlamptonHm, that patch seemed to contain more than intended.... trying again.
Comment #16
jenlamptonI'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:
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.
Comment #17
aquaphenixRe-roll of patch #16 as it doesn't apply correctly with other patches.