Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#51865 closed enhancement (fixed)

Add filter to `wp_image_src_get_dimensions`

Reported by: joemcgill's profile joemcgill Owned by: joemcgill's profile joemcgill
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch needs-unit-tests has-dev-note
Focuses: Cc:

Description

wp_image_src_get_dimensions() was added in WordPress 5.5 as a way to calculate the height and width attributes of a content image so that WordPress could better support native lazy loading and improve calculation of the responsive sizes attribute since the block editor often doesn't add these attributes to the image markup saved in the editor.

This function works by comparing the image file from the src attribute against all of the image sizes available for that attachment, in order to find the dimensions. However, if a plugin has modified either the src file or the attachment meta, this might return a false, resulting in the full size dimensions to be used when calculating height, width and sizes attributes.

Adding a filter to wp_image_src_get_dimensions() would allow developers to fix these dimensions when they occur.

Additionally, the check for wp_image_file_matches_image_meta() in wp_image_src_get_dimensions() can most likely be dropped, since it's purpose is largely duplicative of the matching of files to attachment meta that is done afterwards.

Change History (27)

#1 @joemcgill
4 years ago

  • Component changed from General to Media

This ticket was mentioned in PR #763 on WordPress/wordpress-develop by joemcgill.


4 years ago
#2

  • Keywords has-patch added; needs-patch removed

This adds a filter to the output of wp_image_src_get_dimensions() so that the dimensions can be adjusted/corrected if the automatic size guessing based on attachment metadata fails.

Additionally, this removes an (I think) unnecessary call to wp_image_file_matches_image_meta() which is redundant if its only purpose in this function is to reduce unnecessary loops through the sizes array since it does so itself internally.

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

#3 @joemcgill
4 years ago

  • Keywords 2nd-opinion added

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


4 years ago

#5 @Mista-Flo
4 years ago

  • Keywords needs-unit-tests added

Just looked at the PR, code looks nice, implementation seem okay, it would be nice to handle different scenarios with a unit test I think.

#6 @ianmjones
4 years ago

This is a great change.

We found that since WP5.5 srcset was not being added for images offloaded by WP Offload Media if they included "Object Versioning". That's a small segment of numerics added just before the filename to help with CDN cache busting in various scenarios.

2020/11/puppies.jpg becomes 2020/11/12131415/puppies.jpg.

We were able to filter wp_image_file_matches_image_meta so that passed, and that means intermediate sizes in the src now get a proper srcset attribute on the img tag.

However, in wp_image_src_get_dimensions there's a check that the full size image's "file" entry in the metadata is found in the image src, that still fails and we can't work around it.

The proposed change here would help us also fix the problem for the full size images as we'd be able to check our metadata for a match and supply the size info.

However, I wonder if it would be possible to tweak the function a fraction and so that ...

if ( strpos( $image_src, $image_meta['file'] ) !== false ) {

... becomes ...

if ( strpos( $image_src, wp_basename( $image_meta['file'] ) ) !== false ) {

That would improve the chances of matching greatly, and reduce the need for plugins like WP Offload Media to have to do the work of finding a match.

#7 follow-up: @joemcgill
4 years ago

@ianmjones I've updated the patch to include the wp_basename() change, which makes sense to me. I suspect that this wouldn't cause false positives at this point since we've already matched the attachment ID.

#8 in reply to: ↑ 7 @ianmjones
4 years ago

Replying to joemcgill:

@ianmjones I've updated the patch to include the wp_basename() change, which makes sense to me. I suspect that this wouldn't cause false positives at this point since we've already matched the attachment ID.

Excellent, thank you! 🎉️

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 years ago

#10 @joemcgill
4 years ago

@SergeyBiryukov noted in this Slack comment that the wp_basename() changes should be handled in a separate commit from the filter addition, which makes sense to me. I'll update the patch so these can be handled separately.

#11 @hellofromTonya
4 years ago

  • Keywords needs-dev-note added

❓ Does this ticket still require 2nd-opinion?

Adding needs-dev-note to include in Misc Dev Note, as this patch adds a new filter.

Adding notes from Core Scrub:

Per Sergey:

It seems like the wp_basename() change should probably be added separately from the filter and get its own tests.

I think new filters don't generally require unit tests, but if the function doesn't have any, might be a good idea to add some while we're at it

Per Joe:

This makes sense to me. I'll review and open a separate ticket for the wp_basename() change.

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


4 years ago

#15 @antpb
4 years ago

  • Milestone changed from 5.7 to 5.8

With 5.7 Beta 1 approaching today we need to move this enhancement ticket out of the 5.7 milestone. I am marking this 5.8 as it looks like it's really close. Just needs the patch split out and a new ticket for the filter.

#16 @joemcgill
4 years ago

  • Keywords 2nd-opinion removed
  • Milestone changed from 5.8 to 5.7

Dropping this back in for a late commit to 5.7, after reverting the wp_basename() change. Will handle that in a follow-up ticket.

#17 @joemcgill
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 50134:

Media: Add filter to wp_image_src_get_dimensions.

This adds a new filter, wp_image_src_get_dimensions to the wp_image_src_get_dimensions() function to correct the dimensions returned for a file whenever WordPress isn't able to correctly get the dimensions from attachment metadata.

Fixes #51865.

#19 @joemcgill
4 years ago

Follow up for the basename issue is now tracked in #52417.

#20 @dd32
4 years ago

@joemcgill looks this is causing an increased number of PHP Notices on WordPress.org like the following:

Source: GET https://wordpress.org/support/article/settings-writing-screen/

E_NOTICE: Trying to access array offset on value of type bool in wp-includes/media.php:1605

It looks like the removed wp_image_file_matches_image_meta() was protecting against a fasley $image_meta value.

The callstack is: the_content, apply_filters('the_content'), WP_Hook->apply_filters, wp_filter_content_tags, wp_img_tag_add_width_and_height_attr, wp_image_src_get_dimensions

#21 @joemcgill
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#22 @joemcgill
4 years ago

@dd32 thanks for the heads up. Looking at this now to see if it can be patched without doing the redundant loops through all the sizes, since that seemed unnecessary.

This ticket was mentioned in PR #973 on WordPress/wordpress-develop by joemcgill.


4 years ago
#23

This fixes a potential illegal offset error introduced in [50134] if the
$image_meta doesn't include a file key.

Props dd32.
Fixes https://core.trac.wordpress.org/ticket/51865.

#24 @dd32
4 years ago

Just noting, that I dug into this a tad more, and it was caused by a <img ... class=“wp-image-3241"...> where post ID 3241 was not a valid attachment, but was copy-pasted or potentially imported from another WordPress site.

#25 @joemcgill
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 50136:

Media: Sanity check image meta in 'wp_image_src_get_dimensions'.

This fixes a potential illegal offset error introduced in [50134] if the $image_meta doesn't include a file key.

Props dd32.
Fixes #51865.

Note: See TracTickets for help on using tickets.