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

Laravel contextual binding does not work with form request #41195

Closed
Temepest74 opened this issue Feb 23, 2022 · 14 comments
Closed

Laravel contextual binding does not work with form request #41195

Temepest74 opened this issue Feb 23, 2022 · 14 comments

Comments

@Temepest74
Copy link

  • Laravel Version: 8.83.2
  • PHP Version: 8.1.1

Laravel contextual binding does not work with requests, my example will demonstrate what do I mean.

class ControllerA extends Controller {
    public function index(RequestA $request) {
    }
}  

class ControllerB extends ControllerA {
    public function index(RequestA $request) {
        dump(get_class($request)); //will print RequestA instead of RequestB, here is the problem.
    }
}  

class RequestA extends FormRequest
{
}

class RequestB extends RequestA
{
}

Now, If I use the below code inside RouteServiceProvider, the ControllerB won't have the RequestA bound to it.

$this->app->when(ControllerB::class)->need(RequestA)->give(RequestB);
@rodrigopedra
Copy link
Contributor

Contextual binding works for classes' constructors or resolved from the container using app()->make(...) for example.

Controller actions are not resolved from the container.

One option is to resolve it on the controller itself:

use App\Http\Requests\BarRequest;
use Illuminate\Http\Request;

public function index(Request $request)
{
    $request = tap(BarRequest::createFrom($request), fn($request) => $request->validateResolved());

    dd($request::class);
}

Another is to seek help in the support channels:

https://laravel.com/docs/9.x/contributions#support-questions

@X-Coder264
Copy link
Contributor

X-Coder264 commented Feb 23, 2022

Controller actions are not resolved from the container.

@rodrigopedra The request and other services that are typehinted on a controller action method ARE resolved from the container using $container->make();

https://github.com/laravel/framework/blob/v9.2.0/src/Illuminate/Routing/ControllerDispatcher.php#L40
https://github.com/laravel/framework/blob/v9.2.0/src/Illuminate/Routing/RouteDependencyResolverTrait.php#L80

I don't think contextual binding works for non-constructor dependencies.

In order to accomplish what the issue author wants, method binding should be used. Unfortunately it's not really documented on the container documentation page. The only mention of it is on the queue page (but at least it shows how it's done):

https://laravel.com/docs/9.x/queues#handle-method-dependency-injection

@rodrigopedra
Copy link
Contributor

Thanks for the heads up @X-Coder264 , I know that as you can read on this phrase:

Contextual binding works for classes' constructors or resolved from the container using app()->make(...) for example.

Maybe what is confusing on my text is the usage of "or" instead of "and". It was late down here and I had phrased it first in a different way and I might have overseen how the phrase ended.

I meant that it wouldn't work for non-constructors methods. But your comment makes it way clearer, thanks :)

@rodrigopedra
Copy link
Contributor

One more thing the code snippet I was referring to when I wrote

Controller actions are not resolved from the container.

public function dispatch(Route $route, $controller, $method)
{
$parameters = $this->resolveClassMethodDependencies(
$route->parametersWithoutNulls(), $controller, $method
);
if (method_exists($controller, 'callAction')) {
return $controller->callAction($method, $parameters);
}
return $controller->{$method}(...array_values($parameters));
}

Which as you can see shows that controller actions are not resolved from the container.

Maybe I could have phrased it better.

@X-Coder264
Copy link
Contributor

X-Coder264 commented Feb 24, 2022

Yeah, the entire sentence controller actions are not resolved from the container was confusing, as of course a method cannot be resolved from the container as the container can only resolve objects. Upon further reflection I guess what you wanted to say was that the container does not call the controller action method via $container->call().

@rodrigopedra
Copy link
Contributor

@X-Coder264 this is what I wanted to mean. Thanks!

Sorry English is not my native language. Have a nice day =)

@driesvints
Copy link
Member

We'd welcome a PR to make that more clearer in the docs. Thanks for helping out @X-Coder264 @rodrigopedra 👍

@Temepest74
Copy link
Author

@X-Coder264 could you help me with an example based on my code? I have some trouble implementing it

@Temepest74
Copy link
Author

@driesvints can you reopen this? I tried this code and didn't work.

        $this->app->bindMethod([ControllerA::class, 'index'], function (...$params) {
            Log::debug("1");
        });

This function will never be called. Please provide me with a working example or at least look into what I am doing wrong.

@rodrigopedra
Copy link
Contributor

@Temepest74 you can read the motivation to add this feature on its original PR:

#16800

The idea behind bindMethods is to provide all of a method dependencies for a method that will be resolved from the container. All parameters should be provided, as no reflection is done to other parameters.

And also it only works for methods that are called from the container, using app()->call(...) for example.

Controller actions are NOT called from the container. So you won't be able to decorate them using this approach.

Maybe you could extend your controller, and tell the container to resolve your extended class that overrides that method/action based on condition.

Please, further questions should be directed to the support channels, as according to the docs:

Laravel's GitHub issue trackers are not intended to provide Laravel help or support.

reference: https://laravel.com/docs/9.x/contributions#support-questions

See the link above for the support channels links.

@Temepest74
Copy link
Author

@Temepest74 you can read the motivation to add this feature on its original PR:

#16800

The idea behind bindMethods is to provide all of a method dependencies for a method that will be resolved from the container. All parameters should be provided, as no reflection is done to other parameters.

And also it only works for methods that are called from the container, using app()->call(...) for example.

Controller actions are NOT called from the container. So you won't be able to decorate them using this approach.

Maybe you could extend your controller, and tell the container to resolve your extended class that overrides that method/action based on condition.

Please, further questions should be directed to the support channels, as according to the docs:

Laravel's GitHub issue trackers are not intended to provide Laravel help or support.

reference: https://laravel.com/docs/9.x/contributions#support-questions

See the link above for the support channels links.

So, you just said this is impossible to do? After your colleague approved what the other people said (they said it is possible)

@rodrigopedra
Copy link
Contributor

@Temepest74

First I wrote:

And also it only works for methods that are called from the container, using app()->call(...) for example.

Here it refers to contextual bindings for methods.

Then I wrote:

Controller actions are NOT called from the container. So you won't be able to decorate them using this approach.

Which is true, and no other answer says the opposite.

What @X-Coder264 wrote is:

The request and other services that are typehinted on a controller action method ARE resolved from the container using $container->make();

Which means the dependencies are resolved from the container, not the method calling. If you look at the code linked in that answer you will see the ControllerDispatcher uses reflection to resolve an action's dependencies, it doesn't defer to $app->call(...)

And in the PR I linked above it says contextual bindings for methods are only applied to methods called from the container. Which a controller's method is not.

Resolving dependencies from the container is different than calling a method from the container.

Please send further questions to the support channels I linked above per Laravel's policy.

@Temepest74
Copy link
Author

Oh, ok my bad. I was unable to understand these terms.

@rodrigopedra
Copy link
Contributor

No worries, English is not my native language, I might have worded it badly :)

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

No branches or pull requests

4 participants