Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » Reverting "Too Few Arguments Exception" RFC »

Reverting "Too Few Arguments Exception" RFC

php.internals

Davey

10 years ago
Hi all, Given this thread: http://externals.io/thread/233 I'm not happy with the state of this going into RC1 next week, and without changes (such as the patch I provided), I would like to revert this change and leave it for 7.2. My patch will _retain_ BC for internal functions with non strict_types (except for the error message, which can be reconciled), and for functions that previously threw a TypeError, ArgumentCountError is a subclass so BC is preserved there also. The issue is that the array functions that do this argument count checking themselves and still issue a warning, regardless of strict_types. We can leave the original behavior for array functions, but they then differ from other internals functions. It is a BC break for userland functions (as per the RFC), throwing an ArgumentCountError regardless of strict_types. At this point, we _must_ come to consensus by Monday to get it into RC1 (if there are changes needed) or we should remove it from 7.1. Also, I would like someone more experienced to review my patch. - Davey

Nikita Popov

10 years ago
On Aug 24, 2016 9:55 AM, "Davey Shafik" <davey@php.net> wrote:
> > Hi all, > > Given this thread: http://externals.io/thread/233 > > I'm not happy with the state of this going into RC1 next week, and without > changes (such as the patch I provided), I would like to revert this change > and leave it for 7.2. > > My patch will _retain_ BC for internal functions with non strict_types > (except for the error message, which can be reconciled), and for functions > that previously threw a TypeError, ArgumentCountError is a subclass so BC > is preserved there also. > > The issue is that the array functions that do this argument count checking > themselves and still issue a warning, regardless of strict_types. > > We can leave the original behavior for array functions, but they then > differ from other internals functions. > > It is a BC break for userland functions (as per the RFC), throwing an > ArgumentCountError regardless of strict_types. > > At this point, we _must_ come to consensus by Monday to get it into RC1
(if
> there are changes needed) or we should remove it from 7.1. Also, I would > like someone more experienced to review my patch. > > - Davey
I have some trouble understanding what the issue is here. The mentioned RFC affects only userland functions, so the non-standard behavior of some array functions shouldn't matter. Personally I am entirely indifferent as to what exception gets thrown when too little arguments are passed -- this is a type of error that should not be caught by anything but catch-all handlers. Inability to provide a more specific exception should not be a blocker for this, especially as this is beyond the scope of the original RFC. Nikita

Davey

10 years ago
Nikita, It is _always_ better to have more specific exceptions when it makes sense. In this case, we need to do it to maintain BC. One use-case that wasn't covered in the RFC, is the use of argument unpacking with too few elements in the array/traversable — whereby you would want to guard specifically for this condition, and handle it in a non-generic way. WRT to the internal functions, you were the person to bring up the behavior of internal functions:
> Problem: We already use TypeError for this for internal functions. If we
want to introduce an extra exception for this, lets use it for internal functions as well. In that case we should probably go with something that applies not just to too few arguments, but also to too many. During which I noticed the discrepancy with the array functions. - Davey On Thu, Aug 25, 2016 at 12:45 AM, Nikita Popov <nikita.ppv@gmail.com> wrote:

Christoph Becker

10 years ago
On 24.08.2016 at 18:45, Nikita Popov wrote:
> On Aug 24, 2016 9:55 AM, "Davey Shafik" <davey@php.net> wrote: >> >> Given this thread: http://externals.io/thread/233 >> >> I'm not happy with the state of this going into RC1 next week, and without >> changes (such as the patch I provided), I would like to revert this change >> and leave it for 7.2. >> >> My patch will _retain_ BC for internal functions with non strict_types >> (except for the error message, which can be reconciled), and for functions >> that previously threw a TypeError, ArgumentCountError is a subclass so BC >> is preserved there also. >> >> The issue is that the array functions that do this argument count checking >> themselves and still issue a warning, regardless of strict_types. >> >> We can leave the original behavior for array functions, but they then >> differ from other internals functions. >> >> It is a BC break for userland functions (as per the RFC), throwing an >> ArgumentCountError regardless of strict_types. >> >> At this point, we _must_ come to consensus by Monday to get it into RC1 > (if >> there are changes needed) or we should remove it from 7.1. Also, I would >> like someone more experienced to review my patch. > > I have some trouble understanding what the issue is here. The mentioned RFC > affects only userland functions, so the non-standard behavior of some array > functions shouldn't matter. > > Personally I am entirely indifferent as to what exception gets thrown when > too little arguments are passed -- this is a type of error that should not > be caught by anything but catch-all handlers. > > Inability to provide a more specific exception should not be a blocker for > this, especially as this is beyond the scope of the original RFC.
Indeed, the RFC explicitly claims: | Behavior of internal functions is not going to be changed. The RFC has been accepted, so I don't see a reason to revert the respective changes (even though I'm still not happy with this change happening in a minor version).
-- Christoph M. Becker

