Skip to content

Fix: Colon in url path is not parsed by parse_url #12704

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

Closed
wants to merge 3 commits into from

Conversation

vdauchy
Copy link

@vdauchy vdauchy commented Nov 17, 2023

This aims to solve #12703

Note: This is my first PR to php-src, could you clarify how the test file shall be named (temporarily named bugX.phpt ) ?

@vdauchy vdauchy marked this pull request as ready for review November 17, 2023 16:40
@vdauchy vdauchy requested a review from bukka as a code owner November 17, 2023 16:40
@jorgsowa
Copy link
Contributor

jorgsowa commented Nov 17, 2023

Hey!
You can add the number of Github Issue insetad of X, as was done i.e. https://github.com/php/php-src/pull/12679/files

So for your case would be gh12703.phpt.

Edit: sorry for previous confusion.

@nielsdos
Copy link
Member

nielsdos commented Nov 17, 2023

Also: for bugs on the github bugtracker the filename should start with gh instead of bug. And the title "GH-..." instead of "Bug #..."

@bukka
Copy link
Member

bukka commented Nov 17, 2023

Just to clarify, the bug prefix is for bugs in old bug tracker. For GitHub issue, we use gh prefix. So the test name should be gh12703.phpt

@nielsdos
Copy link
Member

Gah I mixed the two somehow in my answer, sorry, edited to be correct now.

@nicolas-grekas
Copy link
Contributor

Should this target 8.1?

@vdauchy
Copy link
Author

vdauchy commented Nov 17, 2023

  • Test file's name and title corrected. @bukka

@nicolas-grekas I don't know... I will happily re-do a PR to a lower branch if that's required...

@iluuu1994
Copy link
Member

iluuu1994 commented Nov 18, 2023

Related: #7890 (comment)

Changes in the parser are risky, because people use them on invalid URLs. Thus, it's very hard to predict what side-effects a change has. Obviously we should prioritize parsing of valid URLs, but it might be better to be prudent and merge this for master only.

@vdauchy
Copy link
Author

vdauchy commented Nov 18, 2023

Thanks for your comment @iluuu1994, I added an example for context (the case where query params are given) and the cases from the issue you referenced (which this PR solves 😃).

@vdauchy
Copy link
Author

vdauchy commented Jan 26, 2024

Hello @bukka , I was wondering if they would be any follow up / merge of this PR or is it any thing else I shall do ?

@vdauchy
Copy link
Author

vdauchy commented May 11, 2025

Closing following RFC: https://wiki.php.net/rfc/url_parsing_api

@vdauchy vdauchy closed this May 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants