#51865 closed enhancement (fixed)
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 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)
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 ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
4 years ago
#5
@
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
@
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:
↓ 8
@
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
@
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
@
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
@
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
@
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
@
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.
joemcgill commented on PR #763:
4 years ago
#18
#20
@
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
#22
@
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
@
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.
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