FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Implement "Followup improvements for ext/uri" RFC - WHATWG URL percent-encoding by kocsismate · Pull Request #22820 · php/php-src · GitHub

/ php-src Public

Implement "Followup improvements for ext/uri" RFC - WHATWG URL percent-encoding - #22820

Open
kocsismate wants to merge 4 commits into
php:masterfrom
kocsismate:url-percent-encode
ConversationCommits (4)ChecksFiles changed
Open

Implement "Followup improvements for ext/uri" RFC - WHATWG URL percent-encoding#22820
kocsismate wants to merge 4 commits into
php:masterfrom
kocsismate:url-percent-encode

Conversation

kocsismate commented Jul 20, 2026

Copy link
Copy Markdown
Member

kocsismate requested a review from ndossche July 20, 2026 07:51
kocsismate requested a review from TimWolla as a code owner July 20, 2026 07:51
kocsismate added the RFC label Jul 20, 2026
kocsismate commented Jul 20, 2026
View reviewed changes
Comment thread ext/lexbor/lexbor/url/url.c Outdated
kocsismate commented Jul 20, 2026
View reviewed changes
Comment thread ext/uri/uri_parser_whatwg.c Outdated
kocsismate commented Jul 20, 2026
View reviewed changes
Comment thread ext/uri/php_uri.c
TimWolla reviewed Aug 1, 2026
View reviewed changes

TimWolla left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Didn't yet look at the tests.

Comment thread ext/uri/uri_parser_whatwg.c Outdated
Comment thread ext/uri/uri_parser_whatwg.c Outdated
Comment thread ext/uri/uri_parser_whatwg.c Outdated
Comment thread ext/uri/uri_parser_whatwg.c Outdated
Comment thread ext/uri/php_uri.c Outdated
Comment thread ext/uri/php_uri.c
Comment thread ext/uri/uri_parser_whatwg.c Outdated
kocsismate force-pushed the url-percent-encode branch 3 times, most recently from bc6c53a to b9931b3 Compare August 19, 2026 14:40
kocsismate requested a review from arnaud-lb August 19, 2026 14:40

iluuu1994 commented Aug 24, 2026

Copy link
Copy Markdown
Member

@kocsismate Will this land soon?

kocsismate commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

@iluuu1994 @jrfnl IMO this is merge ready, and I would love to make this merged, but I'm not getting reviews since a long time :'( I know timing is very unfortunate, since it's during the Summer, but I'm a bit stressed that my changes won't meet the deadlines. Besides this one, the last missing piece is "RFC 3986 percent-encoding", but it also waits for external review (uriparser/uriparser#337 (comment)).

kocsismate mentioned this pull request Aug 24, 2026
Merged

ndossche commented Aug 24, 2026

Copy link
Copy Markdown
Member

I'm going to remove my request for review, I have no interest in doing free labour, I've done that enough, sorry.

ndossche removed their request for review August 24, 2026 11:59

kocsismate commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

I'm going to remove my request for review, I have no interest in doing free labour, I've done that enough, sorry.

That's totally understandable, and thanks for your tons of help. I really appreciate it!

Girgias commented Aug 24, 2026

Copy link
Copy Markdown
Member

Maybe @nyamsprod can review the tests already?

Girgias approved these changes Aug 24, 2026
View reviewed changes

Girgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I can't comment on the tests as I don't know the spec. But considering the C side is quite straightforward and just forwards everything to Lexbor I'm +1 on the C implementation.

Comment thread ext/uri/php_uri.c
}

/* This should be unreachable in practice, as str is null only due to memory errors. */
if (str == NULL) {

Girgias Aug 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
if (str == NULL) {
if (UNEXPECTED(str == NULL)) {

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

TimWolla TimWolla left review comments
Girgias Girgias approved these changes
arnaud-lb Awaiting requested review from arnaud-lb

Assignees

No one assigned

Labels

ABI break Extension: uri RFC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL