On Sun, 19 Feb 2023 at 08:45, Nikita Popov wrote:
> On Sun, Feb 19, 2023, at 09:21, Max Kellermann wrote:
> > On 2023/02/19 08:56, Nikita Popov wrote:
> > > If you have a function like zend_stream_open_function(), SUCCESS and
> FAILURE are directly meaningful values.
> >
> > Agree, but that doesn't explain why FAILURE needs to be negative.
>
> I expect that there are two main reasons for that:
> - There are probably some places that return a (non-negative) value or
> FAILURE.
> - There are probably some places that check for success/failure using >=
> 0 and < 0. Another POSIX-ism.
>
> I don't think we endorse such usage, but it likely exists.
>
> Let me turn the question around: Is there some reason to change the value
> of FAILURE at this point?
>
> > > The current guideline for use of bool and zend_result in php-src is
> that bool is an appropriate return value for "is" or "has" style functions,
> which return a yes/no answer. zend_result is an appropriate return value
> for functions that perform some operation that may succeed or fail.
> >
> > What does the return value of these functions mean?
> >
> > - zend_make_printable_zval()
> > - zend_make_callable()
> > - zend_parse_arg_bool()
> > - zend_fiber_init_context()
> > - zend_observer_remove_begin_handler()
> > - php_execute_script()1
> >
> > If I understand the guideline correctly, then those examples (and
> > countless others) are defective and should be fixed, correct?
>
> At least in principle, yes. Of course, actually doing a change may not
> always be worthwhile, especially as this might result in a silent behavior
> change for extensions (as putting the return value into an "if" would
> invert behavior now).
>
> I believe Girgias has done extensive work on making the int vs bool vs
> zend_result situation more consistent, so you might want to coordinate with
> him.
>
> Regards,
> Nikita
Yeah, I spent a lot of time around the release of PHP 8.0 and a bit in
between releases to convert various int types to bool or zend_result just
to make the API clearer.
One of the main cases I missed was to change the return value of object
handlers, as those, for the most part, are meant to return zend_result.
I didn't push forward with that one as when I realized we were already in
beta releases or maybe RC and even changing it for a minor version did feel
a bit disruptive.
However, it may make sense to tackle this sooner than later if we are doing
some other object handler changes.
There are however definitely still cases where FAILURE is assumed to be -1
either because this is what the function is expected to return on failure
or checking the value of a POSIX like API.
Ideally, all cases where zend_result is used would actually indicate this
to make it possible to *maybe* convert SUCCESS to true and FAILURE to false
and typedef zend_result to bool.
Best,
George P. Banyard
