[9.x] Fallback to resolving controller middleware like before

[lorisleiva]

Context

PR #44516 introduces a new way of resolving controller middleware statically so that they can be resolved before the controller is instantiated.

Problem

The PR itself is fine and fills a need for the community but there's just a small implementation detail that makes it no longer possible for Laravel Actions to use controller middleware in its ControllerDecorator.

Inside the updated Route@controllerMiddleware method, we now check that the controller class is a Controller instance and, if not, return an empty array.

// ...

if (is_a($controllerClass, Controller::class, true)) {
    return $this->controllerDispatcher()->getMiddleware(
        $this->getController(), $controllerMethod
    );
}

return [];

This is problematic for Laravel Actions since Actions can be used within any class architecture and will likely not want to be tight to a Controller abstract class.

Solution

The simple solution proposed in this PR is to fall back to resolving controller middleware as we were before.

  // ...
  
- if (is_a($controllerClass, Controller::class, true)) {
-    return $this->controllerDispatcher()->getMiddleware(
-        $this->getController(), $controllerMethod
-     );
- }
  
- return [];
+ return $this->controllerDispatcher()->getMiddleware(
+     $this->getController(), $controllerMethod
+ );

This change does not cause any side effect because a safety check already exists within the getMiddleware method of the ControllerDispatcher.

if (! method_exists($controller, 'getMiddleware')) {
    return [];
}

Note that this change does not affect the new feature brought by #44516. It simply relaxes the second if statement so that both solutions can live together.

[taylorotwell]

Hey @lorisleiva - is there anything we can check to know if a class is an action? Some other interface or trait your package requires on all actions?

The change you propose means that POPO controllers that don't extend anything will now be resolved like they were before, which we were trying to avoid.

[lorisleiva]

Hi @taylorotwell,

Thanks for coming back to me. That makes sense.

TL;DR; Yes, it uses the AsController trait to identify actions that can run as controllers.

Laravel Actions works by hooking some logic into the IoC container and wrapping any classes that use a certain trait into the appropriate Decorator based on where it was resolved (using debug_backtrace).

I used traits instead of interfaces to allow adding features to actions directly (like MyAction::dispatch() for jobs). Because of this decision, I sadly cannot use the HasMiddleware interface inside the AsController trait.

I tried to resolve and execute all middleware directly within the ControllerDecorator since with the current solution no controller middleware are resolved anyway. But sadly, I end up executing the route middleware twice.

Since the controller is resolved from the container after middleware are resolved, Laravel Actions has no control over this part of the logic.

Do you have any suggestion regarding how we could make that if statement works with Laravel Actions whilst keeping the strong typing?

[taylorotwell]

Can we do a class_uses_recursive check for your AsController trait?

[inxilpro]

Would it make sense to add a second HasInstanceMiddleware interface or something?

Something like:

interface HasInstanceMiddleware
{
    /**
     * Get the middleware that should be assigned to the controller instance.
     *
     * @return array
     */
    public function getMiddleware();
}

Anything that needs this behavior could then implement that interface…

[lorisleiva]

@inxilpro I could already use the HasMiddleware interface for that purpose but, unfortunately, the whole API of Laravel Actions relies on using traits instead of interfaces so it can provide helper methods. Unfortunately, it is not possible to implement an interface within a trait in PHP. Changing this would be a significant breaking change for Laravel Actions users.

@taylorotwell I'm happy to add a recursive trait check but I assumed this would be coupling the framework too much with an external library.

An other approach would be to resolve the controller from the container before that last if statement. Because once the controller is resolved, the real ControllerDecorator will be revealed and that one can extends the Controller abstract class without a problem.

Something like that:

// ...

$resolvedController = $this->getController();

if (is_a($resolvedController, Controller::class, true)) {
    return $this->controllerDispatcher()->getMiddleware(
        $resolvedController, $controllerMethod
    );
}

return [];

This wouldn’t affect PR #44516 because implementing the HasMiddleware interface makes you return early and therefore the controller wouldn’t be instantiated at this point.

What do you think?

[X-Coder264]

Something like that:

// ...

$resolvedController = $this->getController();

if (is_a($resolvedController, Controller::class, true)) {
    return $this->controllerDispatcher()->getMiddleware(
        $resolvedController, $controllerMethod
    );
}

return [];

This wouldn’t affect PR #44516 because implementing the HasMiddleware interface makes you return early and therefore the controller wouldn’t be instantiated at this point.

We don't want users to have to implement the HasMiddleware interface (which would always return an empty array) to all controllers just to avoid instantiating the controller at that point, especially not due to some very specific package that most projects probably don't even use.

Adding a way of being able to define base classes and traits in userland which would be treated in the same way as the Routing\Controller::class atm makes a lot more sense.

[lorisleiva]

Hi @X-Coder264! Oh I see, I assumed the HasMiddleware interface was always going to be used in the context of the other PR but I understand now that you want to make sure route middleware are resolved before controller instantiation by default, even when no controller middleware are used. Thanks for providing that context!

In this case, @taylorotwell would it be okay if I updated this PR with a recursive trait check on a new trait, similar to the solution offered by @inxilpro but with a trait instead of an interface.

Something like that:

if ($this->hasInstanceMiddleware($controllerClass)) {
    return $this->controllerDispatcher()->getMiddleware(
        $this->getController(), $controllerMethod
    );
}

return [];

Such that hasInstanceMiddleware is defined as follows.

public function hasInstanceMiddleware (string $controllerClass)
{
    return is_a($controllerClass, Controller::class, true)
        || in_array(HasInstanceMiddlewareTrait::class, class_uses_recursive($controllerClass));
}

And HasInstanceMiddlewareTrait contains a simple abstract function.

trait HasInstanceMiddlewareTrait {
    abstract public function getMiddleware();
}

I am conscious an interface would make more sense than a trait here but this would unfortunately not work for Laravel Actions. I am happy to add an additional check for a HasInstanceMiddleware interface so at least all mechanics are covered if that’s preferable.

[taylorotwell]

@lorisleiva can we not just check for your existing trait?

[lorisleiva]

@taylorotwell If you're okay with it, I'm okay with it. I've updated the PR. Let me know what you think.

[inxilpro]

The change you propose means that POPO controllers that don't extend anything will now be resolved like they were before, which we were trying to avoid.

@taylorotwell maybe I'm missing something… but isn't this something we want to preserve? Prior to #44516, any class that has a getMiddleware() method on it would get resolved out of the container and have that method called. Shouldn't that remain the default for 9.x?

It seems like this change would address @lorisleiva's issue and also just generally be more backwards-compatible:

- if (is_a($controllerClass, Controller::class, true)) {
+ if (method_exists($controllerClass, 'getMiddleware')) {

The problem with class_uses_recursive is that it only solves the issue for this single package. Any other code that was relying on the existing getMiddleware() behavior will still be broken…

[lorisleiva]

@inxilpro That's a great point. Initially, I removed that if statement altogether because the ControllerDispatcher already does that check but it was causing the controller to resolve too early. By using that method_exists check before the controller gets resolved, we fix it for Laravel Actions and any class that tries to provide these middleware inside the controller.

EDIT: I've updated the PR again.

[taylorotwell]

I'm so confused. All I'm asking is can we not just do:

if (is_a($controllerClass, Controller::class, true) ||
    CLASS USES SOME TRAIT FROM LARAVEL ACTIONS PACKAGE) {
    return $this->controllerDispatcher()->getMiddleware(
        $this->getController(), $controllerMethod
    );
}

[taylorotwell]

Will just roll with this PR as is I guess 👍

[lorisleiva]

@taylorotwell That's what I did before my last commit (2135794) but I think the merged solution offered by @inxilpro is much better as it avoid coupling the framework with a third party package.