Space in attachment filename breaks srcset

[underdude]

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.

[underdude]

Patch to fix the problem

[nbachiyski]

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

[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.

[underdude]

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...

[joemcgill]

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.

[gnowland]

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 );

[joemcgill]

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]

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?

[underdude]

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.

[joemcgill]

Adds unit tests

[joemcgill]

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

[joemcgill]

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.