Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve URL Encoding of Path Component #180

Merged
merged 4 commits into from
Nov 20, 2020

Conversation

mdawaffe
Copy link
Contributor

The example at https://uri.thephpleague.com/uri/6.0/rfc3986/#uri-normalization shows how Uri::createFromString() can be used to normalize URLs:

$uri = Uri::createFromString(
    "hTTp://www.ExAmPLE.com:80/hello/./wor ld?who=f 3#title"
);
echo $uri; //displays http://www.example.com/hello/./wor%20ld?who=f%203#title

Unfortunately, the actual output is not the same as the documented output:

Documented
http://www.example.com/hello/./wor%20ld?who=f%203#title
Actual
http://www.example.com/hello/./wor ld?who=f%203#title

(Note the unencoded whitespace in the path component and the correctly encoded query component of the actual output.)

This is caused by some unwanted escaping in URI::formatPath()'s regex:

uri/src/Uri.php

Line 975 in 1d6a347

static $pattern = '/(?:[^'.self::REGEXP_CHARS_UNRESERVED.self::REGEXP_CHARS_SUBDELIM.'%:@\/}{]++\|%(?![A-Fa-f0-9]{2}))/';

(It should read | instead of \|.)

This PR corrects that regex and adds a test case.

Unfortunately, that regex change broke another test: one concerning a Windows path syntax I have not encountered before: C| instead of the more common C:. To fix that test, I've changed how Uri::formatFilePath() works, but, being unfamiliar with the syntax, I'm not sure if I've actually fixed the underlying problem or if I've just done enough to pass the existing tests. (Also, the changes I made there are a bit ugly :)).

@nyamsprod
Copy link
Member

@mdawaffe Thanks for the PR ... at first glance I don't see any issue.
You can already add the fix in the CHANGELOG.md and don't forget to mention yourself 👍
Once this is done I'll merge the work and release the patch version 😉

@mdawaffe
Copy link
Contributor Author

@nyamsprod, I took a shot at updating the changelog. I made up a date :) Please edit/reformat/whatever anything you want!

@nyamsprod nyamsprod merged commit f89ee19 into thephpleague:master Nov 20, 2020
@nyamsprod
Copy link
Member

Cool thanks again I'll merge this PR and try to release a patch version this weekend or worse case scenario next monday 👍

@mdawaffe
Copy link
Contributor Author

Awesome - thanks!

@nyamsprod
Copy link
Member

version 6.4.0 is released with your patch ... 💯 🎉

@mdawaffe mdawaffe deleted the encode-path branch November 23, 2020 19:42
@mdawaffe
Copy link
Contributor Author

Great - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants