Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:86097
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 74.125.82.46 as permitted sender)
MIME-Version: 1.0
References: <CAMZ7uYexNm6Fy42GYCUW9MPnFWEun=JFOPPSa+-mxOPL7hpaGg@mail.gmail.com>
 <55417693.5080909@gmail.com>
In-Reply-To: <55417693.5080909@gmail.com>
Date: Thu, 30 Apr 2015 15:07:09 +0000
Message-ID: <CACwt+Jc-G+oC2btdiKoz6y8tiiEfwHomPKGF=zx1aRBnGJqk-A@mail.gmail.com>
To: Stanislav Malyshev <smalyshev@gmail.com>, Olivier Garcia <oliviergarcia@php.net>, 
	PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=047d7beb9c8a250df10514f273d5
Subject: Re: [PHP-DEV] [RFC][VOTE] Improved Error Callback Mechanism
From: patrickallaert@php.net (Patrick ALLAERT)

--047d7beb9c8a250df10514f273d5
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Le jeu. 30 avr. 2015 =C3=A0 02:26, Stanislav Malyshev <smalyshev@gmail.com>=
 a
=C3=A9crit :

> Hi!
>
> > You can cast your vote on the Wiki [1] and the according patch is
> > available as a Pull Request [2].
> >
> > Vote will be open for two weeks, counting from today.
> >
>
> I think the idea of this RFC is nice but it needs a bit more work to
> make it really good and successful. See some of the notes below and more
> technical ones in the patch. I think it would be good not to rush it but
> have it for 7.1 with more time to develop it to maturity.
>

Thank you for your remarks on the PR, I will handle it ASAP.


> Reading the RFC, I'm not sure I fully understand what is the difference
> between E_HOOK_LOG, E_HOOK_DISPLAY and E_HOOK_PROCESS. The code just
> calls all of them in a sequence. What is the difference between them and
> when I use each of them? Especially not clear about E_HOOK_PROCESS - why
> I need it if I could just hook one of the other ones and do whatever I
> need to process the error?
>

E_HOOK_LOG:
Logging (typically using zend_append_error_hook)

E_HOOK_DISPLAY:
Displaying the error (typically using zend_prepend_error_hook + potentially
returning "FAILURE" so that it's the only hook treating the "display" part.
Or zend_clear_error_hook + zend_append_error_hook)

E_HOOK_PROCESS:
For stuff not fitting in the log / display category.
We can perfectly live without it, this was for completeness as well.
Better waiting for someone asking for it in order to avoid YAGNI?

E_HOOK_BAILOUT:
Responsible for bailing out.


> Also, there seems to be no link between E_HOOK_LOG and PG(log_errors)
> and E_HOOK_DISPLAY and PG(display_errors), so these will be called even
> if log/display globals set to disabled. So the code that implements
> something like custom error display would have to copy checks from PHP
> core code if they want the custom error not appear when display_errors
> is off (which they may want to I assume).
>

That is a very valid remark! The design choice we made, which is missing in
the RFC, is that we considered those flags as the one controlling the
"default behaviour" (read: specific to zend_error_display_cb /
zend_error_log_cb) and not the "general" one. We preferred favoring a
generic design which enables hook authors to discard those flags for
whatever good or bad reason at the price of duplicating some logic. For
example, Xdebug provides xdebug.force_display_errors in order to force the
display indepedently of PG(display_errors) [1]

Avoiding a general check around E_HOOK_LOG / E_HOOK_DISPLAY, we hope to
decrease the chance of having a "log" or "display" hook implemented in
E_HOOK_PROCESS for the reason that one want to ignore one of those flags.

We therefore have a preference for keeping the current design, but don't
mind changing if someone brings additional input on the subject. It's a
flexibility vs duplication choice.


> If you override bailout callback and request bailout, then the bailout
> would happen, but nothing in zend_error_bailout_cb() would - i.e. no
> error code setting, no HTTP response code, etc.
>
> Also, zend_error_bailout_cb() actually calls zend_bailout(), so it is
> not clear - should the bailout hook call zend_bailout() or return
> FAILURE? The comment in the code of php_error_cb() says "hook_result =3D=
=3D
> FAILURE means we must bail out" but the bailout does not actually happen
> there. Moreover, it is not clear when bailout hook is supposed to return
> FAILURE at all.
>

Bailout hooks are responsible for calling zend_bailout() (or implementing a
similar behaviour) AND return FAILURE in order to prevent more hooks of
that category to be run.

zend_bailout() could possibly be called if FAILURE is returned by a bailout
hook instead of by the hook itself, but then there is no way one can
implement zend_bailout differently.
We have no use case for that, so we can perfectly take that direction if
you think it's preferable.


> Also, I see a lot of php_get_module_initialized() checks inside the
> specific handlers - but if an extension overrides it, these checks are
> not performed, so the extension may still work with uninitialized data,
> which would be not good. Of course, extension writer can copy-paste all
> these checks but it's easy to forget and if they ever change it would
> also be easy to forget to update it.
>

Yes, those calls to php_get_module_initialized() replace the direct access
to module_initialized variable that were performed from within the original
php_error_cb.
Do you have something to propose here, like moving some part of the default
hooks directly into php_error_cb?

Regarding the copy-paste issue, which I fully agree with, note that the
current situation is worst than with the patch because if one want to
change, e.g., the display, it's the whole php_error_cb content (~240 lines)
that has to be copied (or to take inspiration on) vs. the corresponding
hook only (~50 lines for the most complex one: display) while letting all
the other parts intact.

Looking at xdebug_error_cb [2], there's a lot of copy-paste, including
usage of php_get_module_initialized() that can't be avoided for now.

Cheers,
Patrick

[1]
https://github.com/xdebug/xdebug/blob/fcbefa6ca55757d1aaec72511b71bbfc113e3=
3dc/xdebug_stack.c#L599
[2]
https://github.com/xdebug/xdebug/blob/fcbefa6ca55757d1aaec72511b71bbfc113e3=
3dc/xdebug_stack.c#L531

--047d7beb9c8a250df10514f273d5--