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

Workaround for parsing colons in partial URLs #12

Closed
wants to merge 1 commit into from
Closed

Workaround for parsing colons in partial URLs #12

wants to merge 1 commit into from

Conversation

PVince81
Copy link

@PVince81 PVince81 commented Dec 4, 2016

Whenever a URL contains no host part, the presence of a colon will make
PHP's parse_url fail. To workaround this bug, we need to encode the
colon in this situation.

However we do not encode colons for full URLs.

Fixes #9

@evert would you accept a backport for this to the sabre-uri 1.0 series ? (not sure how to proceed, no stable branches ?)

Please review

// we need to encode it to avoid a PHP bug
$replaceRegExp = '/(?:[^[:ascii:]]|:)/u';
} else {
$replaceRegExp = '/[^[:ascii:]]/u';
Copy link
Member

Choose a reason for hiding this comment

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

Indent

Whenever a URL contains no host part, the presence of a colon will make
PHP's parse_url fail. To workaround this bug, we need to encode the
colon in this situation.

However we do not encode colons for full URLs.
@PVince81
Copy link
Author

PVince81 commented Dec 5, 2016

Fixed indents

@evert
Copy link
Member

evert commented Dec 6, 2016

Hey!

This is a good patch, but I'm going to spend an hour or so trying an alternative route: a full regexp replacement of parse_url. The reason for this is because there is also issue #11 and I might be able to kill two birds with one stone. If I'm not successful within the hour I will give up and merge this instead.

I'll also open a 1.0 branch and backport this change.

@evert
Copy link
Member

evert commented Dec 6, 2016

Ah this patch is actually flawed. It urlencodes any : as far as I can tell.

@PVince81
Copy link
Author

PVince81 commented Dec 6, 2016

Ah this patch is actually flawed. It urlencodes any : as far as I can tell.

Yes, but only does it for relative URLs, not absolute URLs. Somehow the PHP bug only affects relative URLs.

Anyway, will check your other patch. Thanks!

@PVince81 PVince81 closed this Dec 6, 2016
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.

None yet

3 participants