Opened 9 years ago
Closed 9 years ago
#35045 closed defect (bug) (fixed)
Responsive images not added when effective scheme differs from image src scheme
Reported by: | webaware | Owned by: | johnbillion |
---|---|---|---|
Milestone: | 4.4.1 | Priority: | normal |
Severity: | normal | Version: | 4.4 |
Component: | Media | Keywords: | has-patch has-unit-tests https fixed-major |
Focuses: | template | Cc: |
Description
wp_image_add_srcset_and_sizes()
checks for base URL matches between available files and the src URL from the image tag in content. When the protocol scheme is different, the base URL doesn't match the src URL and no responsive images are added.
This can occur in such circumstances as:
- image added with protocol
http:
and page viewed with protocolhttps:
- image added with protocol
https:
and page viewed with protocolhttp:
- image added with no protocol (i.e. protocol-relative URL) and page viewed with either protocol
http:
orhttps:
upload_dir
filter changes baseurl tohttps:
to prevent insecure content
Attachments (12)
Change History (41)
#2
@
9 years ago
- Keywords needs-unit-tests needs-testing added
- Milestone changed from Awaiting Review to 4.4.1
- Version changed from trunk to 4.4
Thanks for the patch @webaware. Some unit tests for this would be great.
#3
@
9 years ago
@johnbillion will see what I can do. Was hoping for some non-keyboard time today :) (Sunday)
#6
@
9 years ago
@jaspermdegroot related, but not the same. #34945 is more of a problem with upload_dir
and while related, is different to this issue which is really all about matching available responsive images to an image from the HTML tag in content.
#8
@
9 years ago
image_base_url.diff wouldn't work properly. This shouldn't be changed:
- $all_sizes[] = $base_url . $image_meta['file']; + $all_sizes[] = $image_base_url . $image_meta['file'];
as $image_meta['file']
already contains the year/month sub-dirs. If we want to keep it, the first block should be moved right under $base_url = trailingslashit( _wp_upload_dir_baseurl() );
and change $base_url.
On the other hand we only use these for comparison. Would be better to just strip https?:
from both.
#9
@
9 years ago
In 35045.patch:
- Remove the protocols from
$image_src
and$base_url
before comparing them to be able to match http, https, and protocol relative URLs. - Add the unit tests to the same patch.
#10
follow-up:
↓ 12
@
9 years ago
This bug was introduced in r35820 because we build a full URL for each size and then compare them against the src
of the image. Instead, we could just compare filenames, which would avoid trying to match schemes in the first place.
35045.diff changes the logic introduced in r35820 to compare file names instead of full URLs.
#12
in reply to:
↑ 10
@
9 years ago
Replying to joemcgill:
Instead, we could just compare filenames...
Don't forget that you can have the same file name in different year/month sub-directories. That seems to happen regularly when uploading directly from some cameras because of their auto-naming schemes.
#13
@
9 years ago
It seems like it would be fairly rare when an ID and a filename match, but result in a false positive. Event so, it's a good point so we can add the rest of the path as well, just to be safer.
#14
follow-up:
↓ 16
@
9 years ago
35045.2.diff prepends the upload path from the full size image in the metadata to all the intermediate sizes before testing to see if one of the image sizes matches the path of the image src
before adding srcset
and sizes
attributes.
#16
in reply to:
↑ 14
@
9 years ago
Replying to joemcgill:
I'm a bit "uneasy" to only match the year/month/filename.ext
part of the image src. Not matching the whole URL might bring other edge cases.
On the other hand that is the simplest way to support CDN plugins that replace all image src in post_content, so probably better to use that for now.
In 35045.3.diff: optimize/refine 35045.2.diff a bit. We can check if $image_meta['file']
is contained in the image src instead of adding it to the all_sizes array and then looking for it.
#18
@
9 years ago
Was also thinking we can move the "URL exists in image meta" check to wp_calculate_image_srcset() as @joemcgill mentions here. It will introduce very little overhead when we are certain the attachment ID matches the image (for galleries, etc.).
As far as I see we need to check if the image src matches one of the srcset URLs after the 'wp_calculate_image_srcset'
filter at the end of wp_calculate_image_srcset(). Are there any legitimate use cases where this won't happen?
#19
@
9 years ago
Was also thinking we can move the "URL exists in image meta" check to wp_calculate_image_srcset()
I was thinking about this again earlier today while discussion #35102. Moving this logic to wp_calculate_image_srcset()
streamlines the code a little bit and has the added benefit allowing the filter to still be reachable by plugin authors who have rewritten image URLs for whatever reason (e.g., CDNs, etc.).
35045.4.diff is a first pass at what this would look like if we moved the logic from 35045.2.diff (et al.) into wp_calculate_image_srcset()
.
#20
@
9 years ago
In 35045.5.diff:
- Move the
false !== strpos( $image_src, $dirname . $image['file'] )
at the beginning of the loop. Otherwise it won't match when the image src is for a size wider than$max_srcset_image_width
. - Simplify/cleanup a bit.
#21
follow-up:
↓ 23
@
9 years ago
In 35045.6.diff:
Added filters for $dirname
and $image_baseurl
. Plugins like ours may store the files in a different location than on the server. For example, WP Offload S3 has an option for object versioning to bust CDN caches so images would go from the server as 2015/12/image.jpg
to S3 as 2015/12/15154454/image.jpg
.
Without this filter, the check of $dirname . $image['file']
will still fail.
Adding the filter on $image_baseurl
has the extra benefit that we can rewrite the URL for the image size files instead of doing it later on the 'wp_calculate_image_srcset' hook.
#23
in reply to:
↑ 21
@
9 years ago
Replying to polevaultweb:
Added filters for
$dirname
and$image_baseurl
.
Don't think we should be adding filters there. That will limit the update possibilities by "locking" us in back-compat. The wp_calculate_image_srcset
filter is powerful enough. If a plugin moves all images and doesn't implement the same directory structure, it seems it should be overriding/replacing wp_image_add_srcset_and_sizes() and/or wp_calculate_image_srcset().
What we can try doing is check (again?) after the wp_calculate_image_srcset
filter if the image src matches one of the srcset URLs. That will make it possible to set the srcset for all cases.
This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.
9 years ago
#25
@
9 years ago
In 35045.7.diff: add wp_calculate_image_srcset_dirname_match
filter for the sake of plugins that add external image sources but do not maintain the year/month sub-dirs. This allows them to easily manipulate the image path before we check for a match.
This patch also fixes #35102.
#26
@
9 years ago
- Keywords needs-testing removed
In 35045.8.diff: same as 35045.7.diff but instead of filtering only the year/month bit it lets plugins fix all inconsistencies in the attachment meta by introducing wp_calculate_image_srcset_meta
filter. This can be used to fix/normalize all sorts of changes made by plugins over the years.
patch to fix