WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#35106 closed defect (bug) (fixed)

Responsive images break uploads with full path stored in metadata

Reported by: SergeyBiryukov Owned by: joemcgill
Milestone: 4.4.1 Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords: has-patch has-unit-tests fixed-major
Focuses: Cc:
PR Number:

Description

Before [8796], we used to store absolute file paths for uploaded images.

On my install, I have a couple files uploaded in a pre-2.7 era, where _wp_attached_file looks like this: H:\home\wordpress\trunk/wp-content/uploads/2008/06/2429111678_2ecb3abb93_b.jpg.

They used to display correctly before 4.4, but now the following markup is generated:

<img width="1024" height="755" src="http://trunk.wordpress/wp-content/uploads/2008/06/2429111678_2ecb3abb93_b.jpg" class="attachment-post-thumbnail size-post-thumbnail wp-post-image" alt="Sticky" srcset="http://trunk.wordpress/wp-content/uploads/H:\home\wordpress\trunk/wp-content/uploads/2008/06/2429111678_2ecb3abb93_b-300x221.jpg 300w, http://trunk.wordpress/wp-content/uploads/H:\home\wordpress\trunk/wp-content/uploads/2008/06/2429111678_2ecb3abb93_b.jpg 1024w" sizes="(max-width: 709px) 85vw, (max-width: 909px) 67vw, (max-width: 984px) 60vw, (max-width: 1362px) 62vw, 840px">

The culprit is wp_calculate_image_srcset(), which assumes that dirname( $image_meta['file'] ) is always relative.

We could borrow the fix from wp_get_attachment_url(), where it was added in [10254].

Attachments (3)

35106.patch (2.6 KB) - added by SergeyBiryukov 4 years ago.
35106.2.patch (3.9 KB) - added by SergeyBiryukov 4 years ago.
35106.diff (9.6 KB) - added by dd32 4 years ago.

Download all attachments as: .zip

Change History (16)

#1 @swissspidy
4 years ago

  • Summary changed from Responsive images break uploads will full path stored in metadata to Responsive images break uploads with full path stored in metadata

#2 follow-up: @joemcgill
4 years ago

  • Summary changed from Responsive images break uploads with full path stored in metadata to Responsive images break uploads will full path stored in metadata

Thanks @SergeyBiryukov,

The fix looks like it will work but makes me wonder if we should institutionalize this knowledge in the form of a helper function for calculating the file path of an image, which could be used in both places?

#3 @SergeyBiryukov
4 years ago

  • Summary changed from Responsive images break uploads will full path stored in metadata to Responsive images break uploads with full path stored in metadata

#4 @joemcgill
4 years ago

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

#5 in reply to: ↑ 2 @SergeyBiryukov
4 years ago

Replying to joemcgill:

The fix looks like it will work but makes me wonder if we should institutionalize this knowledge in the form of a helper function for calculating the file path of an image, which could be used in both places?

Good point, 35106.2.patch introduces _wp_get_attachment_relative_path() and uses it in both places.

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


4 years ago

#7 @azaozz
4 years ago

35106.2.patch looks good. Unfortunately I don't have any pre-2.7 attachments to test it "live".

#8 @azaozz
4 years ago

Ran into another similar case on old multisite installs. The full path is stored again but instead of wp-content/uploads, there is wp-content/blogs.dir/[random]/files. This is handled by the previous row in wp_get_attachment_url(): https://core.trac.wordpress.org/browser/tags/4.4/src/wp-includes/post.php?marks=4872#L4870. In our case it should be:

$file = str_replace( $uploads['basedir'], '', $file );

Seems we will have to cache both basedir and baseurl in _wp_upload_dir_baseurl().

@dd32
4 years ago

#9 @dd32
4 years ago

35106.diff is based off 35106.2.patch

  • Tested with /non-existant/path/wp-content/uploads/2015/12/image.jpg
  • Tested with /non-existant/path/wp-content/uploads/image.jpg
  • _wp_get_attachment_relative_path() returns a standard output, a foldername only, no . or filename.
  • Needed to suffix the image url to _wp_get_attachment_relative_path() in one spot
  • Needed to use _wp_get_attachment_relative_path() within the responsive image generation (It wasn't generating srcsets for them)

@azaozz:

Ran into another similar case on old multisite installs.

I can't quite work out the details here on what needs doing, or where you're proposing it needs to be taken into account, or where it's actually broken.. AFAICT multisite is already being handled right?

#10 @azaozz
4 years ago

35106.diff looks good. The case for old multisite installs can be handled through the (proposed) filter on the image meta for #35045, see https://core.trac.wordpress.org/attachment/ticket/35045/35045.8.diff.

#11 @azaozz
4 years ago

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

In 36120:

Responsive images: add compatibility for versions < 2.7 when the full image path was stored in the metadata. Introduces _wp_get_attachment_relative_path() and uses it in wp_get_attachment_url().

Props dd32, SergeyBiryukov.
Fixes #35106 for trunk.

#12 @azaozz
4 years ago

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

Reopen for 4.4.

#13 @dd32
4 years ago

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

In 36151:

Responsive images: add compatibility for versions < 2.7 when the full image path was stored in the metadata. Introduces _wp_get_attachment_relative_path() and uses it in wp_get_attachment_url().

Merges [36120] to the 4.4 branch.
Props dd32, SergeyBiryukov.
Fixes #35106.

Note: See TracTickets for help on using tickets.