Skip to content

Comments

Add percent encoding of URLs in imagesrcset param of Link response header#1866

Merged
westonruter merged 1 commit intorelease/3.9.0from
fix/srcset-url-encoding
Feb 12, 2025
Merged

Add percent encoding of URLs in imagesrcset param of Link response header#1866
westonruter merged 1 commit intorelease/3.9.0from
fix/srcset-url-encoding

Conversation

@westonruter
Copy link
Member

@westonruter westonruter commented Feb 12, 2025

In testing Optimization Detective for the release in 2 days, I discovered a problem when I upload an image file with non-ASCII filename (البيسون.jpg):

Image

It's returning a 404 because the image URLs in the imagesrcset aren't getting the same encoding as is the original URL in the href.

Link: <https://complicated-lyrebird-c797ca.instawp.xyz/wp-content/uploads/2025/02/%D8%A7%D9%84%D8%A8%D9%8A%D8%B3%D9%88%D9%86-1024x668.jpg>; rel="preload"; fetchpriority="high"; as="image"; imagesrcset="https://complicated-lyrebird-c797ca.instawp.xyz/wp-content/uploads/2025/02/البيسون-1024x668.jpg 1024w, https://complicated-lyrebird-c797ca.instawp.xyz/wp-content/uploads/2025/02/البيسون-300x196.jpg 300w, https://complicated-lyrebird-c797ca.instawp.xyz/wp-content/uploads/2025/02/البيسون-768x501.jpg 768w, https://complicated-lyrebird-c797ca.instawp.xyz/wp-content/uploads/2025/02/البيسون-1536x1002.jpg 1536w, https://complicated-lyrebird-c797ca.instawp.xyz/wp-content/uploads/2025/02/البيسون-2048x1336.jpg 2048w"; imagesizes="(782px < width) 644px, (max-width: 1024px) 100vw, 1024px"; media="screen and (782px < width)"

Expected versus actual:

- https://complicated-lyrebird-c797ca.instawp.xyz/wp-content/uploads/2025/02/%D8%A7%D9%84%D8%A8%D9%8A%D8%B3%D9%88%D9%86-1024x668.jpg
+ https://complicated-lyrebird-c797ca.instawp.xyz/wp-content/uploads/2025/02/البيسون-1024x668.jpg

This is something we missed in #1802 while fixing #1775.

So this PR adds URL encoding of the imagesrcset:

image

@westonruter westonruter added [Type] Bug An existing feature is broken [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels Feb 12, 2025
@github-actions
Copy link

github-actions bot commented Feb 12, 2025

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@westonruter westonruter changed the base branch from trunk to release/3.9.0 February 12, 2025 01:14
@westonruter westonruter added the skip changelog PRs that should not be mentioned in changelogs label Feb 12, 2025
@westonruter westonruter mentioned this pull request Feb 12, 2025
5 tasks
@codecov
Copy link

codecov bot commented Feb 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.70%. Comparing base (cb79463) to head (e731b4b).
Report is 2 commits behind head on release/3.9.0.

Additional details and impacted files
@@                Coverage Diff                @@
##           release/3.9.0    #1866      +/-   ##
=================================================
+ Coverage          66.64%   66.70%   +0.06%     
=================================================
  Files                 88       88              
  Lines               7015     7029      +14     
=================================================
+ Hits                4675     4689      +14     
  Misses              2340     2340              
Flag Coverage Δ
multisite 66.70% <100.00%> (+0.06%) ⬆️
single 37.20% <0.00%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

},
$decoded_url
);
return esc_url_raw( $encoded_url );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This was originally esc_url_raw( $encoded_url ?? '' ). However, this seems to have just been a way to work around the fact that preg_replace_callback() can technically return null when an invalid pattern is provided. However, since we know the pattern is correct based on tests, the use of (string) cast seems more appropriate.

@westonruter westonruter force-pushed the fix/srcset-url-encoding branch from 95ee543 to e731b4b Compare February 12, 2025 01:42
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter Great catch, LGTM!

@westonruter westonruter merged commit 5b5b5f8 into release/3.9.0 Feb 12, 2025
17 checks passed
@westonruter westonruter deleted the fix/srcset-url-encoding branch February 12, 2025 21:17
@westonruter westonruter mentioned this pull request Feb 12, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Plugin] Optimization Detective Issues for the Optimization Detective plugin skip changelog PRs that should not be mentioned in changelogs [Type] Bug An existing feature is broken

Projects

Status: Done 😃

Development

Successfully merging this pull request may close these issues.

3 participants