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
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
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
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
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
On Thu, Aug 25, 2016 at 8:12 PM, Christoph M. Becker cmbecker69@gmx.de
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
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
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--
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.
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
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:
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