Davey

10 years ago
On Thu, Aug 25, 2016 at 8:12 PM, Christoph M. Becker <cmbecker69@gmx.de> wrote:
> On 24.08.2016 at 18:45, Nikita Popov wrote: > > > On Aug 24, 2016 9:55 AM, "Davey Shafik" <davey@php.net> wrote: > >> > >> Given this thread: http://externals.io/thread/233 > >> > >> I'm not happy with the state of this going into RC1 next week, and > without > >> changes (such as the patch I provided), I would like to revert this > change > >> and leave it for 7.2. > >> > >> My patch will _retain_ BC for internal functions with non strict_types > >> (except for the error message, which can be reconciled), and for > functions > >> that previously threw a TypeError, ArgumentCountError is a subclass so > BC > >> is preserved there also. > >> > >> The issue is that the array functions that do this argument count > checking > >> themselves and still issue a warning, regardless of strict_types. > >> > >> We can leave the original behavior for array functions, but they then > >> differ from other internals functions. > >> > >> It is a BC break for userland functions (as per the RFC), throwing an > >> ArgumentCountError regardless of strict_types. > >> > >> At this point, we _must_ come to consensus by Monday to get it into RC1 > > (if > >> there are changes needed) or we should remove it from 7.1. Also, I would > >> like someone more experienced to review my patch. > > > > I have some trouble understanding what the issue is here. The mentioned > RFC > > affects only userland functions, so the non-standard behavior of some > array > > functions shouldn't matter. > > > > Personally I am entirely indifferent as to what exception gets thrown > when > > too little arguments are passed -- this is a type of error that should > not > > be caught by anything but catch-all handlers. > > > > Inability to provide a more specific exception should not be a blocker > for > > this, especially as this is beyond the scope of the original RFC. > > Indeed, the RFC explicitly claims: > > | Behavior of internal functions is not going to be changed. >
This is correct for functions that had the correct behavior before (everything but array functions). I can remove that part of the change — but if we're going to change it, doing in 7.1 rather than > 7.1 would be best. - Davey

Christoph Becker

10 years ago
On 25.08.2016 at 18:37, Davey Shafik wrote:
> On Thu, Aug 25, 2016 at 8:12 PM, Christoph M. Becker <cmbecker69@gmx.de> > wrote: > >> Indeed, the RFC explicitly claims: >> >> | Behavior of internal functions is not going to be changed. > > This is correct for functions that had the correct behavior before > (everything but array functions). > > I can remove that part of the change — but if we're going to change it, > doing in 7.1 rather than > 7.1 would be best.
AIUI, the actual implementation of the too_few_args RFC does what it says, namely to not affect the behavior of internal function at all. The issue you've raised is that we have introduced an inconsistency, because some internal functions (e.g. array_diff) behave differently. If that is so, reverting the too_few_args RFC is out of question, because not changing the behavior of internal functions was intentional, and I assume that everybody who voted on the RFC was aware of that. The inconsistency between internal functions wrt. to too few/many arguments in strict_types mode is not related to the too_few_args RFC at all – we have this as of PHP 7. Fixing this inconsistency as soon as possible might be desirable, but it seems to me that is not even possible in the general case, as not all relevant code is under our control (think of PECL and even "private" extensions). So, even we "fix" the behavior of all bundled internal functions, we still can't claim that *all* functions throw an exception when called with too few or too many arguments in strict_types mode (besides that we easily might miss a few cases). If the docs already make this claim, the docs should be fixed. Finally, I wonder why array_diff(), for instance, even has an explicit check for ZEND_NUM_ARGS() and for Z_TYPE() != IS_ARRAY instead of properly invoking zend_parse_parameters() with "aa+" instead of "+" in the first place? Maybe I'm missing something, but otherwise I would suggest to fix that altogether instead of piecemeal, even if that has to wait until 8.0.
-- Christoph M. Becker

