Everything

php

Change to Too Few Args Exception RFC

php.internals

Davey

10 years ago

Hey all,

I know this is a little late in the process, but it's something I've
noticed while prepping some content around 7.1.

This RFC: https://wiki.php.net/rfc/too_few_args

Passed, and has been implemented, but I feel that throwing an `\Error`
exception is a mistake. I think we should another more concrete exception
class for this error:

`\TooFewArgumentsError extends \Error`

A use case where this may trivially occur is where you are using argument
unpacking and the unpacked array is too small. Writing this, just looks bad:

try {
foo(… $args);
} catch (\Error $e) { }

compared:

try {
foo(… $args);
} catch (\TooFewArgumentsError $e) { }

Thoughts? Dmitry?

Given the tiny change this is, and that is backwards compatible with the
original RFC, I would like to add this to 7.1 for beta3.

I think I can make this change myself.

- Davey

Nikita Popov

10 years ago

On Sat, Aug 6, 2016 at 4:26 AM, Davey Shafik <davey@php.net> wrote:

> Hey all,
>
> I know this is a little late in the process, but it's something I've
> noticed while prepping some content around 7.1.
>
> This RFC: https://wiki.php.net/rfc/too_few_args
>
> Passed, and has been implemented, but I feel that throwing an `\Error`
> exception is a mistake. I think we should another more concrete exception
> class for this error:
>
> `\TooFewArgumentsError extends \Error`
>
> A use case where this may trivially occur is where you are using argument
> unpacking and the unpacked array is too small. Writing this, just looks
> bad:
>
> try {
>    foo(… $args);
> } catch (\Error $e) { }
>
> compared:
>
> try {
>    foo(… $args);
> } catch (\TooFewArgumentsError $e) { }
>
> Thoughts? Dmitry?
>
> Given the tiny change this is, and that is backwards compatible with the
> original RFC, I would like to add this to 7.1 for beta3.
>
> I think I can make this change myself.
>
> - Davey
>

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.

Nikita

Davey

10 years ago

On Sat, Aug 6, 2016 at 2:34 AM, Nikita Popov <nikita.ppv@gmail.com> wrote:

> On Sat, Aug 6, 2016 at 4:26 AM, Davey Shafik <davey@php.net> wrote:
>
>> Hey all,
>>
>> I know this is a little late in the process, but it's something I've
>> noticed while prepping some content around 7.1.
>>
>> This RFC: https://wiki.php.net/rfc/too_few_args
>>
>> Passed, and has been implemented, but I feel that throwing an `\Error`
>> exception is a mistake. I think we should another more concrete exception
>> class for this error:
>>
>> `\TooFewArgumentsError extends \Error`
>>
>> A use case where this may trivially occur is where you are using argument
>> unpacking and the unpacked array is too small. Writing this, just looks
>> bad:
>>
>> try {
>>    foo(… $args);
>> } catch (\Error $e) { }
>>
>> compared:
>>
>> try {
>>    foo(… $args);
>> } catch (\TooFewArgumentsError $e) { }
>>
>> Thoughts? Dmitry?
>>
>> Given the tiny change this is, and that is backwards compatible with the
>> original RFC, I would like to add this to 7.1 for beta3.
>>
>> I think I can make this change myself.
>>
>> - Davey
>>
>
> 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.
>

Are you saying that in PHP 7.0, if you call an internal function with too
few, or too many arguments it will emit a TypeError exception?

Can you provide examples? It's obviously not every function, e.g. fopen()
emits a Warning if you forget the second arg.

With that in mind, to make this BC, TooFewArguments would have to extend
TypeError which isn't great.  Maybe, "ArgumentError" and the message will
make it clear if it's too few or too many?

- Davey

Niklas Keller

10 years ago

2016-08-06 11:53 GMT+02:00 Davey Shafik <davey@php.net>:

