Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [RFC][Vote] Throw Error in Extensions »

[RFC][Vote] Throw Error in Extensions

php.internals

Aaron Piotrowski

10 years ago
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

Pierre Joye

10 years ago
hi, On Mon, Jun 27, 2016 at 10:17 PM, Aaron Piotrowski <aaron@trowski.com> wrote:
> 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>
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

Stas Malyshev

10 years ago
Hi!
>> 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>
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

Dan Ackroyd

10 years ago
On 28 June 2016 at 03:36, Pierre Joye <pierre.php@gmail.com> wrote:
> > 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

Pierre Joye

10 years ago
On Jun 30, 2016 3:41 AM, "Dan Ackroyd" <danack@basereality.com> wrote:
> > On 28 June 2016 at 03:36, Pierre Joye <pierre.php@gmail.com> wrote: > > > > 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.

Dan Ackroyd

10 years ago
On 30 June 2016 at 01:32, Pierre Joye <pierre.php@gmail.com> wrote:
>> 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

Aaron Piotrowski

10 years ago
Hi Dan,
> On Jun 29, 2016, at 3:41 PM, Dan Ackroyd <danack@basereality.com> wrote: > > 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

Jakub Zelenka

10 years ago
Hi, On Mon, Jun 27, 2016 at 4:17 PM, Aaron Piotrowski <aaron@trowski.com> wrote:
> 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> > >
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

Aaron Piotrowski

10 years ago
Hi Jakub,
> On Jun 28, 2016, at 12:28 PM, Jakub Zelenka <bukka@php.net> wrote: > > 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

Christopher Jones

10 years ago
On 28/06/2016 1:17 AM, Aaron Piotrowski wrote:
> 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 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
-- http://twitter.com/ghrd

Aaron Piotrowski

10 years ago
Hi Chris,
> On Jun 30, 2016, at 6:15 AM, Christopher Jones <christopher.jones@oracle.com> wrote: > > 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

Pascal MARTIN

10 years ago
Le 27/06/2016 17:17, Aaron Piotrowski a écrit :
> 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.
Hi, At AFUP, we would be +1 on this RFC, as it fits well into the path started with PHP 7.0 Thanks for your work on this!
-- Pascal MARTIN, AFUP - French UG http://php-internals@afup.org