Le jeu. 30 avr. 2015 à 02:26, Stanislav Malyshev <smalys...@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
