Skip to content
This repository was archived by the owner on Dec 23, 2020. It is now read-only.

Enhancement: reuse the current content regex #23

Merged
merged 13 commits into from
Mar 18, 2020
Merged

Enhancement: reuse the current content regex #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a418120
Use a single regex for loading/srcset/sizes, and only modify attachme…
felixarntz Feb 4, 2020
66105ac
Add missing filter parameters.
felixarntz Feb 4, 2020
4977fbd
Fix too early return.
felixarntz Feb 4, 2020
0c1e7d9
Fix tests.
felixarntz Feb 4, 2020
8b545a4
Move checks for modifying attributes out of loops for performance.
felixarntz Feb 7, 2020
ab4ffda
Update wp-lazy-loading.php
azaozz Feb 10, 2020
051b5f1
Few more name changes
azaozz Feb 10, 2020
f02acc2
Rename wp_filter_content_images to wp_filter_content_tags.
felixarntz Feb 17, 2020
b545296
Update wp_filter_content_tags()
azaozz Mar 8, 2020
e5e1717
Fix unit tests
azaozz Mar 9, 2020
060288b
Remove redundant loop, simplify, check for changes before replacing i…
azaozz Mar 16, 2020
cc21fa6
Update wp-lazy-loading.php
azaozz Mar 18, 2020
40dcacb
FIx tests.
azaozz Mar 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions tests/phpunit/tests/media.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,12 @@ function test_wp_lazy_load_content_media() {
$content_unfiltered = sprintf( $content, $img, $img_xhtml, $img_html5, $iframe, $img_eager );
$content_filtered = sprintf( $content, $lazy_img, $lazy_img_xhtml, $lazy_img_html5, $iframe, $img_eager );

$this->assertSame( $content_filtered, wp_add_lazy_load_attributes( $content_unfiltered ) );
// Do not add srcset and sizes while testing.
add_filter( 'wp_img_tag_add_srcset_and_sizes_attr', '__return_false' );

$this->assertSame( $content_filtered, wp_filter_content_tags( $content_unfiltered ) );

remove_filter( 'wp_img_tag_add_srcset_and_sizes_attr', '__return_false' );
}

