Everything

php

Incorrect Argument Count Error

php.internals

Davey

10 years ago

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

Kalle Sommer Nielsen

10 years ago

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

Davey

10 years ago

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

Derick Rethans

10 years ago

On Fri, 19 Aug 2016, Davey Shafik wrote:

> 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

Davey

10 years ago

On Mon, Aug 22, 2016 at 5:59 PM, Derick Rethans <derick@php.net> wrote:

> On Fri, 19 Aug 2016, Davey Shafik wrote:
>
> > 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