BytesCo / wpsmartcrop

GNU General Public License v2.0
21 stars 13 forks source link

data-* attributes being cut since 2.0.8 #16

Closed pierreberchtold closed 7 months ago

pierreberchtold commented 1 year ago

Since the update to 2.0.8 the data-* attributes on the image tags are being cut. Only the last part of the attribute name is kept. e.g. "data-src" -> "src" "data-multi-word-option" -> "option"

Changing the attribute pattern solves the problem

wp-smartcrop.php:669: old: (?P<name>\w+) new: (?P<name>[\w-]+)

Would it be possible to change that in the next version?

aaronsilber commented 1 year ago

@pierreberchtold Sorry to hear about the issue you're experiencing! Thanks for taking a look and suggesting a fix.

@samuel-reinhardt I don't see any reason to not allow - in attribute names in our regex, unless you have any concerns here?

samuel-reinhardt commented 1 year ago

@aaronsilber We may want to account for more than just "-". Perhaps [^\t\n\f\r />="'<] ?

Spec for attribute names (general and parser token state) https://html.spec.whatwg.org/multipage/syntax.html#attributes-2 https://html.spec.whatwg.org/#attribute-name-state

nikolowry commented 1 year ago

Could you please add a way to disable the_content hook? We've been using this plugin programmatically (only backend features, no frontend) for a few years and are starting to get issues reported by clients for two reasons:


I did figure out an ugly way to force remove your content hooks, but it be better if you could properly name the hook instead:

global $wp_filter;

// Remove WP_Smart_Crop the_content hooks
$smart_crop_content_hooks = array_filter(
    @$wp_filter['the_content']->callbacks ?: [],
    function ($v) {
        return preg_match(
            '/WP_Smart_Crop/',
            @print_r($v, true) ?: '');
    });

foreach ($smart_crop_content_hooks as $key => $value)
    unset($wp_filter['the_content']->callbacks[$key]);
nikolowry commented 1 year ago

Also curious as to why you're using RegEx instead of PHP's DomDocument for attribute parsing and manipulation?

I'm a big RegEx fan myself, but it's worth repeating the old adage of "don't use RegEx to parse XML/HTML" (A decent SO thread on topic https://stackoverflow.com/questions/6751105/why-its-not-possible-to-use-regex-to-parse-html-xml-a-formal-explanation-in-la)

MadtownLems commented 9 months ago

My guess is this issue is what's breaking 6.4's "Expand on Click" functionality: https://wordpress.org/support/topic/plugin-breaks-wordpress-6-4-1-lightbox/

MadtownLems commented 8 months ago

With the interactivity API landing in 6.5, I think this bug is about to break a lot more things. Let me know if I can help test a fix or anything. Thanks and cheers!

veerap000 commented 7 months ago

Same issue here with SmartCrop activated and trying use the lightbox with (WP core) image block.

Image should have attributes for interactivity API to work:

<img
data-wp-effect--setstylesonresize="effects.core.image.setStylesOnResize"
data-wp-effect="effects.core.image.setButtonStyles"
data-wp-init="effects.core.image.initOriginImage"
data-wp-on--click="actions.core.image.showLightbox"
data-wp-on--load="actions.core.image.handleLoad"
... other image stuff ...  />

But prefixes from attributes are removed by SmartCrop and it's breaking the lightbox:

<img
setstylesonresize="effects.core.image.setStylesOnResize"
effect="effects.core.image.setButtonStyles"
init="effects.core.image.initOriginImage"
click="actions.core.image.showLightbox"
load="actions.core.image.handleLoad"
... other image stuff ...  />
samuel-reinhardt commented 7 months ago

The issue with data-* (and other attribute names including interactivity api attributes) being mangled should be resolved with the 2.0.9 release, which is out now!

@nikolowry

Could you please add a way to disable the_content hook?

This is already be possible. You can use core's remove_filter and match the hook, callable and priority. The following code worked as expected for me in a default install, placed inside the functions.php of the TwentTwentyFour theme:

\remove_filter( 'the_content', array( \WP_Smart_Crop::Instance(), 'the_content' ), PHP_INT_MAX );

If you are calling from a plugin, you may want a wrapper function + hook to ensure this is called after our plugin class is made available.

Also curious as to why you're using RegEx instead of PHP's DomDocument for attribute parsing and manipulation?

I don't have firsthand knowledge of it, but I believe the original reasoning was that we're acting on unknown input, which could possibly be malformed HTML. As we move forward with this plugin, one of the changes we're considering is if this is still a concern, and if there are better ways to handle this than relying on regex.