Add default attributes to the route

[AlexStansfield]

Added setDefaultAttributes method to the route class. Allows you to set default attributes when defining a route.

For example:

$app->get('/hello/world', function ($request, $response, $args) {
    $response->write("Hello " . $args['extra']);
    return $response;
})->setDefaultAttributes(['extra' => 'world']);

[geggleto]

it is currently unclear in your PR what would happen if...

//.... /hello/Alex
$app->get('/hello/{name}', function ($request, $response, $args) {
    $response->write("Hello " . $args['name']);
    return $response;
})->setDefaultAttributes(['name' => 'Bob']);

I think it should overwrite Bob...but I would like a test case for this exact situation please :)

Other than that it looks useful 👍

[AlexStansfield]

Yeah it would overwrite Bob. Basically it uses the default if nothing else over rules it. This thread gives a more info about my reasons behind it - http://help.slimframework.com/discussions/questions/7376-setting-attributes-in-a-route

It's just gone midnight here so will add a test to show default is overwritten by a parameter matched in the url tomorrow.

[kminek]

this is nice

exactly the kind of feature i was asking here: #1339 (comment)

[AlexStansfield]

Who needs sleep. Added test case to show default attribute being used and also overridden by named parameter.

[geggleto]

👍

[silentworks]

Ok I think a solution might actually be:

$app->get('/hello/{name}', function ($request, $response, $args) {
    $response->write("Hello " . $request->getAttribute('name', 'Bob'));
    return $response;
});

Yes I know this is not happening from the outside in, but it is a solution I think.

[AlexStansfield]

My use case sees me needing to access the attribute in Middleware. This middleware will be used across many routes, each route will be have different defaults. So it really needs to be set on the route.

Example: http://help.slimframework.com/discussions/questions/7376-setting-attributes-in-a-route

Also if you have a reusable class method for the route (rather than a closure) the same theory applies.

[silentworks]

We had some discussion on IRC yesterday about this and are still considering if this is the way to go, also could you make your implementation happen inside of the Route.php file rather than in App.php please.

[AlexStansfield]

Makes perfect sense, sorry I didn't do that first time. Only thing was really wasn't sure the best name for the method that's called in the run to add the attributes. Now thinking addDefaultAttributesToRequest might have been better.

Crap just saw the php 5.4 error. Will fix.

[akrabat]

Presumably, you mean that you want the default attribute available in the route middleware? I don't see how it's available in app-level middleware.

[akrabat]

I would prefer two methods: setAttributes(array $attributes) and setAttribute($name, $value) as these are then analogous to the the Request's withAttributes and withAttribute methods.

[AlexStansfield]

Hmm, that's a very good point. I'd only been looking at it from the point of a Route middleware. But should it also be accessible in App Middleware? I kinda feel it should even though my own use case doesn't need it. Would need to see if that's actually possible or not.

Method names make sense, will rename the original method and add the new one.

Thanks for being so quick to reply guys, I only picked up Slim 3 on Monday but really liking it a lot already.

[AlexStansfield]

Renamed methods and added setAttribute.

As the framework executes the App middleware before resolving the route it seems these attributes will definitely not be able to be accessed in App middleware. That also goes for any attributes determined by the url matching (e.g /hello/{name}). I'm not sure if this was by design or a side effect of the order in which things are done.

[JoeBengalen]

That is by design:
http://www.slimframework.com/docs/concepts/middleware.html

Routing happens in the app (most inner layer)

[AlexStansfield]

Yeah that's fair enough. I was aware that the route isn't dispatched until after the application middleware is executed, just wasn't aware that the resolving of the route wasn't done until I dived into the code itself.

It makes sense, but not sure if maybe the docs just need to make that a bit clearer so users know that any attributes determined by the route won't be available in the application middleware. But then maybe there are no real use cases for that anyway so it doesn't matter.

[AlexStansfield]

So from the merge last night that introduced a Route Arguments attribute I have rebased my branch and squashed down my previous commits then fixed the tests (attributes now got from $request->getAttribute('blah') rather than $args['blah']).

However, I wasn't sure if you'd prefer these to still be going into the request attributes or if they should be treated as route arguments as before.

Personally I prefer the latter as it means they will always be where I expect them. In my route middlewear I could check for a route argument 'name' whether it's value has been defined by /hello/{name} or explicitly on the route with setAttribute('name', 'blah').

The question then is, if we go with route arguments is Attribute really the right term for them now? should it be setRouteArgument (or setArgument) instead?

[AlexStansfield]

As there is a fair timezone difference (GMT+7 here) I went ahead and changed this to setArguments. I can rollback this last commit if need be.

[akrabat]

I agree with your thinking. setArgument/setArguments that store into the routeArguments sub-array is the right approach as it's the only way to ensure that $args has keys for the optional route parameters.

[silentworks]

In this case, should this be renamed to setParam/setParams as we had in v2?

[akrabat]

Fine with me - though I'd also call it routeParams rather than routeArguments in the rest of the code.

[akrabat]

@AlexStansfield, I'm sorry, but I think that my work in #1348 to move the route argument assignment from the App to the strategy has inadvertently re-implemented a good proportion of your work.

Can you check over and see if it does what you require and if not, update this PR to add what's missing?

[AlexStansfield]

Haha, you guys are really making me work for this feature 😛

Will take a look at you work and see what I need to do.

[AlexStansfield]

@akrabat, looks like you've pretty much done the work. I need to checkout your branch and test but it looks like you've already added the setArgument public method so in theory you should be able to just add arguments in when defining the route.

I'm going to add the tests I added for this use case. Not sure how best to do this as your code isn't merged yet. Should I do a pull request onto your own branch?

[silentworks]

@AlexStansfield it is now merged.

[AlexStansfield]

Ok, so not much left for me in the end. I added the setter and getters to RouteInterface. Added the tests to AppTest for the use case of calling setArgument/setArguments in the route definition and I also fixed some type hinting and docblocks (hopefully correctly)

[JoeBengalen]

Not sure if setters should be in the interface?

[AlexStansfield]

That's a good point and I guess it's a design question.

Should Slim framework support setting arguments on a route definition (therefore something all route implementations should implement) or should it be a specific feature of the default router only?

Will happily remove them if the latter is preferred.

[akrabat]

That's a decision for @codeguy or @silentworks.

[AlexStansfield]

Just spotted @silentworks comment about renaming Arguments to Params. Do you want that done in this Pull Request?

[silentworks]

@AlexStansfield no lets leave it as Arguments as it could get confused with the Request object getParam/getParams methods.

[AlexStansfield]

Cool, then other than the question raised by @JoeBengalen about the setters in the interface I think this is pretty much final version 😀

[silentworks]

Ok cool, will wait on @codeguy to discuss the setters in the interface, we already have setName in that particular interface so I don't really see it as a issue, but let me have a chat with @codeguy and hear what he says.

I am guessing you are getting ready for bed so, you might just see this all merged in when you wake up or further discussions on it.

Thanks again for all your help and your push in making this change possible.

[AlexStansfield]

Only just gone 5pm here (Bangkok) so will be around for a few hours yet.

Many thanks to you guys for being so helpful and quick to respond. Look forward to working more with Slim.

[silentworks]

Will discuss with @codeguy about setters in interfaces later, going to merge this in for now.

[AlexStansfield]

Many Thanks!!!

[coveralls]

Coverage increased (+0.2%) to 82.692% when pulling 0204d3e on AlexStansfield:routeAttributes into 50de533 on slimphp:3.x.