Hey Internals,
I'm working on the patch to change the error exception to
\ArgumentCountError instead of \Error when too few, or too many arguments
are passed in as discussed a couple of weeks ago.
To be BC with 7.0, this extends \TypeError, which is the exception
currently thrown when strict_types=1 for both userland and internal
functions.
I've run into an issue where things are inconsistent for internal functions.
For example, the array_* functions, like array_diff, still have a warning
thrown, regardless of strict_types:
php_error_docref(NULL, E_WARNING, "at least %d parameters are required, %d
given", req_args, ZEND_NUM_ARGS());
It seems that fixing this will be:
a) a mammoth task
b) a huge BC break (stuff that currently throws a warning in strict_types
mode will now throw an exception, similar BC break as we're implementing
for userland functions regardless of strict_types)
c) something we should have done for 7.0, and that we cannot change now.
We are currently heading in 7.1.0RC1 in two weeks, and I want this issue
resolved in its entirety before then.
What say you, internals?
- Davey
2016-08-19 15:05 GMT+02:00 Davey Shafik davey@php.net:
Hey Internals,
I'm working on the patch to change the error exception to
\ArgumentCountError instead of \Error when too few, or too many arguments
are passed in as discussed a couple of weeks ago.To be BC with 7.0, this extends \TypeError, which is the exception
currently thrown when strict_types=1 for both userland and internal
functions.I've run into an issue where things are inconsistent for internal functions.
For example, the array_* functions, like array_diff, still have a warning
thrown, regardless of strict_types:php_error_docref(NULL, E_WARNING, "at least %d parameters are required, %d
given", req_args, ZEND_NUM_ARGS());
This seems more like an oversight than anything else, I guess cases
with variable argument count may have some similar parameter parsing
approaches. I think it should just be changed to the exception and to
respect strict_types. Despite it being a BC break, I don't think that
many will care to notice since its error handling for one and second
of all, we did do that for a lot of things in 7.0, and I would assume
that most of the userland developers expect us to continue converting
into exceptions where reasonable, so the programs will hopefully
quickly adapt.
--
regards,
Kalle Sommer Nielsen
kalle@php.net
On Fri, Aug 19, 2016 at 10:27 PM, Kalle Sommer Nielsen kalle@php.net
wrote:
2016-08-19 15:05 GMT+02:00 Davey Shafik davey@php.net:
Hey Internals,
I'm working on the patch to change the error exception to
\ArgumentCountError instead of \Error when too few, or too many arguments
are passed in as discussed a couple of weeks ago.To be BC with 7.0, this extends \TypeError, which is the exception
currently thrown when strict_types=1 for both userland and internal
functions.I've run into an issue where things are inconsistent for internal
functions.For example, the array_* functions, like array_diff, still have a warning
thrown, regardless of strict_types:php_error_docref(NULL, E_WARNING, "at least %d parameters are required,
%d
given", req_args, ZEND_NUM_ARGS());This seems more like an oversight than anything else, I guess cases
with variable argument count may have some similar parameter parsing
approaches. I think it should just be changed to the exception and to
respect strict_types. Despite it being a BC break, I don't think that
many will care to notice since its error handling for one and second
of all, we did do that for a lot of things in 7.0, and I would assume
that most of the userland developers expect us to continue converting
into exceptions where reasonable, so the programs will hopefully
quickly adapt.
I have completed this as best as I can tell. All test failures also exist
on master without the patch.
https://github.com/php/php-src/pull/2092
The only potential issue is that the format of the warning message has
changed slightly from e.g.:
-Warning: array_diff(): at least 2 parameters are required, 0 given in %s
on line %d
+Warning: array_diff() expects at least 2 parameters, 0 given in %s on line
%d
I suspect there are other functions that are not array functions that emit
similar warnings (especially for functions that expect exact number of
arguments) that I missed, so if you know any of these, please let me know
and I'll update the patch.
If everyone is good with this I will merge down to PHP 7.1 for RC1 next
week.
- Davey
b) a huge BC break (stuff that currently throws a warning in
strict_types mode will now throw an exception, similar BC break as
we're implementing for userland functions regardless of strict_types)
IMO, that's not acceptable at all between 7.0 and 7.1
cheers,
Derick
b) a huge BC break (stuff that currently throws a warning in
strict_types mode will now throw an exception, similar BC break as
we're implementing for userland functions regardless of strict_types)IMO, that's not acceptable at all between 7.0 and 7.1
So… option C?
Bear in mind, this only affects people who have turned on strict_types,
and makes it consistent across the language. If we had implemented type
hints internally it would also have errored out in 7.0, but would be a
TypeError (which ArgumentCountError extends and therefore it's BC).
(FYI, we both voted against this RFC, and I'm not happy about it but trying
to at least make it not as bad :P)
- Davey