Levi Morrison

10 years ago
On Fri, Aug 26, 2016 at 4:53 AM, Christoph M. Becker <cmbecker69@gmx.de> wrote:
> On 25.08.2016 at 18:37, Davey Shafik wrote: > >> On Thu, Aug 25, 2016 at 8:12 PM, Christoph M. Becker <cmbecker69@gmx.de> >> wrote: >> >>> Indeed, the RFC explicitly claims: >>> >>> | Behavior of internal functions is not going to be changed. >> >> This is correct for functions that had the correct behavior before >> (everything but array functions). >> >> I can remove that part of the change — but if we're going to change it, >> doing in 7.1 rather than > 7.1 would be best. > > AIUI, the actual implementation of the too_few_args RFC does what it > says, namely to not affect the behavior of internal function at all. > The issue you've raised is that we have introduced an inconsistency, > because some internal functions (e.g. array_diff) behave differently. > > If that is so, reverting the too_few_args RFC is out of question, > because not changing the behavior of internal functions was intentional, > and I assume that everybody who voted on the RFC was aware of that. > > The inconsistency between internal functions wrt. to too few/many > arguments in strict_types mode is not related to the too_few_args RFC at > all – we have this as of PHP 7. Fixing this inconsistency as soon as > possible might be desirable, but it seems to me that is not even > possible in the general case, as not all relevant code is under our > control (think of PECL and even "private" extensions). > > So, even we "fix" the behavior of all bundled internal functions, we > still can't claim that *all* functions throw an exception when called > with too few or too many arguments in strict_types mode (besides that we > easily might miss a few cases). If the docs already make this claim, > the docs should be fixed. > > Finally, I wonder why array_diff(), for instance, even has an explicit > check for ZEND_NUM_ARGS() and for Z_TYPE() != IS_ARRAY instead of > properly invoking zend_parse_parameters() with "aa+" instead of "+" in > the first place? Maybe I'm missing something, but otherwise I would > suggest to fix that altogether instead of piecemeal, even if that has to > wait until 8.0. > > -- > Christoph M. Becker > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php
Well, its behavior does differ with a single array: it will preserve keys in this case. If more than one array is passed it will numerically index the keys starting at zero. This is the only reason I can think of.

Christoph Becker

10 years ago
On 26.08.2016 at 16:48, Levi Morrison wrote:
> On Fri, Aug 26, 2016 at 4:53 AM, Christoph M. Becker <cmbecker69@gmx.de> wrote: > >> Finally, I wonder why array_diff(), for instance, even has an explicit >> check for ZEND_NUM_ARGS() and for Z_TYPE() != IS_ARRAY instead of >> properly invoking zend_parse_parameters() with "aa+" instead of "+" in >> the first place? Maybe I'm missing something, but otherwise I would >> suggest to fix that altogether instead of piecemeal, even if that has to >> wait until 8.0. > > Well, its behavior does differ with a single array: it will preserve > keys in this case. If more than one array is passed it will > numerically index the keys starting at zero. This is the only reason I > can think of.
Indeed, I've overlooked that array_diff() accepts a single array. So, ZPP could use "aa*". My point is that it's better to rely on common checks (and respective errors), than to have individual checks for each (group of) function(s). If that had already been the case, we wouldn't need to have this discussion. :-)
-- Christoph M. Becker

Davey

10 years ago
FTR, I've created a second PR, without the changes to the array functions: https://github.com/php/php-src/pull/2100 This way we can go either way. - Davey On Sat, Aug 27, 2016 at 1:57 AM, Christoph M. Becker <cmbecker69@gmx.de> wrote: