parse_url raises ValueError in some error code paths.

[devnexen]

No description provided.

[mvorisek]

This would solve phpstan/phpstan#8638, can parse_url not return false then?

[devnexen]

no indeed it would not.

[mvorisek]

looking into the code

php-src/ext/standard/url.c

Line 353 in 56a8baf

/* @todo Find a method to determine why php_url_parse_ex() failed */

false is returned only here and in php_url_parse_ex2 you throw before every null return, so if I am not mistaken false is never returned, thus can/should be removed from

php-src/ext/standard/basic_functions.stub.php

Line 3421 in 56a8baf

function parse_url(string $url, int $component = -1): int|string|array|null|false {}

[Girgias]

This looks sensible, but I don't know if we could make the behaviour default to strict in a minor version :-/

[Girgias]

The naming is getting out of hand, and we might want to start removing or deprecating the version which just passes a char * (which now that I stare at I don't know if it's the content of the str that is constant or the pointer address that is constant...). Or maybe adding a version which takes a zend_string might be another idea?

But to preserve API BC for extension, it might make sense to move this to a function named php_url_parse_ex3() and passing false as the strict parameter. This looks like it would prevent the diffs in most of the other extensions.

[devnexen]

fair enough I did that on purpose in case we would apply the strictness aspect in a couple of sub modules but I can backpedal on this, no problem.

[Girgias]

Maybe let's get some opinions from other people, @cmb69, @derickr ?

[devnexen]

oh ok I saw your comment too late

[cmb69]

In my opinion, parse_url() is still broken beyond repair; instead of trying minor improvements, I suggest to add a replacement with clearly specified semantics. :)

[devnexen]

In my opinion, parse_url() is still broken beyond repair; instead of trying minor improvements, I suggest to add a replacement with clearly specified semantics. :)

you re surely right albeit this was aiming 8.3 while what you suggest might be more fit 9

[cmb69]

you re surely right albeit this was aiming 8.3 while what you suggest might be more fit 9

Bad wording from my side. I don't think we should replace parse_url(), but rather to offer an additional function. That could be introduced in a minor version. :)

[Girgias]

With the new policy this could go ahead, or maybe we should deprecate the function in favour of the new URI API.

[TimWolla]

or maybe we should deprecate the function in favour of the new URI API.

At this point I would just deprecate and not change anything about parse_url().