Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:86049
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.220.48 as permitted sender)
Message-ID: <55417693.5080909@gmail.com>
Date: Wed, 29 Apr 2015 17:25:55 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Thunderbird/31.6.0
MIME-Version: 1.0
To: Olivier Garcia <oliviergarcia@php.net>, 
 PHP Internals <internals@lists.php.net>
References: <CAMZ7uYexNm6Fy42GYCUW9MPnFWEun=JFOPPSa+-mxOPL7hpaGg@mail.gmail.com>
In-Reply-To: <CAMZ7uYexNm6Fy42GYCUW9MPnFWEun=JFOPPSa+-mxOPL7hpaGg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] [RFC][VOTE] Improved Error Callback Mechanism
From: smalyshev@gmail.com (Stanislav Malyshev)

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.

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?

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).

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.

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.
-- 
Stas Malyshev
smalyshev@gmail.com