Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] set_error_handler and set_exception_handler behavior

Posted by Nikita Popov 
Hi internals!

Laruence and myself yesterday tried to slightly modify the
set_error_handler behavior, but the change which we considered trivial
was not perceived as such by others. So we are taking this to the ML.

Current state of things:
* set_error_handler(callback) and set_exception_handler(callback)
will return the previous error/exception handler
* set_exception_handler(NULL) will reset the exception handler (i.e.
use the default PHP exception handler again) and return bool(true)
* set_error_handler(NULL) throws an invalid argument error => inconsistent

What we wanted to do:
Allow the error handler being reset using set_error_handler(NULL). To
be consistent with set_exception_handler(NULL) this was supposed to
return bool(true).

What people objected to:
* Stas: The bool(true) return value does not really make sense.
Instead the previous error/exception handler should be returned, as
always.
* Pierre: One shouldn't be able to reset the error/exception handler
like that in any case. This behavior should be removed and instead
there should be additional reset_error_handler() and
reset_exception_handler() functions (and also get_error_handler() and
get_exception_handler() if I got that right.)

What I would do:
Add support for set_error_handler(NULL) and change the return value
of set_error_handler(NULL)+set_exception_handler(NULL) to the previous
handler (i.e. Stas option). I implemented this option in this PR:
https://github.com/php/php-src/pull/20

So, what are your opinions on this?

Nikita

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Sat, Mar 24, 2012 at 9:26 PM, Nikita Popov <[email protected]> wrote:
> Hi internals!
>
> Laruence and myself yesterday tried to slightly modify the
> set_error_handler behavior, but the change which we considered trivial
> was not perceived as such by others. So we are taking this to the ML.
>
> Current state of things:
>  * set_error_handler(callback) and set_exception_handler(callback)
> will return the previous error/exception handler
>  * set_exception_handler(NULL) will reset the exception handler (i.e..
> use the default PHP exception handler again) and return bool(true)
>  * set_error_handler(NULL) throws an invalid argument error => inconsistent
>
> What we wanted to do:
> Allow the error handler being reset using set_error_handler(NULL). To
> be consistent with set_exception_handler(NULL) this was supposed to
> return bool(true).
>
> What people objected to:
>  * Stas: The bool(true) return value does not really make sense.
> Instead the previous error/exception handler should be returned, as
> always.
>  * Pierre: One shouldn't be able to reset the error/exception handler
> like that in any case. This behavior should be removed and instead
> there should be additional reset_error_handler() and
> reset_exception_handler() functions (and also get_error_handler() and
> get_exception_handler() if I got that right.)
>
> What I would do:
>  Add support for set_error_handler(NULL) and change the return value
> of set_error_handler(NULL)+set_exception_handler(NULL) to the previous
> handler (i.e. Stas option). I implemented this option in this PR:
> https://github.com/php/php-src/pull/20
>
> So, what are your opinions on this?
I was plan to explain why, but you described very well :)

thanks
>
> Nikita
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>



--
Laruence  Xinchen Hui
http://www.laruence.com/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Hi!

> What people objected to:
> * Stas: The bool(true) return value does not really make sense.
> Instead the previous error/exception handler should be returned, as
> always.
> * Pierre: One shouldn't be able to reset the error/exception handler
> like that in any case. This behavior should be removed and instead
> there should be additional reset_error_handler() and
> reset_exception_handler() functions (and also get_error_handler() and
> get_exception_handler() if I got that right.)

I don't think ability to reset handlers by passing null is a big
problem, and creating another four functions seems excessive to me.
Also, the use case for the latter ones is not clear - why would you need
old error handler if you are not changing it? If you are changing it,
the case for getting the old one is clear - you may want to restore it
later. But if you're not, why you'd need it?

As for returning true on null, I see no sense if this behavior. Can
anybody explain to me what is it useful for? Why not return old handler
just as it is done in all other cases of setting the handler?

> What I would do:
> Add support for set_error_handler(NULL) and change the return value
> of set_error_handler(NULL)+set_exception_handler(NULL) to the previous
> handler (i.e. Stas option). I implemented this option in this PR:
> https://github.com/php/php-src/pull/20

I think this makes sense (of course, since it's my proposal :). It is a
behavior change, so it should be confined to master branch.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
On Sat, Mar 24, 2012 at 8:29 PM, Stas Malyshev <[email protected]> wrote:
> I don't think ability to reset handlers by passing null is a big
> problem, and creating another four functions seems excessive to me.
> Also, the use case for the latter ones is not clear - why would you need
> old error handler if you are not changing it? If you are changing it,
> the case for getting the old one is clear - you may want to restore it
> later. But if you're not, why you'd need it?
Agree.

> As for returning true on null, I see no sense if this behavior. Can
> anybody explain to me what is it useful for? Why not return old handler
> just as it is done in all other cases of setting the handler?
Agree.

>> What I would do:
>>  Add support for set_error_handler(NULL) and change the return value
>> of set_error_handler(NULL)+set_exception_handler(NULL) to the previous
>> handler (i.e. Stas option). I implemented this option in this PR:
>> https://github.com/php/php-src/pull/20
>
> I think this makes sense (of course, since it's my proposal :). It is a
> behavior change, so it should be confined to master branch.
Not sure about that part. I think both changes should be considered as
bug fixes and as such should also be eligible for 5.3/5.4. The
set_error_handler(null) change has no impact on BC whatsoever and the
return value change only has a theoretical BC impact (why would you
check the return value if it is *always* true?)

Nikita

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sorry, only registered users may post in this forum.

Click here to login