Skip to content

Add the Uri\Rfc3986\Uri class to ext/uri without wither support - #18836

Merged
kocsismate merged 15 commits into
php:masterfrom
kocsismate:ext-url6
Jul 5, 2025
Merged

Add the Uri\Rfc3986\Uri class to ext/uri without wither support#18836
kocsismate merged 15 commits into
php:masterfrom
kocsismate:ext-url6

Conversation

@kocsismate

@kocsismatekocsismate commented Jun 11, 2025

Copy link
Copy Markdown
Member

There are a few WIP parts but the PR can already be reviewed.

Relates to #14461 and https://wiki.php.net/rfc/url_parsing_api

@kocsismate
kocsismate requested a review from dstogov as a code ownerJune 11, 2025 17:57
@kocsismate
kocsismate requested review from TimWolla and ndossche and removed request for dstogovJune 11, 2025 17:58
@kocsismate
kocsismate marked this pull request as draft June 11, 2025 17:58
@kocsismate
kocsismateforce-pushed the ext-url6 branch 2 times, most recently from 43c1d9f to 155e070CompareJune 11, 2025 18:07
@kocsismatekocsismate changed the title Add Uri\Rfc3986\Uri class to ext/uriAdd the Uri\Rfc3986\Uri class to ext/uri without wither supportJun 11, 2025
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/uriparser/include/uriparser/Uri.h Outdated

@ndosschendossche 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.

Cursory glance, I see there's some todos wrt memory management as well so maybe I'll wait a bit on that

Comment threadZend/zend_string.h Outdated
Comment threadext/uri/php_uri.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated

@TimWollaTimWolla 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.

Some first remarks from just looking at the diff on GitHub.

Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated

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.

Something I'm only noticing now: I think this should rather be called ext/uri/parser_rfc3986.c and php_lexbor.c would be ext/uri/parser_whatwg.c. The php_uriparser.c name is confusing to me, because uriparser is an extremely generic term.

To give another comparison with ext/random, since it is architecturally similar: Each engine has its own engine_enginename.c file, e.g. engine_xoshiro256starstar.c.

Comment threadext/uri/php_uriparser.c Outdated

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.

And similarly the public symbols in the file should also be prefixed with php_uri_. Just uriparser_ can conflict with the uriparser library itself.

Here I would suggest php_uri_rfc3986_handler.

But I'm also happy to leave this to a follow-up to not introduce too much churn.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes, this and the file renaming is something that I'm happy to do after most parts are merged.

Comment threadext/uri/php_uriparser.h Outdated
Comment threadext/uri/php_uri.c Outdated
Comment threadext/uri/php_uri.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.h Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uri.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
@TimWolla

This comment was marked as resolved.

@kocsismate

Copy link
Copy Markdown
MemberAuthor

The following patch fixes the leaks / memory management for me:

@TimWolla Thanks for the patch, it's awesome! TBH I didn't even think about using the UriMakeOwnerA() function, because I wanted to avoid its performance penalty. But you are right, this is the right call, it simplifies the code so much. I love it!

@TimWolla

Copy link
Copy Markdown
Member

@kocsismate When stack allocating the struct the code should become even simpler (and possibly faster, due to fewer pointer indirection).

@kocsismate
kocsismate marked this pull request as ready for review June 16, 2025 07:59
@kocsismate
kocsismateforce-pushed the ext-url6 branch 2 times, most recently from 0b3cfd8 to 062dc4dCompareJune 20, 2025 06:04
@kocsismate
kocsismateforce-pushed the ext-url6 branch 2 times, most recently from 226735b to 7541b33CompareJuly 1, 2025 20:08
Comment threadext/uri/uriparser/src/UriCopy.c Outdated
@DanielEScherzer

Copy link
Copy Markdown
Member

I have no further suggestions, but don't feel qualified to approve this

@TimWollaTimWolla 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.

Looks mostly okay for an initial merge now. The additional clean-up can happen in a follow-up to move forward. The leak should be fixed, though.

Comment threadext/uri/php_uri.c Outdated
Comment on lines 107 to 166

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.

I don't particularly like that you reordered these. It makes the diff hard to read.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yes. I should have waited a little bit, sorry! A few methods had to be removed due to the added implementation aliases, and that was the point I think when I moved a few methods.

Comment threadext/uri/php_uri.c Outdated
Comment on lines 443 to 455

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.

Similarly to the other remark about file naming and organization, it might make sense to move the PHP_METHOD implementations into the matching source file to keep php_uri.c slim.

Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated

@ndosschendossche 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.

Besides the things that Tim said, here some small remarks

Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated

@TimWollaTimWolla 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.

Some minor suggestions, but this is good to merge for me now. Anything else can be done in follow-ups.

Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment on lines +165 to +167
static size_t str_to_int(const char *str, size_t len)
{
int result = 0;

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.

Suggested change
staticsize_tstr_to_int(constchar*str, size_tlen)
{
intresult=0;
staticzend_longstr_to_int(constchar*str, size_tlen)
{
zend_longresult=0;

It's being stored in a zend_long, so should return a zend_long. But perhaps @nielsdos has other suggestions.

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.

No your suggestion is fine

Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
Comment threadext/uri/php_uriparser.c Outdated
@kocsismate
kocsismate merged commit 5a9f5a6 into php:masterJul 5, 2025
@kocsismate
kocsismate deleted the ext-url6 branch July 5, 2025 08:00
Sign up for freeto 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.

4 participants

@kocsismate@TimWolla@DanielEScherzer@ndossche