Make WordPress Core

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's profile webaware Owned by: johnbillion's profile 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 protocol https:
  • image added with protocol https: and page viewed with protocol http:
  • image added with no protocol (i.e. protocol-relative URL) and page viewed with either protocol http: or https:
  • upload_dir filter changes baseurl to https: to prevent insecure content

Attachments (12)

image_base_url.diff (1.0 KB) - added by webaware 9 years ago.
patch to fix
35045-unit-test.diff (2.0 KB) - added by webaware 9 years ago.
Unit test
35045.patch (2.9 KB) - added by azaozz 9 years ago.
35045.diff (3.3 KB) - added by joemcgill 9 years ago.
35045.2.diff (3.5 KB) - added by joemcgill 9 years ago.
35045.3.diff (3.9 KB) - added by azaozz 9 years ago.
35045.3.2.diff (3.9 KB) - added by azaozz 9 years ago.
35045.4.diff (5.2 KB) - added by joemcgill 9 years ago.
Move src match to wp_calculate_image_srcset()
35045.5.diff (5.2 KB) - added by azaozz 9 years ago.
35045.6.diff (6.1 KB) - added by polevaultweb 9 years ago.
35045.7.diff (5.8 KB) - added by azaozz 9 years ago.
35045.8.diff (6.1 KB) - added by azaozz 9 years ago.

Download all attachments as: .zip

Change History (41)

@webaware
9 years ago

patch to fix

#1 @webaware
9 years ago

  • Keywords has-patch added

#2 @johnbillion
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 @webaware
9 years ago

@johnbillion will see what I can do. Was hoping for some non-keyboard time today :) (Sunday)

#4 @jaspermdegroot
9 years ago

This seems to be a duplicate of #34945

@webaware
9 years ago

Unit test

#5 @webaware
9 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#6 @webaware
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.

#7 @johnbillion
9 years ago

  • Owner set to johnbillion
  • Status changed from new to reviewing

#8 @azaozz
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.

Last edited 9 years ago by azaozz (previous) (diff)

@azaozz
9 years ago

#9 @azaozz
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.
Last edited 9 years ago by azaozz (previous) (diff)

@joemcgill
9 years ago

#10 follow-up: @joemcgill
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.

#11 @webaware
9 years ago

@joemcgill sounds good, makes sense.

#12 in reply to: ↑ 10 @azaozz
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.

Last edited 9 years ago by azaozz (previous) (diff)

#13 @joemcgill
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.

@joemcgill
9 years ago

#14 follow-up: @joemcgill
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.

@azaozz
9 years ago

#16 in reply to: ↑ 14 @azaozz
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.

@azaozz
9 years ago

#17 @azaozz
9 years ago

In 35045.3.2.diff: A little more cleanup.

#18 @azaozz
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?

@joemcgill
9 years ago

Move src match to wp_calculate_image_srcset()

#19 @joemcgill
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().

Last edited 9 years ago by joemcgill (previous) (diff)

@azaozz
9 years ago

#20 @azaozz
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.

@polevaultweb
9 years ago

#21 follow-up: @polevaultweb
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.

#22 @johnbillion
9 years ago

  • Keywords https added

#23 in reply to: ↑ 21 @azaozz
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

@azaozz
9 years ago

#25 @azaozz
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.

@azaozz
9 years ago

#26 @azaozz
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.

#27 @azaozz
9 years ago

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

In 36121:

Responsive images: fix the check whether the attachment meta matches the image src to work with http/https and CDNs.

Props webaware, joemcgill, azaozz.
Fixes #35045 and #35102 for trunk.

#28 @azaozz
9 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for 4.4.

#29 @dd32
9 years ago

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

In 36152:

Responsive images: fix the check whether the attachment meta matches the image src to work with http/https and CDNs.

Merges [36121] to the 4.4 branch.
Props webaware, joemcgill, azaozz.
Fixes #35045 and #35102.

Note: See TracTickets for help on using tickets.