Le jeu. 30 avr. 2015 à 02:26, Stanislav Malyshev <smalyshev@gmail.com> a
écrit :
> 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 ==
> 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/fcbefa6ca55757d1aaec72511b71bbfc113e33dc/xdebug_stack.c#L599
[2]
https://github.com/xdebug/xdebug/blob/fcbefa6ca55757d1aaec72511b71bbfc113e33dc/xdebug_stack.c#L531
