Hello,
Voting has opened on the RFC to change most conditions in extensions that raise E_ERROR or E_RECOVERABLE_ERROR to throw an instance of Error instead.
RFC: https://wiki.php.net/rfc/throw_error_in_extensions https://wiki.php.net/rfc/throw_error_in_extensions
PR: https://github.com/php/php-src/pull/1942 https://github.com/php/php-src/pull/1942
Aaron Piotrowski
hi,
Hello,
Voting has opened on the RFC to change most conditions in extensions that raise
E_ERRORorE_RECOVERABLE_ERRORto throw an instance of Error instead.RFC: https://wiki.php.net/rfc/throw_error_in_extensions https://wiki.php.net/rfc/throw_error_in_extensions
PR: https://github.com/php/php-src/pull/1942 https://github.com/php/php-src/pull/1942
I like the idea.
It should be 2/3 tho'.
Also "Generally none, though it is possible some exceptions thrown
could be unintentionally caught by code written for PHP 7. However, it
is rare for Errorexceptions to be caught outside of cleanup or
logging, so catching these exceptions is likely desirable over a fatal
error." is a BC break, not sure there are codes out there that expects
or may behave (more) badly with this than with a fatal error.
Cheers,
Pierre
@pierrejoye | http://www.libgd.org
Hi!
Voting has opened on the RFC to change most conditions in extensions that raise
E_ERRORorE_RECOVERABLE_ERRORto throw an instance of Error instead.RFC: https://wiki.php.net/rfc/throw_error_in_extensions https://wiki.php.net/rfc/throw_error_in_extensions
PR: https://github.com/php/php-src/pull/1942 https://github.com/php/php-src/pull/1942
Isn't there a case that php_error(E_ERROR) does not return? At least it
was in 5.x, I'm not sure if that didn't change. If so, we need to be
very careful here - some code may make assumptions about the things
because of previous E_ERROR conditions, and if zend_throw_error returns
where php_error didn't there might be subtle and dangerous bugs.
--
Stas Malyshev
smalyshev@gmail.com
I like the idea.
It should be 2/3 tho'.
Why?
It's not a language change, so doesn't seem to meet the criteria for
needing a 2/3 pass rate.
For the record, I'm beginning to think the RFC process should probably
be slightly more orchestrated, and RFCs should have a "pre-vote"
announcement at least one week before the vote actually opens, when
the RFC author thinks the discussion of the RFC is complete.
This point would be the time for the implementations full impact on
the PHP engine to be analyzed, and also when the final voting choice
can be discussed/challenged before the voting is actually open.
cheers
Dan
I like the idea.
It should be 2/3 tho'.
Why?
It's not a language change, so doesn't seem to meet the criteria for
needing a 2/3 pass rate.
It is changing the core like ext/standard and other core functions.
It's not a language change, so doesn't seem to meet the criteria for
needing a 2/3 pass rate.It is changing the core like ext/standard and other core functions.
That's true, but doesn't appear to be the required test. From the Voting RFC:
a feature affecting the language itself (new syntax for example) will be considered as 'accepted'
if it wins a 2/3 of the votes. Other RFCs require 50% + 1 votes to get 'accepted'.
Although the core functions ship as PHP they aren't part of the
language, so 50% + 1 seems to appropriate.
cheers
Dan
Hi Dan,
For the record, I'm beginning to think the RFC process should probably
be slightly more orchestrated, and RFCs should have a "pre-vote"
announcement at least one week before the vote actually opens, when
the RFC author thinks the discussion of the RFC is complete.This point would be the time for the implementations full impact on
the PHP engine to be analyzed, and also when the final voting choice
can be discussed/challenged before the voting is actually open.
Sorry if this RFC was a little rushed. I originally hoped to make these changes for 7, then the original PR got pushed off to 7.1 and forgotten about until it was almost too late even for 7.1.
I agree that a pre-vote phase could be useful, as it may help bring more attention to an RFC before voting actually opens. There seems to be a pattern of issues with RFCs not being discussed until voting has begun. The pre-vote phase could replace one week of the discussion period, so an RFC would still only need a minimum of two weeks between announcement and voting.
Cheers!
Aaron Piotrowski
Hi,
Hello,
Voting has opened on the RFC to change most conditions in extensions that
raiseE_ERRORorE_RECOVERABLE_ERRORto throw an instance of Error instead.RFC: https://wiki.php.net/rfc/throw_error_in_extensions <
https://wiki.php.net/rfc/throw_error_in_extensions>;
PR: https://github.com/php/php-src/pull/1942 <
https://github.com/php/php-src/pull/1942>;
Just noticed the openssl case in X509_digest and it's obviously oversight
by whoever added that bit because it should be warning as it's for all
other similar fails. I'm going to change it to warning to make it
consistent.
In general I agree with the idea but the patch should be a bit more
sensible and considers consistency with other errors in the extension. It
should be also reviewed by all active maintainers or regular contributors
to the changed extensions before it gets merged so it might be a bit late
for 7.1
Cheers
Jakub
Hi Jakub,
Hi,
Just noticed the openssl case in X509_digest and it's obviously oversight
by whoever added that bit because it should be warning as it's for all
other similar fails. I'm going to change it to warning to make it
consistent.
I noticed most others were warnings in openssl, but I did not want to make assumptions about what level an error should be. If an error was E_ERROR, I assumed there was a reason it was fatal. If you change it to a warning I'll be sure not to overwrite this when merging the patch.
In general I agree with the idea but the patch should be a bit more
sensible and considers consistency with other errors in the extension. It
should be also reviewed by all active maintainers or regular contributors
to the changed extensions before it gets merged so it might be a bit late
for 7.1
I agree, either the maintainers or someone very familiar with each extension should examine the changes before merging. Note that this patch is only meant to allow catching and handling of otherwise fatal errors, not to modify overall error handling in each extension. I would rather individual extension maintainers make decisions on error levels.
Thanks!
Aaron Piotrowski
Hello,
Voting has opened on the RFC to change most conditions in extensions that raise
E_ERRORorE_RECOVERABLE_ERRORto throw an instance of Error instead.RFC: https://wiki.php.net/rfc/throw_error_in_extensions https://wiki.php.net/rfc/throw_error_in_extensions
PR: https://github.com/php/php-src/pull/1942 https://github.com/php/php-src/pull/1942Aaron Piotrowski
Hi Aaron,
I was someone who spent time on the RFC template to try and get better
quality and to capture more information about each RFC. I think your
RFC needs a lot more content before going to the vote.
Chris
Hi Chris,
Hi Aaron,
I was someone who spent time on the RFC template to try and get better
quality and to capture more information about each RFC. I think your
RFC needs a lot more content before going to the vote.Chris
It was unclear if these changes even needed a formal RFC, but the RMs felt it would be better to have an RFC to ensure extension maintainers and others were aware of and agreed with the changes to be made. The RFC was short because I felt there wasn't much to say. However, I have added to the RFC a list of extensions and under what conditions an instance of Error will be thrown instead of a fatal or recoverable fatal error. Hopefully this is along the lines of what you were looking for.
If possible, I would still like each extension maintainer to take a look at the changes made to ensure I have not missed cleaning up cases where bail-out behavior was being relied upon.
Cheers!
Aaron Piotrowski