WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#36549 closed defect (bug) (fixed)

Space in attachment filename breaks srcset

Reported by: underdude Owned by: joemcgill
Milestone: 4.6 Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords: has-patch commit
Focuses: Cc:
PR Number:

Description

By default spaces in filenames are replaced with dashes on upload phase by sanitize_file_name, but if you create the attachments some other way, e.g. with wp_insert_attachment, the filenames stay untouched.

As the srcset syntax needs spaces to separate things, a space in filename breaks the whole srcset. Chrome gives you a Failed parsing 'srcset' attribute value since it has an unknown descriptor. in dev tools console.

This is obviously an edge case since attachment filenames shouldn't have spaces in the first place.

This can be fixed by replacing spaces when generating image urls for the srcset.

Attachments (4)

36549.diff (665 bytes) - added by underdude 4 years ago.
Patch to fix the problem
36549.2.diff (1.1 KB) - added by joemcgill 4 years ago.
36549.3.diff (1.1 KB) - added by joemcgill 4 years ago.
36549.4.diff (2.8 KB) - added by joemcgill 4 years ago.
Adds unit tests

Download all attachments as: .zip

Change History (15)

@underdude
4 years ago

Patch to fix the problem

#1 @nbachiyski
4 years ago

Is the space the only problem, would it be OK to use urlencode?

#2 follow-up: @joemcgill
4 years ago

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

Hi @underdude. Thanks for the report. I wonder if we should try to catch this when the file name is created rather than when we are generating 'srcset' attributes.

#3 in reply to: ↑ 2 @underdude
4 years ago

Replying to joemcgill:

Hi @underdude. Thanks for the report. I wonder if we should try to catch this when the file name is created rather than when we are generating 'srcset' attributes.

Yes, definitely that should be the primary way and that's what the default file upload process does. Since spaces (in addition to other special characters) in urls usually lead to problems later.

In our case we had a lot of previously imported content where the files were imported using the wp_insert_attachment which leaves the filenames as they are and only creates the attachment objects to db. And everything was working fine before the responsive image update.

It's also debatable that if this is a "user error", since processing the filenames to safe ones could be considered as something the developer should take care of when inserting attachments with wp_insert_attachment .

The urls can be fixed using the wp_calculate_image_srcset filter, so in cases like ours, a core fix is not required to make things work again.

We discussed this matter briefly with @nbachiyski on the WordCamp Finland Contributor Day and thought that if there is no side effects, this could be just a safety mechanism to prevent existing spaces in filenames not to break things even though the actual issue is somewhere else. Replacing spaces (or urlencoding the filename) is quite cheap operation afterall...

#4 @joemcgill
4 years ago

Hi @underdude,

Thanks for the added explanation. Since this seems to be an edge case that doesn't occur in core directly, but is related to improperly formed file names using wp_insert_attachment(), I think we should consider adding a patch there, but not in wp_calculate_image_srcset(). Reason being, we're attempting to keep wp_calculate_image_srcset() as lean as possible since it's used via a content filter on the front end.

#5 @gnowland
4 years ago

Typical +1 "it could happen to anyone" post as this issue bit me too. Here's the fix via filter for anyone else who may have this problem, as it took me a non-trivial amount of time (hours) to debug and patch. It would be great to see the above patch applied to cover those cases where image urls are malformed for whatever reason. Thanks guys.

<?php
function mynamespace_encode_img_src($sources){
        foreach($sources as $source => $values ){
                $sources[$source]['url'] = str_replace( ' ', '%20', $values['url'] );
        }
        return $sources;
}
add_filter( 'wp_calculate_image_srcset', 'mynamespace_encode_img_src', 10, 1 );
Last edited 4 years ago by gnowland (previous) (diff)

@joemcgill
4 years ago

#6 @joemcgill
4 years ago

36549.2.diff takes a similar approach to 36549.diff, but runs the image URL of each source in the srcset string through esc_url() right before it's returned. This should also catch any other strange characters that may have been included in filenames.

@joemcgill
4 years ago

#7 @joemcgill
4 years ago

On second thought, we already run srcset attributes through esc_attr() when used in HTML elements (see example). That should handle all of the special characters besides spaces, so esc_url() would be needlessly redundant in most cases. 36549.3.diff is essentially the same as 36549.diff but moves the str_replace() call after the filter is run, which seems right to me, but is there any reason we would not want to run the string replace after the filter?

#8 @underdude
4 years ago

Running the str_replace() after the filter seems right to me too. That way any new spaces can't be introduced through the filter and the replacements before the filter can't be undone.

#9 @joemcgill
4 years ago

  • Keywords has-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.6
  • Version changed from trunk to 4.4

@joemcgill
4 years ago

Adds unit tests

#10 @joemcgill
4 years ago

  • Keywords commit added; needs-unit-tests removed

36549.4.diff Adds a unit test and includes the fix from 36549.3.diff.

#11 @joemcgill
4 years ago

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

In 38052:

Media: URL encode spaces in srcset attributes.

In some cases, images in the media library may contain spaces in
their filenames. This results in an invalid srcset attribute,
causing broken images on the front end. This change fixes the issue
by replacing spaces in URLs with URL encoded '%20' characters before
returning the srcset string.

Props underdude, joemcgill.
Fixes #36549.

Note: See TracTickets for help on using tickets.