Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:90538
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.220.180 as permitted sender)
MIME-Version: 1.0
Sender: giovanni.g@gmail.com
In-Reply-To: <5693B6BB.9020607@gmail.com>
References: <CAML1Pf7ti=CG1DRfpPYw0yeyRCUC0ca=ZkKkm3xEHFB_P3_ruw@mail.gmail.com>
	<5693B6BB.9020607@gmail.com>
Date: Tue, 12 Jan 2016 10:37:30 +0100
Message-ID: <CAML1Pf5-P3XtyetnPwiDOLTOdMHjretKGtpa1hxuZzY45Fgb7Q@mail.gmail.com>
To: Rowan Collins <rowan.collins@gmail.com>
Cc: internals@lists.php.net
Content-Type: multipart/alternative; boundary=001a114a96b26ce0c705291fcdde
Subject: Re: [PHP-DEV] Throwable and Error exceptions break existing PHP 5.x code
From: giovanni@giacobbi.net (Giovanni Giacobbi)

--001a114a96b26ce0c705291fcdde
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

I want to thank Markus and Bj=C3=B6rn for getting me up to speed with the
pointers.
Please see the rest of my response below.

On 11 January 2016 at 15:05, Rowan Collins <rowan.collins@gmail.com> wrote:

> Giovanni Giacobbi wrote on 11/01/2016 13:31:
>
>> set_exception_handler(callback function, bool also_throwables =3D false)=
;
>>
>> The new parameter "also_throwables" defaults to false, so the same
>> behaviour as before is preserved. If you want it to catch also the new
>> PHP7
>> Error exceptions, you can just set it to true.
>>
>
> This is an interesting idea, but other than the specific transition to PH=
P
> 7, I'm not sure when it would be used.
>
> Put it this way: if Errors existed when you first wrote this code, would
> you want to handle them using your custom handler functions, or would you
> prefer them to the fall through to the default server White Screen of Doo=
m?
> My guess is that you'd want to pass them to the handler, even if it
> immediately checked "if ($e instanceOf Error)" and used a different outpu=
t
> style.
>
>
I would *obviously* use this mechanic if it was available, I'm sure I'm not
the only one frustrated by the fact that it was not possible to cach fatal
errors and had to make stunts with apache's error log to do some complete
error auditing.But the fact is that it did NOT exist, so when you say
"other than the specific transition to PHP 7", well I think it is a big
deal and motivates the introduction of a new feature just for that.

Since set_exception_handler() is intended as a last-ditch "something's gone
> very wrong" function anyway, I think it receiving all Throwables makes
> sense, even if the BC break in your scenario is unfortunate.
>
>
Yes, it is very unfortunate. Not for the two lines of codes that required
to fix the issue (I wrapped it in a if ($e instanceof Exception) { ... }
block), but because of the feeling this gave me about the general attitude
towards the community.

I love PHP and I enjoy programming in it, and I always defend it when my
peers and collegues talk [really] bad about it. In the past I always felt
that BCs I got in my project were inevitable or caused by poor
implementation or not strictly adhering to the documentation guidelines.
This is why when we migrated from PHP 4 to 5.2 we rewrote most of the code.
It was a big effort, but we did it by checking the documentation all the
time, respecting deprecates, and so on. Because of this migrations from 5.2
to 5.3 and 5.3 to 5.6 were painless, close to zero changes or just bugs
catching. Everything was done by the book.

Now comes this breakage, but this time, funny to say, I felt stub in the
back. That's why after years of being in read-only mode I decided to speak
up.

I identified two possible indipendent change paths, which I'd like you to
consider separately. Both seem better than my initial idea of the boolean
flag in set_exception_handler():

1) Check the handler's signature compatibility before calling it.
Indeed there is no reason to fix a my_hnd(Exception $e) and not a
my_hnd(ExceptionA $e) where class ExceptionA extends Exception, so this
proposal would be to simply ignore the set_handler_exception()'s callback
if it is not compatible, or maybe even better issue an E_NOTICE or E_STRICT
in such case.
Although it doesn't completely solve the original problem, it mitigates it
and makes the engine look better, as it comes at very little cost and
*zero* BC breaks (code wouldn't work anyway).

2) Introduce set_throwable_handler() and leave set_exception_handler() as
it was in PHP 5.x, i.e. catching only Exception descendants.
Indeed this is what I expected to see, i.e. everything worked as before,
and if I want to exploit this new powerful feature I have to "enable" it by
change just one single line of code!
Unfortunately as PHP7 is released this would hironically be BC break itself
now, so I understand it is never going to happen, but I want you to
consider it nonetheless to at least introduce set_throwable_handler() even
if set_exception_handler() stays broken (maybe mitigated by the change 1).
Also think about the possible upcoming changes to the Throwable definition,
I saw in another thread that you are thinking about dropping
Throwable::getCode(), which would make things even worse, in case of
something as simple as:
set_exception_handler(function($e) { print $e->getCode(); });

Thank you for listening to me, I want to stress that I really appreciate
what you do even when I disagree with it.

Kind regards
Giovanni

--001a114a96b26ce0c705291fcdde--