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

[6.x, 8.x] Laravel 8 support PR broke auto method dependency resolving when string keys are used #34903

Closed
stancl opened this issue Oct 20, 2020 · 20 comments
Labels

Comments

@stancl
Copy link
Contributor

stancl commented Oct 20, 2020

  • Laravel Version: 8.11.0
  • PHP Version: 7.4

Description:

#34884 broke Livewire (and presumably other things too).

Steps To Reproduce:

@livewire('my-component', ['foo' => 'bar'])

causes the result of static::getMethodDependencies($container, $callback, $parameters) to be ['foo' => 'bar']

Which results in:

Cannot unpack array with string keys

@patrickbrouwers
Copy link
Contributor

Can confirm this issue with routes with string values (like a slug or uuid):

Route::get('articles/{article:slug}', [ArticlesController::class, 'show'])->name('articles.show');

@driesvints
Copy link
Member

@stancl why are you passing arrays with string keys there?

@driesvints
Copy link
Member

driesvints commented Oct 20, 2020

@stancl @patrickbrouwers do your both use cases work again if we revert this one single change?

#34884 (review)

@driesvints driesvints added the bug label Oct 20, 2020
@patrickbrouwers
Copy link
Contributor

@driesvints reverting that line indeed fixes the routing issue

@stancl
Copy link
Contributor Author

stancl commented Oct 20, 2020

@driesvints That way the values get matched with the parameters in mount() by their name.

Do you want me to try upgrading to the last version and reverting this one change in vendor?

@driesvints
Copy link
Member

@stancl yes

@stancl
Copy link
Contributor Author

stancl commented Oct 20, 2020

@driesvints Yeah, fixes it completely 👍🏻

@timrspratt
Copy link
Contributor

timrspratt commented Oct 20, 2020

Wrapping it in array_values() appears to solve it.

$callback(...array_values(static::getMethodDependencies($container, $callback, $parameters)))

From my quick test, the livewire component properties resolve, even if the order doesn't match that in the mount() method.

@stancl
Copy link
Contributor Author

stancl commented Oct 20, 2020

@timrspratt How can it work when the order doesn't match and there are no names? Are you using typehints maybe?

@timrspratt
Copy link
Contributor

timrspratt commented Oct 20, 2020

@stancl try it? I think the getMethodDependencies uses reflection and puts them in the correct order. I am using typehints, but it should go off the property names.

@stancl
Copy link
Contributor Author

stancl commented Oct 20, 2020

Yeah, that works.

@taylorotwell
Copy link
Member

Fixing and releasing a patch.

@driesvints
Copy link
Member

Thanks all

@GrahamCampbell
Copy link
Member

causes the result of static::getMethodDependencies($container, $callback, $parameters) to be ['foo' => 'bar']

I don't see how this can be the case. Where in the code does a string key get used?

@GrahamCampbell
Copy link
Member

I don't see how this can be the case. Where in the code does a string key get used?

That should be fixed, rather than hacking an array_values in.

@driesvints
Copy link
Member

@GrahamCampbell this was fixed. See taylor's comment above.

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Oct 26, 2020

Fixed with an array_values hack that shouldn't be needed. We need to work out what actually went wrong and fix the underlying issue.

@driesvints
Copy link
Member

The method was first using call_user_func_array where the second argument expects an indexed array. All examples in the PHP manual are with non-named array keys and they don't matter for that function: https://www.php.net/manual/en/function.call-user-func-array.php

Changing this to directly calling the callback made this incorrect usage more explicit: 82ffe3c#r43417881

Therefor this isn't a bug and calling array_values is a good workaround to the problem. Please don't re-open this.

@driesvints
Copy link
Member

driesvints commented Oct 26, 2020

I found what's causing the issue in Livewire. It's because Livewire is explicitly overriding the getMethodDependencies in its extended class and altering the behavior of this method:

https://github.com/livewire/livewire/blob/master/src/ImplicitlyBoundMethod.php#L11-L23

I think it's best that Livewire wraps the result of that method in an array_values call unless it explicitly needs the named parameters (which I don't see a reason for).

@driesvints
Copy link
Member

Ping @calebporzio ^

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

No branches or pull requests

6 participants