/**
Expand All @@ -70,11 +75,15 @@ function test_wp_lazy_load_content_media_opted_in() {
$content_unfiltered = sprintf( $content, $img );
$content_filtered = sprintf( $content, $lazy_img );

// Do not add srcset and sizes while testing.
add_filter( 'wp_img_tag_add_srcset_and_sizes_attr', '__return_false' );

// Enable globally for all tags.
add_filter( 'wp_lazy_loading_enabled', '__return_true' );

$this->assertSame( $content_filtered, wp_add_lazy_load_attributes( $content_unfiltered ) );
$this->assertSame( $content_filtered, wp_filter_content_tags( $content_unfiltered ) );
remove_filter( 'wp_lazy_loading_enabled', '__return_true' );
remove_filter( 'wp_img_tag_add_srcset_and_sizes_attr', '__return_false' );
}

/**
Expand All @@ -88,10 +97,14 @@ function test_wp_lazy_load_content_media_opted_out() {
%1$s';
$content = sprintf( $content, $img );

// Do not add srcset and sizes while testing.
add_filter( 'wp_img_tag_add_srcset_and_sizes_attr', '__return_false' );

// Disable globally for all tags.
add_filter( 'wp_lazy_loading_enabled', '__return_false' );

$this->assertSame( $content, wp_add_lazy_load_attributes( $content ) );
$this->assertSame( $content, wp_filter_content_tags( $content ) );
remove_filter( 'wp_lazy_loading_enabled', '__return_false' );
remove_filter( 'wp_img_tag_add_srcset_and_sizes_attr', '__return_false' );
}
}
173 changes: 134 additions & 39 deletions wp-lazy-loading.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,16 @@
*/
function _wp_lazy_loading_initialize_filters() {
// The following filters would be merged into core.
foreach ( array( 'the_content', 'the_excerpt', 'comment_text', 'widget_text_content' ) as $filter ) {
// After parsing blocks and shortcodes.
add_filter( $filter, 'wp_add_lazy_load_attributes', 25 );
foreach ( array( 'the_content', 'the_excerpt', 'widget_text_content' ) as $filter ) {
add_filter( $filter, 'wp_filter_content_tags' );
}

// The following filters are only needed while this is a feature plugin.
add_filter( 'wp_get_attachment_image_attributes', '_wp_lazy_loading_add_attribute_to_attachment_image' );
add_filter( 'get_avatar', '_wp_lazy_loading_add_attribute_to_avatar' );

// The following relevant filter from core should be removed when merged.
remove_filter( 'the_content', 'wp_make_content_images_responsive' );
}

add_action( 'plugins_loaded', '_wp_lazy_loading_initialize_filters', 1 );
Expand Down Expand Up @@ -102,7 +104,7 @@ function _wp_lazy_loading_add_attribute_to_attachment_image( $attr ) {
*/
function wp_lazy_loading_enabled( $tag_name, $context ) {
// By default add to all 'img' tags.
// See https://github.com/whatwg/html/issues/2806
// See https://html.spec.whatwg.org/multipage/embedded-content.html#attr-img-loading
$default = ( 'img' === $tag_name );

/**
Expand All @@ -118,57 +120,150 @@ function wp_lazy_loading_enabled( $tag_name, $context ) {
}

/**
* Add `loading="lazy"` to `img` HTML tags.
* Filters specific tags in post content and modifies their markup.
*
* Currently the "loading" attribute is only supported for `img`, and is enabled by default.
* This function adds `srcset`, `sizes`, and `loading` attributes to `img` HTML tags.
*
* @since (TBD)
*
* @see wp_img_tag_add_loading_attr()
* @see wp_img_tag_add_srcset_and_sizes_attr()
*
* @param string $content The HTML content to be filtered.
* @param string $context Optional. Additional context to pass to the filters. Defaults to `current_filter()` when not set.
* @return string Converted content with 'loading' attributes added to images.
* @return string Converted content with images modified.
*/
function wp_add_lazy_load_attributes( $content, $context = null ) {
function wp_filter_content_tags( $content, $context = null ) {
if ( null === $context ) {
$context = current_filter();
}

if ( ! wp_lazy_loading_enabled( 'img', $context ) ) {
$add_loading_attr = wp_lazy_loading_enabled( 'img', $context );

if ( false === strpos( $content, '<img' ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@azaozz azaozz Mar 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@azaozz azaozz Mar 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return $content;
}

return preg_replace_callback(
'/<img\s[^>]+>/',
function( array $matches ) use( $content, $context ) {
if ( ! preg_match( '/\sloading\s*=/', $matches[0] ) ) {
$tag_html = $matches[0];

/**
* Filters the `loading` attribute value. Default `lazy`.
*
* Returning `false` or an empty string will not add the attribute.
* Returning `true` will add the default value.
*
* @since (TBD)
*
* @param string $default The filtered value, defaults to `lazy`.
* @param string $tag_html The tag's HTML.
* @param string $content The HTML containing the image tag.
* @param string $context Optional. Additional context. Defaults to `current_filter()`.
*/
$value = apply_filters( 'wp_set_image_loading_attr', 'lazy', $tag_html, $content, $context );
if ( ! preg_match_all( '/<img\s[^>]+>/', $content, $matches ) ) {
return $content;
}

// List of the unique `img` tags found in $content.
$images = array();

if ( $value ) {
if ( ! in_array( $value, array( 'lazy', 'eager' ), true ) ) {
$value = 'lazy';
}
foreach ( $matches[0] as $image ) {
if ( preg_match( '/wp-image-([0-9]+)/i', $image, $class_id ) ) {
$attachment_id = absint( $class_id[1] );

return str_replace( '<img', '<img loading="' . $value . '"', $tag_html );
}
if ( $attachment_id ) {
/*
* If exactly the same image tag is used more than once, overwrite it.
* All identical tags will be replaced later with 'str_replace()'.
*/
$images[ $image ] = $attachment_id;
continue;
}
}

$images[ $image ] = 0;
}

// Reduce the array to unique attachment IDs.
$attachment_ids = array_unique( array_filter( array_values( $images ) ) );

if ( count( $attachment_ids ) > 1 ) {
/*
* Warm the object cache with post and meta information for all found
* images to avoid making individual database calls.
*/
_prime_post_caches( $attachment_ids, false, true );
}

foreach ( $images as $image => $attachment_id ) {
$filtered_image = $image;

// Add 'srcset' and 'sizes' attributes if applicable.
if ( $attachment_id > 0 && false === strpos( $filtered_image, ' srcset=' ) ) {
$filtered_image = wp_img_tag_add_srcset_and_sizes_attr( $filtered_image, $context, $attachment_id );
}

// Add 'loading' attribute if applicable.
if ( $add_loading_attr && false === strpos( $filtered_image, ' loading=' ) ) {
$filtered_image = wp_img_tag_add_loading_attr( $filtered_image, $context );
}

if ( $filtered_image !== $image ) {
$content = str_replace( $image, $filtered_image, $content );
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return $content;
}

/**
* Adds `loading` attribute to an existing `img` HTML tag.
*
* @since (TBD)
*
* @param string $image The HTML `img` tag where the attribute should be added.
* @param string $context Additional context to pass to the filters.
* @return string Converted `img` tag with `loading` attribute added.
*/
function wp_img_tag_add_loading_attr( $image, $context ) {
/**
* Filters the `loading` attribute value. Default `lazy`.
*
* Returning `false` or an empty string will not add the attribute.
* Returning `true` will add the default value.
*
* @since (TBD)
*
* @param string $value The 'loading' attribute value, defaults to `lazy`.
* @param string $image The HTML 'img' element to be filtered.
* @param string $context Additional context about how the function was called or where the img tag is.
*/
$value = apply_filters( 'wp_img_tag_add_loading_attr', 'lazy', $image, $context );

if ( $value ) {
if ( ! in_array( $value, array( 'lazy', 'eager' ), true ) ) {
$value = 'lazy';
}

return str_replace( '<img', '<img loading="' . $value . '"', $image );
}

return $image;
}

/**
* Adds `srcset` and `sizes` attributes to an existing `img` HTML tag.
*
* @since (TBD)
*
* @param string $image The HTML `img` tag where the attribute should be added.
* @param string $context Additional context to pass to the filters.
* @param int $attachment_id Image attachment ID.
* @return string Converted 'img' element with 'loading' attribute added.
*/
function wp_img_tag_add_srcset_and_sizes_attr( $image, $context, $attachment_id ) {
/**
* Filters whether to add the `srcset` and `sizes` HTML attributes to the img tag. Default `true`.
*
* Returning anything else than `true` will not add the attributes.
*
* @since (TBD)
*
* @param bool $value The filtered value, defaults to `true`.
* @param string $image The HTML `img` tag where the attribute should be added.
* @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_img_tag_add_srcset_and_sizes_attr', true, $image, $context, $attachment_id );

if ( true === $add ) {
$image_meta = wp_get_attachment_metadata( $attachment_id );
return wp_image_add_srcset_and_sizes( $image, $image_meta, $attachment_id );
}

return $matches[0];
},
$content
);
return $image;
}