Slim: Simplifying queuing internal middleware

[l0gicgate]

I think that we should add helpers on the App component to instantiate and add middleware to the queue.

Example to add the ErrorMiddleware:

$callableResolver = $app->getCallableResolver();
$responseFactory = $app->getResponseFactory();
$errorMiddleware = new ErrorMiddleware($callableResolver, $responseFactory, true, true, true);
$app->add($errorMiddleware);

The only real input we need from the user is for the last 3 parameters:

/*
 * @param bool $displayErrorDetails - Should be set to false in production
 * @param bool $logErrors - Parameter is passed to the default ErrorHandler
 * @param bool $logErrorDetails - Display error details in error log
 */

Should we do something like:

$app->addErrorMiddleware(true, true, true);

Same goes for the RoutingMiddleware:

// Currently
$routingMiddleware = new RoutingMiddleware($routeResolver);
$app->add($routingMiddleware);

// Could be
$app->addRoutingMiddleware();

It would remove a lot of the cluttering statements

[shadowhand]

Instead of adding more things to App, what about making a factory?

$app->add(SlimMiddleware::createRouting($app));
$app->add(SlimMiddleware::createErrorHandler($app));

[adriansuter]

Good idea.

Probably we should also make sure, that the error middleware and the routing middleware would not be added more than once.

[adriansuter]

Or some sort of a setup (helper) class that helps setting up the slim framework with the default core built-in middlewares?

SlimSetup::addDefaultErrorMiddleware($app, true, true, true) : MiddlewareInterface;

[shadowhand]

@adriansuter I actually like that more, duplicating $app twice in a line is awkward. With defaults, it gets real clean:

SlimMiddleware::addRouting($app);
SlimMiddleware::addErrorHandling($app);

[adriansuter]

@shadowhand Beware! A class name ending by Middleware which is not really a middleware might be confusing for users.

[shadowhand]

@adriansuter true, SlimSetup is probably reasonable. I feel like AppFactory could also be a reasonable place for these helpers:

$app = AppFactory::create();
AppFactory::addRoutingMiddleware($app);
AppFactory::addErrorHandlingMiddleware($app);

[adriansuter]

We have to return the middleware instances respectively, so that for example

AppFactory::addErrorHandlingMiddleware($app)
    ->setErrorHandler(MyNamedException::class, $handler);

is possible.

[l0gicgate]

I think throwing it in AppFactory is a good move

[adriansuter]

Are we sure that AppFactory is the right place to put these helpers in?

I mean, up to now AppFactory does not have this kind of helper methods. All static methods (setters basically) are there to configure the factory itself, such that the factory knows how to create the App instance upon a call to \Slim\Factory\AppFactory::create().

Wouldn't we mix functionalities by adding these static methods to the AppFactory? In the real world, if you have to fix (or upgrade) a product, usually you won't go to the producer neither, do you?

I feel like we should create a helper class only for this kind of setting up the instance $app.

[shadowhand]

Creating a separate MiddlewareSetup class would also be fine. Separating concerns is always 👍

[l0gicgate]

We could also add the helpers on the App. The reason being is I don't want people to have to import extra stuff. It defeats the purpose. If you create MiddlewareSetup then it needs to be imported. With AppFactory it's implied that's it's being used so that would be fine as well.. But if you put it on App then it works for all cases without having to re-import stuff, not to mention all dependencies are available internally to do it for App.

Just my two cents.

[adriansuter]

@l0gicgate I do undestand. So in order to avoid another import, we should probably proceed as you wrote in your first comment. So simply

$errorMiddleware = $app->addErrorMiddleware(true, true, true);

No need for any additional imports nor for any new factories nor for any new static helper classes.

The downside is that we are filling our App class, as @shadowhand already mentioned.

Another idea

Another idea would be to extend the middleware classes (at least the slim core ones) by a static method that can be used to self-create and self-add to a given $app. So

$errorMiddleware = ErrorMiddleware::create($app, true, true, true);

Of course that way the middleware is a self-factory - not really convincing, I know. And the user would need another import statement.

[shadowhand]

@adriansuter I mentioned something like that before: https://github.com/slimphp/Slim/issues/2762#issuecomment-513332562

The problem with that is then you're passing $app to the factory and the result to $app->add(...), which looks awkward.

[adriansuter]

The idea was, that $app->add(..) would be called directly in the factory. So the factory creates the middleware, adds it to the app and returns it.

