Might be better to simply NULL the pointer in the uri_handler_t struct instead.

I got the same comment from @DanielEScherzer in the original PR, and I wrote him that I would like to avoid making the handlers optional if possible, because this way the existence of the handlers don't have to be checked before their usage - it's advantageous both for maintainability and performance.

The parse_url based implementation is special because it's not directly exposed for userland - it's just an internal URI "backend" for BC, and these handlers aren't necessarily needed for now. We could of course expose the to_string handlers later for 3rd party extensions if we want to. Then the code should probably be changed to something else.

A function that triggers undefined behavior when called (this is what ZEND_UNREACHABLE implies for production builds) and not having a function (i.e. dereferencing a NULL pointer when trying to call the function) are functionally the same. In both cases the PHP binary will do something bad (ideally just crash).

Thus it seems to be preferable to clearly indicate that the handler is not available by using NULL rather than pretending there is a handler when calling it is unsafe.

OK, I see what you mean now. I can't comment on what method is preferable, but intentionally passing a NULL value instead of a handler function, while callers of the handlers never expect NULL also seems wrong. Normally, static analyzers would emit an error in this case (in PHP for sure, and I don't know about C), that's why I didn't even think about this solution.

TBH the code which uses ZEND_UNREACHABLE() is unreachable indeed if one uses the internal API: currently, no function is exposed that would make use of the relevant handlers.

Ditto

Defaulting to parse_url in a new API is probably not a good idea. Instead the “legacy” users should just pass "parse_url" explicitly.

Defaulting to parse_url here works because that's the default indeed where php_uri_get_handler() is called, the other "backends" can only be used if the config is explicitly passed (not null).

The other reason why I opted for this approach is that it would be inconvenient to create and free a new zend_string when the legacy implementation is needed, and I wanted to avoid adding a known string just for this purpose, or exposing the C string based uri_handler_by_name function instead.

The addition of these new helpers is not clear to me. It feels like just another layer of indirection by moving the enum into the function name. There's also already uri_property_handler_from_internal_uri(), why doesn't it work here?

These functions come from the time when the property was passed as a zend_string, so having separate methods used to make sense. You are right, these are not really needed anymore, so I'm fine with removing them.

Although the alternative code is quite much longer, and a bit more difficult to use:

- zend_result result = php_uri_get_host(internal_uri, URI_COMPONENT_READ_RAW, &host_zv);
+ zend_result result = php_uri_property_handler_from_internal_uri(internal_uri, URI_PROPERTY_NAME_USERNAME)->read_func(internal_uri, URI_COMPONENT_READ_RAW, &host_zv);

+it makes the handlers directly available for usage, which I wanted to avoid for now (because write handlers are not always available)