Enhancement: reuse the current content regex #23

azaozz · 2020-03-08T23:18:24Z

Based on #20. This is another "patch" for #2 with some changes and enhancements.

Additions/changes compared to pull/20:

Add loading attribute to all img tags in the content, not just tags that have wp-image-#### class names.
Tweak/fix functions and vars names to make them more self-documenting.
Introduce wp_image_tag_add_srcset_and_sizes_attr() to go with wp_image_tag_add_loading_attr(). It makes it possible to bypass adding of srcset and sizes attributes on a per-image basis, before doing anything else. This also prepares it for adding filtering of iframe in the future (wp_iframe_tag_add_loading_attr(), etc.).
Removes the limitation from wp_filter_content_tags() to only add srcset and sizes when $context is set to the_content (i.e. only when run as callback to the_content filter). This makes the function useful for filtering any HTML string and adding all supported img tag attributes.

…nt images.

Some (small) optimizations and name tweaks. Return early if there's nothing to do, etc.

- Filter all found img tags. - Tweak/fix functions and vars names to make them more self-documenting. - Introduce wp_image_tag_add_srcset_and_sizes_attr()

azaozz · 2020-03-08T23:32:14Z

Still TODO:

Consider not adding the content filtering to the comment_text filter. Comments do not support images by default. If a plugin wants to enable images in comments, it can easily add that filter.

Add `add_filter( 'wp_image_tag_add_srcset_and_sizes_attr', '__return_false' );` to bypass adding of the attributes for now. Should probably revisit after merge to core.

…mg tags.

azaozz · 2020-03-16T00:47:29Z

In 060288b also removed passing the attachment_id to wp_image_tag_add_loading_attr(). Reasons:

It is not needed for the loading attr.
There is a really annoying mismatch of the found attachment IDs in post_content when the WP posts have been exported from one site and then imported in another (it is tested for/avoided when adding srcset and sizes). Passing the ID here as an optional param gives the wrong impression that it can always be relied on...

felixarntz

@azaozz This is looking great. I have a few follow-up comments. Let's discuss and address these, but afterwards I think this is good to be merged. At minimum, one unnecessary piece of code needs to be changed here :)

wp-lazy-loading.php

felixarntz · 2020-03-17T00:27:38Z

wp-lazy-loading.php

 	$add_loading_attr = wp_lazy_loading_enabled( 'img', $context );

-	if ( false === strpos( $content, '<img' ) || ( ! $add_srcset_sizes && ! $add_loading_attr ) ) {
+	if ( false === strpos( $content, '<img' ) ) {


I don't have a strong opinion here, but by removing the $add_srcset_sizes condition we actually change the behavior of where srcset and sizes are added (currently only supported for the_content). This is kind of an unintended side effect (this is only about loading), therefore I would rather have us keep the current behavior to only do that in the_content. Changing that is for another discussion IMO.

If we keep it, then having this condition can avoid unnecessary execution of the heavier logic below.

Yeah, went back and forth on this a few times. If we keep it, it makes wp_filter_content_tags() a bit harder to use for a "random" HTML string/content. It is possible to use wp_make_content_images_responsive() like that.

If we remove $add_srcset_sizes, by default this will run on the_content, the_excerpt, and widget_text_content (after removing comment_text). Thinking wp_make_content_images_responsive() should have been running on both the_excerpt, and widget_text_content from when it was added. It makes sense to add srcset, sizes, and loading attributes to images there.

...can avoid unnecessary execution of the heavier logic below.

It will (most likely) be running the preg_match_all() anyway. Then the difference would be preg_match( '/wp-image-([0-9]+)/i', $image, $class_id ) on the img tags which should be really fast as these would usually be few short strings.

I'm okay with this change (also supporting the other filters for srcset and sizes), but we need to clarify that when talking about the core patch (and if we keep it also in the dev note).
From a technical standpoint it makes sense, I just wonder whether it will result in unrelated discussion and overcomplicate these efforts. But we can change still change it back if undesirable per core patch discussion.

we need to clarify that when talking about the core patch
...
I just wonder whether it will result in unrelated discussion

Yeah, might happen. Thinking to separate this into another trac ticket. Just want to avoid hard-coding it there.

wp-lazy-loading.php

felixarntz · 2020-03-17T00:34:17Z

wp-lazy-loading.php

+		if ( $filtered_image !== $image ) {
+			$content = str_replace( $image, $filtered_image, $content );
+		}
+	}


Nice, I really like the single loop here used here.

felixarntz · 2020-03-17T00:36:53Z

wp-lazy-loading.php

+	 * @param string $context       Additional context about how the function was called or where the img tag is.
+	 * @param int    $attachment_id The image attachment ID.
+	 */
+	$add = apply_filters( 'wp_image_tag_add_srcset_and_sizes_attr', true, $image, $context, $attachment_id );


I'm not sure whether we should introduce a filter like this. I get the idea and am generally not opposed, but this is an unrelated change and would raise further discussion points that don't actually help what we want to achieve here.

I'd say let's simplify this function to only be the part within the below if clause.

Yeah, still on the fence about it too.

Having a filter somewhat matches wp_image_tag_add_loading_attr() (and eventually wp_iframe_tag_add_loading_attr()). It also lets plugin disable srcset and sizes depending on $context, which might be useful in some edge cases but not strictly necessary.

Not having a filter makes introducing wp_image_tag_add_srcset_and_sizes_attr() somewhat questionable as the same code may as well be inside the loop where that function is called...

Perhaps can keep it for now and revisit when making the core patch? Then there will likely be more people looking and helping :)

