Hello internals,
I want to start a discussion on an RFC to add a declare() statement to
convert all errors triggered within a file to exceptions.
Currently, the only way to handle notices/warnings in an exception-like
manner is via set_error_handler, (for example, example #1 on
https://www.php.net/errorexception) - however this has several
disadvantages (in particular, it cannot be safely used in libraries without
affecting other libraries). The declare() would only affect code on the
particular PHP file for which the declare was set to 1, in the same way
that strict_types only affects code on a particular PHP file. I have
listed error_exception, error_exceptions, and strict_errors as potential
names for the declare(), although I don't feel that any of them clearly and
succinctly describe what is happening here (if anyone has a better name I'm
certainly interested).
As I do not have wiki karma, I have posted a draft RFC here:
https://github.com/iggyvolz/php-rfcs/blob/master/error-exceptions.txt. A
partially complete patch is linked at the bottom of the draft (very much
based off the strict_types patch).
Thanks,
Katie Volz
I want to start a discussion on an RFC to add a declare() statement to
convert all errors triggered within a file to exceptions.
Hi Katie,
Personally, I'm not a fan of promoting messages to exceptions in this
way, because APIs designed to throw exceptions generally look rather
different from ones designed to warn and continue, so blindly converting
seems like putting a square peg in a round hole. However, I know people
do it already, so will give my thoughts on the principle.
My main concern is that it needs a tighter definition of "error", and
possibly a configurable one.
The snippet from the manual which you include in your draft RFC
references the current global error_reporting setting. This makes sense
with a global error handler, but less so for a per-file declare - it
means the caller can choose whether exceptions are thrown, somewhat
defeating the point. Perhaps the declare directive could take an error
level as its argument, e.g. declare(error_exceptions=E_ALL-E_NOTICE)?
Relatedly, you would need to define how this interacts with the @ (error
suppression or "shut up") operator - would that force the code to run
past a point that would otherwise throw an exception? Again, I think
that choice should be taken away from the caller, otherwise the author
needs to account for both modes, e.g.
declare(error_exceptions=E_WARNING);
try {
$fh = fopen($blah, $mode);
// if caller can suppress ErrorExceptions, we still need to check
if $fh is false
// ...
} catch ( ErrorException $e ) {
// the caller shouldn't actually care what happens, because we can
catch it here
}
Other than that, there's the usual awkwardness of declare - it has to be
set in each file, and new directives are errors in old versions of PHP.
That doesn't necessarily mean we shouldn't use it for now, though.
Regards,
--
Rowan Tommins (né Collins)
[IMSoP]
On Fri, May 22, 2020 at 12:56 PM Rowan Tommins rowan.collins@gmail.com
wrote:
Personally, I'm not a fan of promoting messages to exceptions in this
way, because APIs designed to throw exceptions generally look rather
different from ones designed to warn and continue, so blindly converting
seems like putting a square peg in a round hole. However, I know people
do it already, so will give my thoughts on the principle.
To this (and also to George's point) - I definitely agree that this isn't a
perfect solution. I see this as more of a "stopgap" for people who prefer
exception-style handling rather than having to do both.
My main concern is that it needs a tighter definition of "error", and
possibly a configurable one.The snippet from the manual which you include in your draft RFC
references the current global error_reporting setting. This makes sense
with a global error handler, but less so for a per-file declare - it
means the caller can choose whether exceptions are thrown, somewhat
defeating the point. Perhaps the declare directive could take an error
level as its argument, e.g. declare(error_exceptions=E_ALL-E_NOTICE)?
I hadn't thought putting the error types in the declare(). My only issue
with that is how that would interact with the error_reporting setting - for
example if I set my error_reporting to E_WARNING and error_exceptions to
E_ALL I'm not sure whether a notice would be thrown.
As far as a tighter definition of error - I've kept it loose in the draft
as I'm not sure how exactly to define errors which can and can't be
processed at runtime. For example, a compile error cannot be caught at
runtime, nor can (at least in my patch) warnings which are issued before
current_execute_data is populated (that is the object which I read to check
if the flag is set).
Relatedly, you would need to define how this interacts with the @ (error
suppression or "shut up") operator - would that force the code to run
past a point that would otherwise throw an exception? Again, I think
that choice should be taken away from the caller, otherwise the author
needs to account for both modes, e.g.declare(error_exceptions=E_WARNING);
try {
$fh = fopen($blah, $mode);
// if caller can suppress ErrorExceptions, we still need to check
if $fh is false
// ...
} catch ( ErrorException $e ) {
// the caller shouldn't actually care what happens, because we can
catch it here
}
The error suppression operator should behave as you've described it. In a
file with declare(), no warnings will ever be issued, meaning that a
suppression operator on a function defined with the flag (or, for that
matter, within such a file itself) will never have an effect. I will
clarify this in the RFC as well as add a test for that case.
I did a similar PoC last summer (see:
https://github.com/php/php-src/pull/4549)
where Nikita commented that a lot of those warnings could/should be
promoted
to error regardless, and this is what myself and a couple of other people
have
been during for the past year.
From what I can see your approach is slightly different than mine - this
does not manually change any function's error behaviour but removes the
warning system entirely. As we saw in
https://wiki.php.net/rfc/engine_warnings - there is still (and may always
be) significant pushback on moving errors to exceptions. As mentioned
above, I see this as a "stopgap" for authors who do not want to deal with
the error system in any way.
I hadn't thought putting the error types in the declare(). My only issue
with that is how that would interact with the error_reporting setting - for
example if I set my error_reporting toE_WARNINGand error_exceptions to
E_ALLI'm not sure whether a notice would be thrown.
In my mind error_reporting should make absolutely no difference. As you
say, this option would be for people who don't want to interact with the
current error system in any way, and that includes its global
configuration state.
For instance, imagine you wrote this:
declare(error_exceptions=E_WARNING);
$fh = fopen($filePath, 'r');
// code using $fh
The point of using the declare here would be to avoid checking if $fh is
false, since any errors opening the file would become exceptions. If
error_reporting(0) can turn off that exception, you'll have to check for
false anyway, defeating the purpose.
Regards,
--
Rowan Tommins (né Collins)
[IMSoP]
On Sun, May 24, 2020 at 7:29 PM Rowan Tommins rowan.collins@gmail.com
wrote:
In my mind error_reporting should make absolutely no difference. As you
say, this option would be for people who don't want to interact with the
current error system in any way, and that includes its global
configuration state.
My main concern for this was that it would make implementation more
difficult - however it turned out to be fairly straightforward.
One note on this - currently declare() statements do not allow constants or
expressions in them. I had to patch zend_compile.c to allow this, so this
will affect other declare statements as well (for example,
declare(ticks=E_ALL - 37) will be valid). Unknown constants or incorrect
types (ex: declare(error_exception=[]), declare(error_exception=E_ALL -
FOO_BAR)) will fail here.
Hello internals,
I want to start a discussion on an RFC to add a declare() statement to
convert all errors triggered within a file to exceptions.Currently, the only way to handle notices/warnings in an exception-like
manner is via set_error_handler, (for example, example #1 on
https://www.php.net/errorexception) - however this has several
disadvantages (in particular, it cannot be safely used in libraries without
affecting other libraries). The declare() would only affect code on the
particular PHP file for which the declare was set to 1, in the same way
that strict_types only affects code on a particular PHP file. I have
listed error_exception, error_exceptions, and strict_errors as potential
names for the declare(), although I don't feel that any of them clearly and
succinctly describe what is happening here (if anyone has a better name I'm
certainly interested).As I do not have wiki karma, I have posted a draft RFC here:
https://github.com/iggyvolz/php-rfcs/blob/master/error-exceptions.txt. A
partially complete patch is linked at the bottom of the draft (very much
based off the strict_types patch).Thanks,
Katie Volz
I did a similar PoC last summer (see:
https://github.com/php/php-src/pull/4549)
where Nikita commented that a lot of those warnings could/should be promoted
to error regardless, and this is what myself and a couple of other people
have
been during for the past year.
There are still extensions which didn't receive this treatment mostly due
to lack
of time and hands, so if you want to join on this feel free to send a PR to
php-src
Due to the work already done, I personally see less interest in such a
declare.
Mostly because doing a promotion of these warnings, which are usually tied
in to a failure return value such as null or false, allows us in some
instance to
"clean" up the return value, but only if it's done on the extension
level/in core.
Now I do acknowledge the number of E_WARNINGs still present on master is
large, but I'd personally prefer to focus on making the language better for
everyone and let legit warnings be warnings mostly due to the legacy
behaviour
for I/O operations.
Best regards
George P. Banyard
Hello internals,
I want to start a discussion on an RFC to add a declare() statement to
convert all errors triggered within a file to exceptions.
Hi Katie/Iggy,
I'm not sure this is the right thing to do.
Having internal code just throw generic ErrorExceptions does not get
PHP to a place where it is possible for users to listen for individual
error types, which I think is the biggest problem with the current
situation.
I won't duplicate my message from another thread, but I think there is
an alternative path: https://news-web.php.net/php.internals/110315
The error handler cannot be set in library code without affecting other libraries
We could also just make the default error handler convert all
errors/warnings to exceptions.....I've been doing that for my error
handlers for years, and I believe it's the right thing to do for 95%+
of applications. Though it does require some use of the silence
operator, which apparently some people find disconcerting.
cheers
Dan
Ack