Add the Uri\Rfc3986\Uri class to ext/uri without wither support by kocsismate · Pull Request #18836 · php/php-src

kocsismate · 2025-06-11T17:57:49Z

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

ext/uri/php_uriparser.c

ext/uri/uriparser/include/uriparser/Uri.h

ndossche

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

Zend/zend_string.h

ext/uri/php_uri.c

ext/uri/php_uriparser.c

TimWolla

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

ext/uri/php_uriparser.c

TimWolla · 2025-06-12T07:19:33Z

ext/uri/php_uriparser.c

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.

TimWolla · 2025-06-12T07:22:45Z

ext/uri/php_uriparser.c

+	efree(uriparser_uris);
+}
+
+const uri_handler_t uriparser_uri_handler = {


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.

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

ext/uri/php_uriparser.h

ext/uri/php_uri.c

ext/uri/php_uriparser.c

ext/uri/php_uriparser.h

ext/uri/php_uriparser.c

ext/uri/php_uri.c

ext/uri/php_uriparser.c

kocsismate · 2025-06-15T04:36:13Z

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 · 2025-06-15T10:14:46Z

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

ext/uri/uriparser/src/UriCopy.c

DanielEScherzer · 2025-07-01T23:03:52Z

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

TimWolla

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.

TimWolla · 2025-07-02T07:18:15Z

ext/uri/php_uri.c

-PHP_METHOD(Uri_WhatWg_InvalidUrlException, __construct)
-{
-	zend_string *message = NULL;
-	zval *errors = NULL;
-	zend_long code = 0;
-	zval *previous = NULL;
-
-	ZEND_PARSE_PARAMETERS_START(0, 4)
-		Z_PARAM_OPTIONAL
-		Z_PARAM_STR(message)
-		Z_PARAM_ARRAY(errors)
-		Z_PARAM_LONG(code)
-		Z_PARAM_OBJECT_OF_CLASS_OR_NULL(previous, zend_ce_throwable)
-	ZEND_PARSE_PARAMETERS_END();
-
-	if (zend_update_exception_properties(INTERNAL_FUNCTION_PARAM_PASSTHRU, message, code, previous) == FAILURE) {
-		RETURN_THROWS();
-	}
-
-	if (errors == NULL) {
-		zval tmp;
-		ZVAL_EMPTY_ARRAY(&tmp);
-		zend_update_property(uri_whatwg_invalid_url_exception_ce, Z_OBJ_P(ZEND_THIS), ZEND_STRL("errors"), &tmp);
-	} else {
-		zend_update_property(uri_whatwg_invalid_url_exception_ce, Z_OBJ_P(ZEND_THIS), ZEND_STRL("errors"), errors);
-	}
-	if (EG(exception)) {
-		RETURN_THROWS();
-	}
-}
-
-PHP_METHOD(Uri_WhatWg_UrlValidationError, __construct)
-{
-	zend_string *context;
-	zval *type;
-	bool failure;
-
-	ZEND_PARSE_PARAMETERS_START(3, 3)
-		Z_PARAM_STR(context)
-		Z_PARAM_OBJECT_OF_CLASS(type, uri_whatwg_url_validation_error_type_ce)
-		Z_PARAM_BOOL(failure)
-	ZEND_PARSE_PARAMETERS_END();
-
-	zend_update_property_str(uri_whatwg_url_validation_error_ce, Z_OBJ_P(ZEND_THIS), ZEND_STRL("context"), context);
-	if (EG(exception)) {
-		RETURN_THROWS();
-	}
-
-	zend_update_property_ex(uri_whatwg_url_validation_error_ce, Z_OBJ_P(ZEND_THIS), ZSTR_KNOWN(ZEND_STR_TYPE), type);
-	if (EG(exception)) {
-		RETURN_THROWS();
-	}
-
-	zval failure_zv;
-	ZVAL_BOOL(&failure_zv, failure);
-	zend_update_property(uri_whatwg_url_validation_error_ce, Z_OBJ_P(ZEND_THIS), ZEND_STRL("failure"), &failure_zv);
-	if (EG(exception)) {
-		RETURN_THROWS();
-	}
-}


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

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.

TimWolla · 2025-07-02T07:19:11Z

ext/uri/php_uri.c

+PHP_METHOD(Uri_Rfc3986_Uri, equals)
+{
+	zend_object *that_object;
+	zend_object *comparison_mode = NULL;
+
+	ZEND_PARSE_PARAMETERS_START(1, 2)
+		Z_PARAM_OBJ_OF_CLASS(that_object, uri_rfc3986_uri_ce)
+		Z_PARAM_OPTIONAL
+		Z_PARAM_OBJ_OF_CLASS(comparison_mode, uri_comparison_mode_ce)
+	ZEND_PARSE_PARAMETERS_END();
+
+	uri_equals(INTERNAL_FUNCTION_PARAM_PASSTHRU, that_object, comparison_mode);
+}


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.

ext/uri/php_uriparser.c

ndossche

Besides the things that Tim said, here some small remarks

ext/uri/php_uriparser.c

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

Co-authored-by: Tim Düsterhus <tim@tideways-gmbh.com>

TimWolla

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

ext/uri/php_uriparser.c

TimWolla · 2025-07-04T13:36:09Z

ext/uri/php_uriparser.c

+static size_t str_to_int(const char *str, size_t len)
+{
+	int result = 0;


Suggested change

static size_t str_to_int(const char *str, size_t len)

{

int result = 0;

static zend_long str_to_int(const char *str, size_t len)

{

zend_long result = 0;

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

No your suggestion is fine

ext/uri/php_uriparser.c

kocsismate requested a review from dstogov as a code owner June 11, 2025 17:57

github-actions bot added Category: Build System Category: Engine ABI break Extension: uri labels Jun 11, 2025

kocsismate requested review from TimWolla and ndossche and removed request for dstogov June 11, 2025 17:58

kocsismate marked this pull request as draft June 11, 2025 17:58

kocsismate force-pushed the ext-url6 branch 2 times, most recently from 43c1d9f to 155e070 Compare June 11, 2025 18:07

kocsismate changed the title ~~Add Uri\Rfc3986\Uri class to ext/uri~~ Add the Uri\Rfc3986\Uri class to ext/uri without wither support Jun 11, 2025

kocsismate commented Jun 11, 2025

View reviewed changes

ext/uri/php_uriparser.c Outdated Show resolved Hide resolved

kocsismate commented Jun 11, 2025

View reviewed changes

ext/uri/php_uriparser.c Outdated Show resolved Hide resolved

kocsismate commented Jun 11, 2025

View reviewed changes

ext/uri/uriparser/include/uriparser/Uri.h Outdated Show resolved Hide resolved

ndossche requested changes Jun 11, 2025

View reviewed changes

TimWolla reviewed Jun 11, 2025

View reviewed changes

ext/uri/php_uriparser.c Outdated Show resolved Hide resolved

ext/uri/php_uriparser.c Outdated Show resolved Hide resolved

kocsismate mentioned this pull request Jun 11, 2025

[RFC] Add RFC 3986 and WHATWG compliant URL parsing support #14461

Closed

TimWolla reviewed Jun 12, 2025

View reviewed changes

ext/uri/php_uriparser.c Outdated Show resolved Hide resolved

ext/uri/php_uriparser.c Outdated Show resolved Hide resolved

TimWolla reviewed Jun 12, 2025

View reviewed changes

ext/uri/php_uriparser.h Show resolved Hide resolved

ext/uri/php_uriparser.c Outdated Show resolved Hide resolved

DanielEScherzer reviewed Jun 12, 2025

View reviewed changes

ext/uri/php_uri.c Outdated Show resolved Hide resolved

ext/uri/php_uriparser.c Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

kocsismate force-pushed the ext-url6 branch from 7438aab to e50f18b Compare June 15, 2025 04:53

kocsismate marked this pull request as ready for review June 16, 2025 07:59

kocsismate force-pushed the ext-url6 branch 2 times, most recently from 0b3cfd8 to 062dc4d Compare June 20, 2025 06:04

kocsismate force-pushed the ext-url6 branch 3 times, most recently from 226735b to 7541b33 Compare July 1, 2025 20:08

kocsismate commented Jul 1, 2025

View reviewed changes

ext/uri/uriparser/src/UriCopy.c Show resolved Hide resolved

TimWolla reviewed Jul 2, 2025

View reviewed changes

ext/uri/php_uriparser.c Show resolved Hide resolved

TimWolla reviewed Jul 2, 2025

View reviewed changes

ext/uri/php_uriparser.c Outdated Show resolved Hide resolved

ndossche reviewed Jul 2, 2025

View reviewed changes

ext/uri/php_uriparser.c Show resolved Hide resolved

ext/uri/php_uriparser.c Outdated Show resolved Hide resolved

ext/uri/php_uriparser.c Show resolved Hide resolved

ext/uri/php_uriparser.c Show resolved Hide resolved

kocsismate and others added 13 commits July 4, 2025 08:21

Add Uri\Rfc3986\Uri class to ext/uri

2001503

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

Review round 1 fixes

54980ef

Another batch of fixes

4a2d9b1

Fix memory management

84b2c7a

Co-authored-by: Tim Düsterhus <tim@tideways-gmbh.com>

Fix Windows build

d01d31b

Improve and simplify exceptions

47fcc2d

Remove accidentally added extra ;

773ac66

Try to incorporate uriparser_copy_uri into uriparser

6ee8397

uriparser improvements

861baef

More improvements

b01a1ad

Allocate on stack

35a1217

Fixes after rebase

4f94e0d

Fixes

09efcdd

kocsismate force-pushed the ext-url6 branch from 471a762 to 09efcdd Compare July 4, 2025 06:29

TimWolla approved these changes Jul 4, 2025

View reviewed changes

Fix review comments

23f3197

TimWolla approved these changes Jul 4, 2025

View reviewed changes

ext/uri/php_uriparser.c Outdated Show resolved Hide resolved

ext/uri/php_uriparser.c Outdated Show resolved Hide resolved

One last fix

403b83b

TimWolla approved these changes Jul 4, 2025

View reviewed changes

kocsismate merged commit 5a9f5a6 into php:master Jul 5, 2025
9 checks passed

kocsismate deleted the ext-url6 branch July 5, 2025 08:00

Comments

Conversation

kocsismate commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ndossche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TimWolla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

kocsismate commented Jun 15, 2025

Uh oh!

TimWolla commented Jun 15, 2025

Uh oh!

Uh oh!

DanielEScherzer commented Jul 1, 2025

Uh oh!

TimWolla left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ndossche left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TimWolla left a comment

kocsismate commented Jun 11, 2025 •

edited

Loading

ndossche left a comment •

edited

Loading