Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[8.x] Fix forwarded call with named arguments #40421

Merged
merged 1 commit into from
Jan 15, 2022
Merged

[8.x] Fix forwarded call with named arguments #40421

merged 1 commit into from
Jan 15, 2022

Conversation

bakerkretzmar
Copy link
Contributor

@bakerkretzmar bakerkretzmar commented Jan 14, 2022

This PR fixes one instance where passing named arguments is currently broken.

// Doesn't work before this PR, ends up calling the base `listContents` method
// with arguments `true, false` instead of `'', true`

app('filesystem')->listContents(recursive: true);

Laravel's support policy is very clear that named arguments are not guaranteed to be backwards-compatible:

PHP's named arguments functionality are not covered by Laravel's backwards compatibility guidelines [...] using named arguments when calling Laravel methods should be done cautiously.

However, the array_values call that this PR removes explicitly prevents named arguments from being used at all, even if the developer is using them intentionally and understands the implications of doing so.

I understand that there are a lot of strong opinions about whether named arguments are a good idea, and personally I agree with the decision to exclude them from any backwards-compatibility guarantees, but they are part of the language so it makes sense for it to at least be possible to use them.

See also #38066.

@taylorotwell
Copy link
Member

taylorotwell commented Jan 14, 2022

Is it possible this array_values call was present for some other reason? Is this going to introduce some bug that was intended to fix?

@bakerkretzmar
Copy link
Contributor Author

bakerkretzmar commented Jan 14, 2022

It was added as a replacement for call_user_func_array to make Laravel 6 compatible with PHP 8, here: 82ffe3c#diff-4f23ce415eb581db9639b4aba229a0c5df474b436a7dd46a075a06dcbb4532db

There are several other very similar places in the framework doing this kind of thing with __call() and it's inconsistent, some of them use ...array_values($parameters) and some just use ...$parameters. Even in that commit, it was done differently in different places.

Honestly my guess is that array_values is there just to discourage/prevent use of named arguments (and I totally understand discouraging using them I just don't want it to be prevented entirely). @GrahamCampbell would probably know for sure though.

@taylorotwell
Copy link
Member

I don't think you can even unpack an array with string keys until PHP 8.1?

@bakerkretzmar
Copy link
Contributor Author

I've tested this on PHP 8 and 8.1 and it works in function arguments (but not in something like ['one', ...['two' => 'three']]). I'll try on 7.4.

@bakerkretzmar
Copy link
Contributor Author

bakerkretzmar commented Jan 14, 2022

It's fine on PHP 7.4 because there aren't named arguments, so the $parameters array could never have string keys. Basically the change in this PR is safe to make inside magic methods like __call() because they get their arguments from PHP.

@bakerkretzmar
Copy link
Contributor Author

bakerkretzmar commented Jan 14, 2022

Okay cool, I just tried it with call_user_func_array too and named arguments actually work fine there, it can handle an associative array. So even though PHP 8 wasn't out/supported, technically this used to work before it was updated to use argument unpacking with ... and array_values.

@bakerkretzmar
Copy link
Contributor Author

bakerkretzmar commented Jan 14, 2022

Last comment I swear 😅 after looking through every array_values call in the framework, I think this is actually the one and only place this is broken, specifically because it's inside a __call(). Other magic methods in the framework don't call array_values on their parameters like this, and everywhere else array_values is used it's on user input, which makes sense because you don't know if it has string keys or not. Inside magic methods it's safe not to.

@taylorotwell taylorotwell merged commit 40fdde0 into laravel:8.x Jan 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants