Don't apply auto-sizes if there is no width and add filter for disabling #7812

joemcgill · 2024-11-15T17:10:35Z

While investigating Trac:62413, it occurred to me that wp_img_tag_add_auto_sizes() doesn't explicitly confirm that a width attribute is present before applying auto-sizes. This PR is mostly a hardening measure to ensure this can't occur.

Trac ticket: https://core.trac.wordpress.org/ticket/62413

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

github-actions · 2024-11-15T17:23:14Z

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

The Plugin and Theme Directories cannot be accessed within Playground.
All changes will be lost when closing a tab with a Playground instance.
All changes will be lost when refreshing the page.
A fresh instance is created each time the link below is clicked.
Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

felixarntz

@joemcgill Does this actually help? WordPress should never add loading="lazy" in the first place if an image doesn't have both width and height attributes.

joemcgill · 2024-11-15T18:36:34Z

@felixarntz:

WordPress should never add loading="lazy" in the first place if an image doesn't have both width and height attributes.

That's true, but our tests and the wp_img_tag_add_auto_sizes() function itself doesn't ensure that the img has a width attribute before adding sizes="auto"

github-actions · 2024-11-15T18:38:46Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props joemcgill, flixos90.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

src/wp-includes/media.php

joemcgill · 2024-11-15T20:33:43Z

src/wp-includes/media.php

+	 * @since 6.7.1
+	 *
+	 * @param boolean $enabled Whether auto-sizes for lazy loaded images is enabled.
+	 * @param string  $image   The image tag markup being modified.


Passing image markup as context allows this to be filtered based on the specific image, which is much more precise.

We may want to align this with the way the lazy-loading filters work. There is one general "feature enabled" filter, and another one for "whether to add it for this image".

The filter here would be more about whether to add it for the particular image. To be fair, that's enough on its own, if we don't want to introduce a separate generic "is the sizes=auto feature enabled?" filter. But I'd prefer that we then name the filter consistently with the equivalent lazy-loading filter, e.g. wp_img_tag_add_auto_sizes. That would also align with the function name.

Renamed for consistency in bbaed9c.

See above comment regarding whether or not we should pass image markup (for now). In the future we definitely should, though I'm not sure how crucial that is for a 6.7.1 hot fix filter.

joemcgill · 2024-11-15T20:35:05Z

src/wp-includes/media.php

+	if ( ! is_string( $loading ) || 'lazy' !== strtolower( trim( $loading, " \t\f\r\n" ) ) ) {
+		return $image;
+	}
+
+	// Bail early if the image doesn't have a width attribute.
+	$width = $processor->get_attribute( 'width' );
+	if ( ! is_string( $width ) ) {


Could consolidate these two if statements, but left it for readability.

This reverts commit 3cfcbe2.

joemcgill

Made a couple small changes here.

First, I reverted the idea I was exploring in f07f4d1, which would consolidate all of the auto-sizes logic in one place, rather than having separate implementations in wp_get_attachment_image and wp_img_tag_add_auto_sizes.

I've also updated the filter name to align with other similar filters we have in place.

src/wp-includes/media.php

joemcgill · 2024-11-15T22:31:01Z

src/wp-includes/media.php

+	 * @since 6.7.1
+	 *
+	 * @param boolean $enabled Whether auto-sizes for lazy loaded images is enabled.
+	 * @param string  $image   The image tag markup being modified.


Renamed for consistency in bbaed9c.

felixarntz

This looks quite solid, one more point of feedback.

felixarntz · 2024-11-15T22:47:22Z

src/wp-includes/media.php

@@ -1137,8 +1137,12 @@ function wp_get_attachment_image( $attachment_id, $size = 'thumbnail', $icon = f
 			}
 		}

+		/** This filter is documented in wp-includes/media.php */
+		$add_auto_sizes = apply_filters( 'wp_img_tag_add_auto_sizes', true, '' );


It's not great to pass an empty string here. Obviously we don't have a full image tag available here, but that means the filter would be cumbersome to use if developers can't rely on there being an HTML string in this parameter.

I could see two alternatives here for the short-term:

We could for now make this a simple on/off filter, without any context, both here and in wp_img_tag_add_auto_sizes(). This would be similar to how wp_lazy_loading_enabled works. Given the main reason for the filter for now is to "emergency turn off" the feature in case of a problem, I think that's a reasonable short-term solution. We could still explore granular filtering options per image in the 6.8 cycle without being in a rush.

Or we don't use this filter here, but keep it in wp_img_tag_add_auto_sizes(), but then in both places apply a context-less filter as a general on/off switch. This would be similar to how lazy-loading filters work: There is wp_lazy_loading_enabled as on/off switch, and wp_img_tag_add_loading_attr, which is intended for a specific content image.

I think the first option would be simpler, sufficient for what's needed as a hot fix, and more future-proof, so I'd prefer that. We could then expand the filtering for 6.8.

I noticed that too. I'd opt for removing the image context entirely if we're not consolidating this logic all in the wp_img_tag_add_auto_sizes() function and consider adding it as an enhancement.

Removed in 214f91d.

I'll make one last case for why I think we should reconsider consolidating this logic to wp_img_tag_add_auto_sizes() as was experimented in 3cfcbe2:

The consolidation allows the image context to be consistently passed to the filter, making it more useful.

It makes the filter name, wp_img_tag_add_auto_sizes make more sense, because it matches the name of the function responsible for the logic.

It avoids edge cases where changes to the markup from wp_get_attachment_image_attributes or wp_get_attachment_image are not considered before auto-sizes gets applied.

The main downside that I see for not consolidating this logic in one place is that it means adding the need to parse the finalized HTML again to inspect the attributes. I'm not convinced this alone outweighs the benefits of consolidation.

I think there are ways to consolidate the logic without the downside of parsing the HTML again, but this is something that's better done as part of a 6.8 enhancement because then we're not in a rush or constrained by making as few changes as possible.

felixarntz · 2024-11-15T22:48:26Z

src/wp-includes/media.php

+	 * @since 6.7.1
+	 *
+	 * @param boolean $enabled Whether auto-sizes for lazy loaded images is enabled.
+	 * @param string  $image   The image tag markup being modified.


See above comment regarding whether or not we should pass image markup (for now). In the future we definitely should, though I'm not sure how crucial that is for a 6.7.1 hot fix filter.

azaozz · 2024-11-15T23:40:41Z

src/wp-includes/media.php

+
+	// Bail early if the image doesn't have a width attribute.
+	$width = $processor->get_attribute( 'width' );
+	if ( ! is_string( $width ) ) {


Is it possible for $width to be an empty string here? Perhaps empty( $width ) || ! is_string( $width ) may be better.

Fixed in baee465

felixarntz · 2024-11-18T18:03:50Z

As coordinated with @joemcgill via DM, this PR is now included in #7813, as they address the same problem.

Don't apply auto-sizes if there is no width

80720a2

joemcgill added 2 commits November 15, 2024 11:38

Fix test

c1a21a0

Revert change to wp_get_attachment_image

4d4b5aa

felixarntz reviewed Nov 15, 2024

View reviewed changes

joemcgill added 2 commits November 15, 2024 12:10

Add test for when wp_get_attachment_image doesn't produce a width

f3cf359

Improve PHPUnit test

05591bb

joemcgill marked this pull request as ready for review November 15, 2024 18:38

joemcgill added 2 commits November 15, 2024 14:13

Move all processing to wp_img_tag_add_auto_sizes

3cfcbe2

Add filter for enabling auto-sizes functionality

e45b275

joemcgill changed the title ~~Don't apply auto-sizes if there is no width~~ Don't apply auto-sizes if there is no width and add filter for disabling Nov 15, 2024

joemcgill commented Nov 15, 2024

View reviewed changes

Revert "Move all processing to wp_img_tag_add_auto_sizes"

f07f4d1

This reverts commit 3cfcbe2.

joemcgill mentioned this pull request Nov 15, 2024

Print CSS rule to fix potential visual issues with auto-sizes #7813

Closed

Rename filter and apply in wp_get_attachment_image

bbaed9c

joemcgill commented Nov 15, 2024

View reviewed changes

felixarntz reviewed Nov 15, 2024

View reviewed changes

Remove image parameter from the filter

214f91d

azaozz reviewed Nov 15, 2024

View reviewed changes

felixarntz added a commit to felixarntz/wordpress-develop that referenced this pull request Nov 18, 2024

Include changes from WordPress#7812.

1a147f1

felixarntz added a commit to felixarntz/wordpress-develop that referenced this pull request Nov 18, 2024

Add second filter call from WordPress#7812.

3420366

felixarntz closed this Nov 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't apply auto-sizes if there is no width and add filter for disabling #7812

Don't apply auto-sizes if there is no width and add filter for disabling #7812

joemcgill commented Nov 15, 2024 •

edited

Loading

github-actions bot commented Nov 15, 2024

felixarntz left a comment

joemcgill commented Nov 15, 2024

github-actions bot commented Nov 15, 2024

joemcgill Nov 15, 2024

felixarntz Nov 15, 2024

joemcgill Nov 15, 2024

felixarntz Nov 15, 2024

joemcgill Nov 15, 2024

joemcgill left a comment

joemcgill Nov 15, 2024

felixarntz left a comment

felixarntz Nov 15, 2024

joemcgill Nov 15, 2024

joemcgill Nov 15, 2024

felixarntz Nov 15, 2024 •

edited

Loading

felixarntz Nov 15, 2024

azaozz Nov 15, 2024

felixarntz Nov 18, 2024

felixarntz commented Nov 18, 2024

Don't apply auto-sizes if there is no width and add filter for disabling #7812

Don't apply auto-sizes if there is no width and add filter for disabling #7812

Conversation

joemcgill commented Nov 15, 2024 • edited Loading

github-actions bot commented Nov 15, 2024

Test using WordPress Playground

Some things to be aware of

felixarntz left a comment

Choose a reason for hiding this comment

joemcgill commented Nov 15, 2024

github-actions bot commented Nov 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz commented Nov 18, 2024

joemcgill commented Nov 15, 2024 •

edited

Loading

felixarntz Nov 15, 2024 •

edited

Loading