Actually revisiting this, we should probably have this filter if we make the other change from above (supporting more than the_content here). With this filter, we'll allow developers to opt out for other contexts than the_content (not sure anybody will wanna do that, but at least it allows someone to get back to the previous behavior if they have some random reason to do so).

felixarntz · 2020-03-17T00:41:46Z

@azaozz

Still TODO:

Consider not adding the content filtering to the comment_text filter. Comments do not support images by default. If a plugin wants to enable images in comments, it can easily add that filter.

That sounds good to me, let's remove it.

- Improve function names. - Remove left-over code. - Do not run on `comment_text` filter.

felixarntz and others added 9 commits February 4, 2020 22:18

Use a single regex for loading/srcset/sizes, and only modify attachme…

a418120

…nt images.

Add missing filter parameters.

66105ac

Fix too early return.

4977fbd

Fix tests.

0c1e7d9

Move checks for modifying attributes out of loops for performance.

8b545a4

Update wp-lazy-loading.php

ab4ffda

Some (small) optimizations and name tweaks. Return early if there's nothing to do, etc.

Few more name changes

051b5f1

Rename wp_filter_content_images to wp_filter_content_tags.

f02acc2

Update wp_filter_content_tags()

b545296

- Filter all found img tags. - Tweak/fix functions and vars names to make them more self-documenting. - Introduce wp_image_tag_add_srcset_and_sizes_attr()

Fix unit tests

e5e1717

Add `add_filter( 'wp_image_tag_add_srcset_and_sizes_attr', '__return_false' );` to bypass adding of the attributes for now. Should probably revisit after merge to core.

azaozz mentioned this pull request Mar 9, 2020

Use single regex for loading/srcset/sizes, and only modify attachment images #20

Closed

Remove redundant loop, simplify, check for changes before replacing i…

060288b

…mg tags.

felixarntz suggested changes Mar 17, 2020

View reviewed changes

azaozz added 2 commits March 17, 2020 22:54

Update wp-lazy-loading.php

cc21fa6

- Improve function names. - Remove left-over code. - Do not run on `comment_text` filter.

FIx tests.

40dcacb

felixarntz approved these changes Mar 18, 2020

View reviewed changes

felixarntz merged commit 9b32a7b into master Mar 18, 2020

felixarntz mentioned this pull request Mar 18, 2020

Optimize performance of regex for adding loading attribute #2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement: reuse the current content regex #23

Enhancement: reuse the current content regex #23

azaozz commented Mar 8, 2020 •

edited

Loading

azaozz commented Mar 8, 2020 •

edited

Loading

azaozz commented Mar 16, 2020

felixarntz left a comment •

edited

Loading

felixarntz Mar 17, 2020

azaozz Mar 18, 2020 •

edited

Loading

felixarntz Mar 18, 2020

azaozz Mar 18, 2020 •

edited

Loading

felixarntz Mar 17, 2020

felixarntz Mar 17, 2020

azaozz Mar 18, 2020 •

edited

Loading

felixarntz Mar 18, 2020

felixarntz commented Mar 17, 2020

Enhancement: reuse the current content regex #23

Enhancement: reuse the current content regex #23

Conversation

azaozz commented Mar 8, 2020 • edited Loading

azaozz commented Mar 8, 2020 • edited Loading

azaozz commented Mar 16, 2020

felixarntz left a comment • edited Loading

Choose a reason for hiding this comment

felixarntz Mar 17, 2020

Choose a reason for hiding this comment

azaozz Mar 18, 2020 • edited Loading

Choose a reason for hiding this comment

felixarntz Mar 18, 2020

Choose a reason for hiding this comment

azaozz Mar 18, 2020 • edited Loading

Choose a reason for hiding this comment

felixarntz Mar 17, 2020

Choose a reason for hiding this comment

felixarntz Mar 17, 2020

Choose a reason for hiding this comment

azaozz Mar 18, 2020 • edited Loading

Choose a reason for hiding this comment

felixarntz Mar 18, 2020

Choose a reason for hiding this comment

felixarntz commented Mar 17, 2020

azaozz commented Mar 8, 2020 •

edited

Loading

azaozz commented Mar 8, 2020 •

edited

Loading

felixarntz left a comment •

edited

Loading

azaozz Mar 18, 2020 •

edited

Loading

azaozz Mar 18, 2020 •

edited

Loading

azaozz Mar 18, 2020 •

edited

Loading