add-route-arguments-as-attributes-to-the-request-handler-strategy

[dominikzogg]

No description provided.

[coveralls]

Coverage remained the same at 100.0% when pulling 1e5d970 on dominikzogg:add-route-arguments-as-attributes-to-the-request-handler-strategy into 2b0ed80 on slimphp:4.x.

[l0gicgate]

I’m not sure I’m in favour of doing this. Why can’t we add this to RouteContext Instead. @adriansuter thoughts?

[dominikzogg]

@l0gicgate please don't, cause it make the code of the request handler depending on the framework. Expressive and my own work with attribute for the arguments, it seems to be common practice.

[l0gicgate]

How does that make it dependant though? You can already retrieve the route arguments via the route attribute from the RouteContext object.

To me filling up the request’s attributes with a for loop seems like object pollution. I don’t like the “shove it all in there” approach without structure.

With the default strategy it’s contained in $args at least. Also, for the record, you can create your own invocation strategy to achieve this, I don’t agree that this should be default behavior.

[dominikzogg]

RouteContext is Slim only, using PSR15 and still depend on the framework is useless to me. Strategy is not changeable, see

Slim/Slim/Routing/Route.php

Line 366 in 2b0ed80

$strategy = new RequestHandler();

[adriansuter]

I think by default we should leave the route arguments in the $args array.

But feel free to create your own route strategy (https://www.slimframework.com/docs/v4/objects/routing.html#route-strategies) or build a middleware that does exactly what you need.

// Middleware (needs to be called after the routing middleware and therefore has to be added before)
$app->add(function (Request $request, \Psr\Http\Server\RequestHandlerInterface $requestHandler) {
    /** @var Slim\Routing\Route\Route $route */
    $route = $request->getAttribute('route');
    foreach ($route->getArguments() as $key => $val) {
        $request = $request->withAttribute($key, $val);
    }

    return $requestHandler->handle($request);
});

Just make sure, that you would not overwrite existing attributes!

[dominikzogg]

@adriansuter as mentioned in my previous post gets the strategy overridden if the controller implements the RequestHandlerInterface. Middleware would work.

But I don't see the point why you're not interested in being compatible with other microframeworks work with PSR15.

[adriansuter]

I couldn't find any information in the specs of PSR-15 about storing the route placeholders (arguments) in the server request. Could you give me a link?

Or is this some kind of common silent agreement between many microframeworks? If yes, do you have a list of microframeworks that support this kind of behaviour?

[l0gicgate]

@adriansuter it’s not common practice. And as I said, this can be achieved with creating a different invocation strategy which we could include for users to use if they wanted too.

Then you can set the default invocation strategy with $app->getRouteCollector()->setDefaultInvocationStrategy($requestHandlerWithRouteArgumentsAsAttributesStrategy)

Every route you map thereafter will have that strategy and do what you want.

[dominikzogg]

@l0gicgate @adriansuter

Please read the mentioned if statement to see, that changing the strategy is not possible without a code change.

About common: path values as attribute was default in Slim 3 and is in Expressive.

https://github.com/slimphp/Slim/blob/3.x/Slim/Handlers/Strategies/RequestResponse.php#L36
https://github.com/zendframework/zend-expressive-router/blob/master/src/Middleware/RouteMiddleware.php#L49

About middleware: Adding a middleware would mean a general behavior change cause I would need to route first.

About Route Context: this context exists only in slim, so I loose the benefits of PSR15.

[l0gicgate]

@dominikzogg we need to change the logic in the Route component to accept custom strategies for RequestHandler. I can PR this later today. Then we can add a new invocation strategy for that.

[dominikzogg]

@l0gicgate thank you

[l0gicgate]

Follow discussion in #2774