WordPress.org

Make WordPress Core

Opened 8 weeks ago

Last modified 2 days ago

#51865 assigned enhancement

Add filter to `wp_image_src_get_dimensions`

Reported by: joemcgill Owned by: joemcgill
Milestone: 5.7 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch 2nd-opinion needs-unit-tests needs-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 (11)

#1 @joemcgill
8 weeks ago

  • Component changed from General to Media

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


8 weeks ago

  • 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
8 weeks ago

  • Keywords 2nd-opinion added

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


7 weeks ago

#5 @Mista-Flo
7 weeks 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
7 weeks 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
7 weeks 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
7 weeks 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 days ago

#10 @joemcgill
4 days 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
2 days 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.

Note: See TracTickets for help on using tickets.