Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#35102 closed defect (bug) (fixed)

Responsive images support for external URLs

Reported by: polevaultweb's profile polevaultweb Owned by: joemcgill's profile joemcgill
Milestone: 4.4.1 Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords: has-patch fixed-major
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Since the introduction of [35821] in #34898, the ability for srcsets to be added to images with external src URLs has been stopped. For example, our plugin https://wordpress.org/plugins/amazon-s3-and-cloudfront/ replaces local URLs with S3 ones in post content, currently srcset will not be added.

Attachments (1)

35102.diff (652 bytes) - added by polevaultweb 9 years ago.

Download all attachments as: .zip

Change History (14)

@polevaultweb
9 years ago

#1 @SergeyBiryukov
9 years ago

  • Description modified (diff)

#2 @SergeyBiryukov
9 years ago

  • Milestone changed from Awaiting Review to 4.4.1

#3 @polevaultweb
9 years ago

  • Keywords has-patch needs-testing added

The patch adds a filter so the check to see if the image can have srcset added to it can be overridden.

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

#4 @joemcgill
9 years ago

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

Thanks for the report @polevaultweb. Can you give more information about how your plugin replaces the local URLs? If you can point me to some code that I could look at, that would be helpful as well.

WordPress makes use of the attachment ID information found in the class attribute of the image markup to decide which alternate image sizes should be loaded in a srcset attribute. To avoid loading the wrong images when the ID is incorrect (e.g., if the markup was imported from another WP installation), we check to make sure the image src matches one of the image sizes listed in the attachment metadata for that ID. See #34898 and a related bug that might be causing issues for you #35045.

#5 @polevaultweb
9 years ago

No problem @joemcgill! WP Offload S3 (https://wordpress.org/plugins/amazon-s3-and-cloudfront/) copies local files to S3 and then replaces local URLs in post_content via the database, and also hooks into wp_get_attachment_url to swap local for S3 URLs. The code in [35821] generates URLs for images locally based on image sizes so the image src can be searched for, therefore our S3 URLs will never be found and srcset applied.

Let me know if you need anything further.

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

#6 @joemcgill
9 years ago

After a brief Slack chat with @polevaultweb it seems like this use case is something that should be handled by the plugin itself, but after r35821, the wp_calculate_image_srcset filter is no longer reachable because we bail early if the src doesn't match one of the available sizes in the attachment meta.

@polevaultweb I wonder if a workaround would be to filter wp_get_attachment_metadata to insert your offloaded files so wp_image_add_srcset_and_sizes() can find the corresponding image sizes?

#7 @polevaultweb
9 years ago

Actually, the change proposed in #35045 to just compare filenames will work for our use case. This can be closed once #35045 is in, just in case other scenarios aren't covered by it.

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

#8 @joemcgill
9 years ago

  • Status changed from reviewing to accepted

Thanks @polevaultweb. I'm going to keep this ticket open for now and look into ways we might be able to make the filter in wp_calculate_image_srcset reachable for cases like these.

#9 @azaozz
9 years ago

See https://core.trac.wordpress.org/ticket/35045#comment:18

This is quite close to #35045 and the solution for one of them should fix the other.

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


9 years ago

#11 @azaozz
9 years ago

  • Resolution set to fixed
  • Status changed from accepted 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.

#12 @azaozz
9 years ago

  • Keywords fixed-major added; needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for 4.4.

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