Skip to content

Conversation

carusogabriel
Copy link
Contributor

@carusogabriel carusogabriel commented Apr 26, 2020

…t typed as array

Currently, the error message shown to the user is:

Argument 1 passed to A::__set_state() must be of the type string, array given

Which isn't necessary that. The problem is before that: the type that is actually wrong. With this patch,
the error message now is:

Type of first argument of Foo::__set_state() must be array

This is proposed separately from #4177 as I believe is an improvement, not a BC.

…t typed as array

Currently, the error message shown to the user is:

```
Argument 1 passed to A::__set_state() must be of the type string, array given
```

Which isn't necessary that. The problem is before that: the type that is actually wrong. With this patch,
the error message now is:

```
Type of first argument of Foo::__set_state() must be array
```
&& ZEND_TYPE_IS_SET(fptr->common.arg_info[0].type)
&& ZEND_TYPE_FULL_MASK(fptr->common.arg_info[1].type) != MAY_BE_ARRAY
) {
zend_error(error_type, "Type of first argument of %s::__set_state() must be array", ZSTR_VAL(ce->name));
Copy link
Member

@kocsismate kocsismate Apr 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please use the %s::__set_state(): Argument #1 ($%s) must be of type array, %s given form?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll adopt it in #4177, thanks!

@nikic
Copy link
Member

nikic commented Apr 26, 2020

I don't understand why this is separate from #4177.

@carusogabriel carusogabriel deleted the php8/improve-error-message branch April 26, 2020 12:50
@carusogabriel
Copy link
Contributor Author

@nikic Yeah, looks like this will need to go there. I'll keep #5462 separate as I believe it has the same intentions as #5441.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants