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

[9.x] Fix bound method contextual binding #45500

Merged

Conversation

imdhemy
Copy link
Contributor

@imdhemy imdhemy commented Jan 3, 2023

Issue description

Using contextual bindings with a method parameter will result in an error. This is because when the container tries to resolve the method dependencies, It encounters a buildStack that does not contain the class that contains the method to be called, as it was already popped from the build stack in a previous step.

Recreation example

The following commit reproduces the issue on a fresh laravel project.
imdhemy/laravel-reproduce@93a9f0d

Error message

You can find the error message here:
Target [App\Console\Commands\MyInterface] is not instantiable.
https://github.com/imdhemy/laravel-reproduce/actions/runs/3823527853/jobs/6504792663#step:5:23

PR description

This PR fixes this by temporarily pushing that class to the build stack so that the container can resolve the method dependencies correctly.

@taylorotwell
Copy link
Member

I don't think contextual binding has ever been designed to actually support specifying contextual bindings at the method level?

I think it would require actual first-party support of that feature, like this: https://github.com/laravel/framework/pull/30542/files

@taylorotwell
Copy link
Member

For example, I would expect it to support syntax like:

$app->when('Foo@handle')->needs(...)->give(...)

@taylorotwell
Copy link
Member

Linking this here for my own reference:

https://github.com/laravel/framework/pull/30542/files

@taylorotwell taylorotwell reopened this Jan 4, 2023
@taylorotwell
Copy link
Member

Would actually like to try to solve this.

@taylorotwell
Copy link
Member

Pushed a fix for static methods, which would be strings in call and not objects in the callback array.

@taylorotwell
Copy link
Member

I think this PR is likely better than the current situation. Ideally, we would be able to specify different implementations for different methods on the class but I think that is likely an edge case.

@imdhemy
Copy link
Contributor Author

imdhemy commented Jan 4, 2023

I'd be happy to work on this. I will check it in the morning.

@taylorotwell
Copy link
Member

No need to do anymore work I don't think.

@imdhemy
Copy link
Contributor Author

imdhemy commented Jan 5, 2023

I spent some time working on this today. As you already mentioned, we are already in a better situation with the current implementation.


Regarding the edge case, I started with a controller, and managed to make it work with the following syntax:

$this->app->when('MyController@<methodName>`)->needs('<abstract>')->give('<concrete>');

By pushing the MyController@<methodName> to the container's build stack on the Controller dispatcher.


Regarding the same edge case, I also managed to make it work with the following syntax:

$this->app->when(`<concrete>@<method>`)->needs('<abstract>')->give('<concrete>');

// should be called as follows:
$app->call('Foo@method');

by pushing the <concrete>@<method> to the container's build stack on the call method of the container.

[1]

// targeting something like `Foo@method`
if (is_string($callback) && ! in_array($callback, $this->buildStack, true)) {
    $this->buildStack[] = $callback;

    $pushedToBuildStack = true;
}

IMO, For the context of this PR, after pushing the code related to targeting $app->call('Foo@method') [1] to the container, we can merge this PR. And then I can prepare another PR for the controller dispatcher. I think the same technique can be used for all dispatchers if there are other dispatchers that need to be targeted.

What do you think?

@taylorotwell
Copy link
Member

I honestly think we should just merge this PR as is and not make any further changes for the time being because this PR solves most every use case people have wanted. I haven't seen many requests for method level contextual injection where multiple methods on the same class receive different types.

If you really need that behavior you can just make a separate class.

@taylorotwell taylorotwell merged commit 440361e into laravel:9.x Jan 5, 2023
michael-rubel added a commit to michael-rubel/laravel-enhanced-container that referenced this pull request Jan 13, 2023
@michael-rubel
Copy link
Contributor

michael-rubel commented Jan 13, 2023

@taylorotwell this breaks the package I've made to let people specify the context in any place of the app.

Looks like global binding is no longer available for the same class if you use contextual one.
Fixed this by relying on BoundMethod::call() directly instead of app()->call().

michael-rubel added a commit to michael-rubel/laravel-enhanced-container that referenced this pull request Jan 13, 2023
michael-rubel added a commit to michael-rubel/laravel-enhanced-container that referenced this pull request Jan 18, 2023
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.

3 participants