Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

reserved characters in path should be unencoded #258

Merged

Conversation

samsonasik
Copy link
Contributor

like in zend-uri issue zendframework/zend-uri#13 with zend-uri PR zendframework/zend-uri#17

@samsonasik samsonasik changed the title reserved characters in path should be unencoded unreserved characters in path should be unencoded Jul 15, 2017
@samsonasik samsonasik changed the title unreserved characters in path should be unencoded reserved characters in path should be unencoded Jul 15, 2017
@@ -556,7 +556,7 @@ private function filterScheme($scheme)
private function filterPath($path)
{
$path = preg_replace_callback(
'/(?:[^' . self::CHAR_UNRESERVED . ':@&=\+\$,\/;%]+|%(?![A-Fa-f0-9]{2}))/u',
'/(?:[^(' . self::CHAR_UNRESERVED . '):@&=\+\$,\/;%]+|%(?![A-Fa-f0-9]{2}))/u',
Copy link
Member

Choose a reason for hiding this comment

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

This is confusing the way you have it written; it looks like a mistake, though it's not.

I thought at first you were trying to create a capturing group by placing parens, and, since this expression is within square brackets, that would not work.

What you are trying to do is add parens to the character list, and, as such, it's not a mistake. It just reads like one.

As such, when I merge, I'm going to disambiguate and push them both into the concatenated string. Further, one common practice when writing a character group for a regex is to flip the order of parens, braces, etc: )( instead of (). This helps ensure that you don't accidentally create a capturing group, a character group, or a quantifying expression.

@weierophinney weierophinney merged commit 93f68cf into zendframework:master Aug 17, 2017
weierophinney added a commit that referenced this pull request Aug 17, 2017
reserved characters in path should be unencoded
weierophinney added a commit that referenced this pull request Aug 17, 2017
weierophinney added a commit that referenced this pull request Aug 17, 2017
weierophinney added a commit that referenced this pull request Aug 17, 2017
@weierophinney
Copy link
Member

Thanks, @samsonasik

@weierophinney weierophinney added this to the 1.4.1 milestone Aug 17, 2017
@samsonasik samsonasik deleted the reserved-chars-unencoded branch August 18, 2017 02:01
samsonasik added a commit to samsonasik/zend-uri that referenced this pull request Aug 18, 2017
@samsonasik
Copy link
Contributor Author

@weierophinney Thank you, I've updated the zend-uri PR at zendframework/zend-uri#17 to use )( instead of ()

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

Successfully merging this pull request may close these issues.

2 participants