[shadowhand]

Ah, but what if someone wants to add a middleware to a route group?

[adriansuter]

The helpers are for the app middleware layer only. Do we need helpers for the routing group middleware layer as well?

If yes, then we could add a second factory into the middleware that, additionally to the params above, receives the route group as well. This factory would perform the same operations, but adds the middleware to the route group instead of the app.

Hmm, but maybe something like that is confusing for users. Too much things under the hood. And a user learns to use the ->add() method to add a middleware.

So I guess we cannot prevent "awkward" code, can we?

[l0gicgate]

The helpers would be only for ErrorMiddleware and RoutingMiddleware. It's really just a UX thing.

[adriansuter]

The RoutingMiddleware would only be added to the application layer, right?

So I suggest to add two methods to the App class that can be used to automatically create and add those middleware instances. If someone really wants to add the ErrorMiddleware to the route group layer or to the route layer, then there is no helper to do that.

[l0gicgate]

@adriansuter the RoutingMiddleware can be added in a specific position by the user. At the moment it is automatically added at the end of the queue (executed last) in case the user decided not to add it manually. The reason for that is so you can control when routing gets performed. In Slim 3 that was controlled with the settings property determineRouteBeforeAppMiddleware

[adriansuter]

@l0gicgate Sure, but only on the application layer (middleware stack of $app). I mean, there is no need for a helper to create and add this middleware to a route group or to a route. This middleware would only make sense in the middleware stack of the application.

[adriansuter]

Proposition:

class App extends RouteCollectorProxy implements RequestHandlerInterface
{
    // ...
    public function addRoutingMiddleware(): RoutingMiddleware
    {
        $routingMiddleware = new RoutingMiddleware(
            $this->getRouteResolver(),
            $this->getRouteCollector()->getRouteParser()
        );
        $this->add($routingMiddleware);
        return $routingMiddleware;
    }

    public function addErrorMiddleware(
        bool $displayErrorDetails,
        bool $logErrors,
        bool $logErrorDetails
    ): ErrorMiddleware {
        $errorMiddleware = new ErrorMiddleware(
            $this->getCallableResolver(),
            $this->getResponseFactory(),
            $displayErrorDetails,
            $logErrors,
            $logErrorDetails
        );
        $this->add($errorMiddleware);
        return $errorMiddleware;
    }
    // ...
}

Maybe we should check somewhere that the routing middleware can't be added twice. Or maybe better that the routing middleware would not perform routing if the request has already the routingResults attribute.

[l0gicgate]

@adriansuter yes, only on the App layer.

Your example is pretty much exactly what I had in mind. I'm okay to proceed with this.

[adriansuter]

In https://github.com/slimphp/Twig-View/issues/118 the proposed solution is

$app->add(TwigMiddleware::create($app, $basePath, $twigContainerKey));

So basically, the middleware itself defines the static helper method.

To be consistent, do you think we should instead of https://github.com/slimphp/Slim/issues/2762#issuecomment-514383937 use something like https://github.com/slimphp/Slim/issues/2762#issuecomment-513332562?

$app->add(RoutingMiddleware::create($app));
$app->add(ErrorMiddleware::create($app, true, true, true));

Or do you think that these two middlewares should be handled differently?

[l0gicgate]

I'm not too worried about the twig side of things honestly. Let's proceed as planned with this https://github.com/slimphp/Slim/issues/2762#issuecomment-514383937.

I know this sounds silly, but the whole intent of this is less imports and less boilerplate code. Doing it the way you proposed adds two more imports:

use Slim\Middleware\RoutingMiddleware;
use Slim\Middleware\ErrorMiddleware;

...

$app->add(RoutingMiddleware::create($app));
$app->add(ErrorMiddleware::create($app, true, true, true));

I would like to avoid that.

[shadowhand]

@l0gicgate I don't know why avoiding imports is a helpful here. Most editors can automatically add use statements.

[l0gicgate]

@shadowhand so we're assuming everyone has a good editor? The entire purpose of this is to reduce boilerplate code, that includes imports.

[adriansuter]

Keep it slim, right? As slim as possible but not slimmer (quote by A. Einstein, slightly adapted by A. Suter 😉).

I can understand both of you. Here, I think, we need to make a compromise between being absolutely consistent and introducing special cases which might make someone's life easier. Let's implement those special cases (helpers).