# Incorrect Argument Count Error - Thread: https://externals.io/message/95308 - Original archive: https://news-web.php.net/php.internals/95308 - Started by Davey Shafik on 2016-08-19T13:05:19+00:00 - Messages: 5 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #95308 — Davey Shafik — 2016-08-19T13:05:19+00:00 URL: https://externals.io/message/95308#95308 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 --- ## #95312 — Kalle Sommer Nielsen — 2016-08-19T14:27:56+00:00 URL: https://externals.io/message/95308#95312 · In reply to: #95308 2016-08-19 15:05 GMT+02:00 Davey Shafik : > 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 --- ## #95363 — Davey Shafik — 2016-08-22T08:18:33+00:00 URL: https://externals.io/message/95308#95363 · In reply to: #95312 On Fri, Aug 19, 2016 at 10:27 PM, Kalle Sommer Nielsen wrote: > 2016-08-19 15:05 GMT+02:00 Davey Shafik : > > […] > functions. > > […] > %d > > […] > > 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 --- ## #95365 — Derick Rethans — 2016-08-22T09:59:33+00:00 URL: https://externals.io/message/95308#95365 · In reply to: #95308 > 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 --- ## #95366 — Davey Shafik — 2016-08-22T10:11:11+00:00 URL: https://externals.io/message/95308#95366 · In reply to: #95365 > > > > […] > > 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