> On Sat, Aug 6, 2016 at 2:34 AM, Nikita Popov <nikita.ppv@gmail.com> wrote:
>
> > On Sat, Aug 6, 2016 at 4:26 AM, Davey Shafik <davey@php.net> wrote:
> >
> >> Hey all,
> >>
> >> I know this is a little late in the process, but it's something I've
> >> noticed while prepping some content around 7.1.
> >>
> >> This RFC: https://wiki.php.net/rfc/too_few_args
> >>
> >> Passed, and has been implemented, but I feel that throwing an `\Error`
> >> exception is a mistake. I think we should another more concrete
> exception
> >> class for this error:
> >>
> >> `\TooFewArgumentsError extends \Error`
> >>
> >> A use case where this may trivially occur is where you are using
> argument
> >> unpacking and the unpacked array is too small. Writing this, just looks
> >> bad:
> >>
> >> try {
> >>    foo(… $args);
> >> } catch (\Error $e) { }
> >>
> >> compared:
> >>
> >> try {
> >>    foo(… $args);
> >> } catch (\TooFewArgumentsError $e) { }
> >>
> >> Thoughts? Dmitry?
> >>
> >> Given the tiny change this is, and that is backwards compatible with the
> >> original RFC, I would like to add this to 7.1 for beta3.
> >>
> >> I think I can make this change myself.
> >>
> >> - Davey
> >>
> >
> > 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.
> >
>
> Are you saying that in PHP 7.0, if you call an internal function with too
> few, or too many arguments it will emit a TypeError exception?
>
> Can you provide examples? It's obviously not every function, e.g. fopen()
> emits a Warning if you forget the second arg.
>
> With that in mind, to make this BC, TooFewArguments would have to extend
> TypeError which isn't great.  Maybe, "ArgumentError" and the message will
> make it clear if it's too few or too many?
>
> - Davey
>

I don't like "ArgumentError", how about "WrongArgumentCountError"? Maybe
also "WrongArgumentsError".

Regards, Niklas

Thomas Punt

10 years ago

Hi!

> From: me@kelunik.com>
> I don't like "ArgumentError", how about "WrongArgumentCountError"? Maybe
> also "WrongArgumentsError".

I'd favour simply ArgumentCountError. No need to prepend a "wrong" to theerror class name - we can already guess it's wrong because it has resulted inan error :)
-Tom

Davey

10 years ago

AFAICT, to make this change, I'd have to modify:

ZEND_API ZEND_COLD void zend_internal_type_error(zend_bool throw_exception,
const char *format, ...) /* {{{ */

To be:

ZEND_API ZEND_COLD void zend_internal_type_error(zend_bool throw_exception,
zend_class_entry *zend_error, const char *format, …) /* {{{ */

Which… would be a BC break for extensions perhaps?

There is also zend_type_error, I'm not sure how that relates, I don't think
it gets used in this case. Or, I could add
zend_(internal_?)argument_count_error and use that instead.

Thoughts?

- Davey


On Sat, Aug 6, 2016 at 4:10 AM, Thomas Punt <tpunt@hotmail.co.uk> wrote:

Davey

10 years ago

Hey all,

Can we please reach a resolution on this?

- Davey

On Sun, Aug 7, 2016 at 9:04 AM, Davey Shafik <davey@php.net> wrote:

Nikita Popov

10 years ago

On Sat, Aug 6, 2016 at 11:53 AM, Davey Shafik <davey@php.net> wrote:

> On Sat, Aug 6, 2016 at 2:34 AM, Nikita Popov <nikita.ppv@gmail.com> wrote:
>
>> On Sat, Aug 6, 2016 at 4:26 AM, Davey Shafik <davey@php.net> wrote:
>>
>>> Hey all,
>>>
>>> I know this is a little late in the process, but it's something I've
>>> noticed while prepping some content around 7.1.
>>>
>>> This RFC: https://wiki.php.net/rfc/too_few_args
>>>
>>> Passed, and has been implemented, but I feel that throwing an `\Error`
>>> exception is a mistake. I think we should another more concrete exception
>>> class for this error:
>>>
>>> `\TooFewArgumentsError extends \Error`
>>>
>>> A use case where this may trivially occur is where you are using argument
>>> unpacking and the unpacked array is too small. Writing this, just looks
>>> bad:
>>>
>>> try {
>>>    foo(… $args);
>>> } catch (\Error $e) { }
>>>
>>> compared:
>>>
>>> try {
>>>    foo(… $args);
>>> } catch (\TooFewArgumentsError $e) { }
>>>
>>> Thoughts? Dmitry?
>>>
>>> Given the tiny change this is, and that is backwards compatible with the
>>> original RFC, I would like to add this to 7.1 for beta3.
>>>
>>> I think I can make this change myself.
>>>
>>> - Davey
>>>
>>
>> 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.
>>
>
> Are you saying that in PHP 7.0, if you call an internal function with too
> few, or too many arguments it will emit a TypeError exception?
>
> Can you provide examples? It's obviously not every function, e.g. fopen()
> emits a Warning if you forget the second arg.
>
> With that in mind, to make this BC, TooFewArguments would have to extend
> TypeError which isn't great.  Maybe, "ArgumentError" and the message will
> make it clear if it's too few or too many?
>

Forgot to mention, this is only for strict_types=1, or if zpp explicitly
opts to throw (e.g. constructors).

Nikita