Skip to content

Conversation

@jorgsowa
Copy link
Contributor

I was a bit confused when I encountered the error does not accept unknown named parameters.
The issue was that I passed an associative array to a variadic parameter, which made the message misleading.
Because of that, I felt a clearer error message would be more appropriate in this case.

I changed the error message from <function> does not accept unknown named parameters to <function> does not accept named arguments for variadic parameters.

The discussion about this problem was touched in #11026

@iluuu1994
Copy link
Member

/cc @Girgias @kocsismate, given you have both worked on error messages historically.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

I think the new wording makes more sense, as the current one is very confusing.

I was having a quick look to see why this error message appears, and it's due to the fact that the +/* ZPP specifiers (and the fast ZPP Z_PARAM_VARIADIC equivalent) don't support the named parameters. And it doesn't seem like it's very easy or even sensible to migrate to the Z_PARAM_VARIADIC_WITH_NAMED fast ZPP specifier that does support the named args.

--EXPECT--
array_merge() does not accept unknown named parameters
array_diff_key() does not accept unknown named parameters
array_merge() does not accept named arguments for variadic parameters
Copy link
Member

Choose a reason for hiding this comment

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

IMO this could still be slightly simplified. How about something like:

Suggested change
array_merge() does not accept named arguments for variadic parameters
Internal function array_merge() does not accept named variadic arguments

Meaning, any variadic argument must be positional, not named. Also hint at the fact that this is only the case for internal functions. @Girgias WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I like this wording :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, changed 👍🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants