laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.2k stars 10.89k forks source link

[Request] Object routing #10

Closed franzliedke closed 11 years ago

franzliedke commented 11 years ago

It would be awesome if you were able to just pass model objects instead of arrays when generating routes. That would allow for complete flexibility when generating routes, without having to worry about changing the parameter array that is passed everywhere where URLs are generated.

In relation to these issues: https://github.com/symfony/symfony/issues/5999 https://github.com/illuminate/database/pull/38

(Meaning I've raised this at the Symfony routing level and tried to implement ArrayAccess for models, which should simplify this).

taylorotwell commented 11 years ago

This sounds cool but I don't totally follow. Maybe post some code examples of what this could look like in Laravel?

On Jan 11, 2013, at 5:17 PM, Franz Liedke notifications@github.com wrote:

It would be awesome if you were able to just pass model objects instead of arrays when generating routes. That would allow for complete flexibility when generating routes, without having to worry about changing the parameter array that is passed everywhere where URLs are generated.

In relation to these issues: symfony/symfony#5999 illuminate/database#38

(Meaning I've raised this at the Symfony routing level and tried to implement ArrayAccess for models, which should simplify this).

— Reply to this email directly or view it on GitHub.

franzliedke commented 11 years ago

Imagine you generate profile URLs like this all over the place:

URL::route('profile', array('id' => $user->id));

Now you want to change the URLs to include the username. You change the route, now you have to change all the places where you generate the URL.

URL::route('profile', array('id' => $user->id, 'username' => $user->username));

And all that just for a few cosmetic changes (the username isn't needed to identify the user) - changes that should only belong to the routes. Just imagine if you could do this instead:

URL::route('profile', $user);

Happiness ensues.

JoostK commented 11 years ago

I've thought about this before as well. I think in your Eloquent you have to specify the parts that make up for the secondary key (the id being the primary) and that the values of those fields will be put into segments. Then in the router matching maybe also some magic to convert from these segments to the eloquent model, something along the following line:

Route::get('user/{@User}', function(User $user)
{
    // $user is now an instance of the User class. Here I'm using a type hint which would
    // prevent null values from being put in, but the router can inspect this and just throw
    // an HttpNotFoundException when the selected object is null. When a null value is
    // wanted the route closure must be defined as `User $user = null`, to allow for the
    // null, I think it is possible to read out default parameter values using Reflection.
});

A different syntax for the actual segment(s) can be used, because the Class information is also available in the type hint. Note that {@User} can actually be multiple segments, if e.g. your Eloquent model is defined as the following:

class User extends Eloquent
{
    protected $segments = ['firstname', 'lastname'];
}

This would be VERY cool as you don't even have to handle finding the object and bombing out when the record doesn't exist. There is one problem with it in that it doesn't allow you to set additional where conditions, possibly to do authorization etc.

Also, if no type hint is given you just get an array of all the segments instead of an actual model instance:

Route::get('user/{@User}', function($user)
{
    // $user = ['firstname' => 'Joost', 'lastname' => 'Koehoorn'];
});

You can do very nifty things with this kind of routing :smiley:

EDIT Additional where clauses can probably be set if you specify Query as the type hint:

Route::get('user/{@User}', function(Query $user)
{
    // Because of the fact the the type hint references a Query instance, we are
    // passed in a query which is setup with all where clauses already set.
    $user = $user->where('admin', '=', '1')->get();
});
franzliedke commented 11 years ago

@JoostK To be honest, I'm not totally sure this kind of code belongs into a controller. But I agree it would be pretty cool, though I'm afraid that it couldn't be as flexible as it would need to.

JoostK commented 11 years ago

If you don't do it like this you still need to setup all your routes to manually specify the model's secondary keys. If an extra field is necessary for the model, you only have to change the model definition and you would be done. This really extracts all boilerplate code of fetching a model.

There is a problem I just thought of, my approach doesn't cope with @taylorotwell's approach of using contracts to fetch a user, not messing with any ORM directly. I believe however that this can still be done when you don't supply a type hint, as you can then still use the passed associative array to pass to the contract implementation.

franzliedke commented 11 years ago

Can you expand on the contracts approach?

JoostK commented 11 years ago

You've probably already seen it but here you go: http://vimeo.com/53029232

JoostK commented 11 years ago

I now see that I'm taking this a lot further than you proposed. I didn't keep in mind that in L4 passing of arguments has changed from index-based to being associative.

taylorotwell commented 11 years ago

@franzliedke in your example of passing model to URL::route, how would it know what to extract from the object, does it extract the properties matching the wildcard segments?

franzliedke commented 11 years ago

Yes, that's the point and the thing that's currently not done (and why we need the models to implement ArrayAccess for an easy solution).

taylorotwell commented 11 years ago

Well, you wouldn't necessarily need ArrayAccess. I do think this is a cool idea though.

JoostK commented 11 years ago

Simply calling toArray will probably do the job? Extra keys will just be ignored by the router I think?

stephenfrank commented 11 years ago

I like the sound of this feature request but, as is stands, the additional key/value pairs in a URL::route() all get appended as a query string.

Route::get('/', array('as' => 'home', function() {}));

URL::route('home', array('this' => 'that'))); // http://example.dev/?this=that

Ignoring all other keys except the ones provided for as URI parameters would break this nifty feature (that I happen to find quite useful). I'd like to see forward/backward compatibility methods considered or maybe it's something that just happens when an instanceof Model is passed into the method.

franzliedke commented 11 years ago

Hmm, good point, @stephenfrank.

I think I mainly wanted to add ArrayAccess stuff to avoid the instanceof thing (and because it would be more flexible, plus convenient). Implementing Iterator might be another option, too.

JoostK commented 11 years ago

I wasn't even aware of this feature, @stephenfrank, good point.

bencorlett commented 11 years ago

I think this looks cool, but it is quite application specific and I'm not sure mixing database with routing is a good thing. Maybe @dandoescode could weigh in, he's pretty good with architecture stuff?

franzliedke commented 11 years ago

That's the point of implementing ArrayAccess - so that the routing code doesn't really have to know it's really dealing with a model object.

dhrrgn commented 11 years ago

Would be pretty simple, and would not require ArrayAccess. However, this is not where this request belongs. When calling Url::route() it simply passes the info along to the Symfony\Component\Routing\Generator\UrlGenerator, so your beef is with Symfony. As soon as their Routing component supports it, so will Laravel.

@taylorotwell you can probably close this.

franzliedke commented 11 years ago

That's why I mentioned this Symfony issue so that you guys can throw your weight in. :)

Dan, how else would you do it if not with ArrayAccess?

dhrrgn commented 11 years ago

@franzliedke The problem (as other's have stated) is that extra parameters are appended as a query string to the Url. Sure, you could specify in the model which ones to include, but that is outside of the Model's purview in my opinion.

dhrrgn commented 11 years ago

ArrayAccess doesn't really allow anything other than isset() and accessing like an array, to do anything useful with it, you would have to implement it an an Iterator as well (so you can loop over it). Why go through all that trouble when you can just call getAttributes() on it? You wouldn't want to use toArray() because that calls toArray() on all relations as well, and you certainly would not need those for creating a Url.

franzliedke commented 11 years ago

I am pretty sure the Symfony guys won't ever implement anything using functions from Laravel classes!?

dhrrgn commented 11 years ago

No, I know they won't. Was just using it as an example here. Even the guy on the Symfony request has said that ArrayAccess won't help much (because of the query params thing), so our conversation at this point, is well, pointless (that came out like me being a dick, but im not trying to be).

franzliedke commented 11 years ago

Gotcha, thanks for the responses. And don't worry, maybe I'm just a little slow tonight. :)

JoostK commented 11 years ago

I see now that @taylorotwell has partly implemented my example, nice. It's a little less fancy but also much more generic, so that's a good thing.

taylorotwell commented 11 years ago

So, it sounds like implementing this in any of the existing routing methods isn't feasible. That does leave open the option of a separate method? Are we still interested in pursuing this or want to let it rest?

stephenfrank commented 11 years ago

Here is another take on this (and I'm not suggesting that this is worth committing to core)... but I've loosely coded this in my base Model that extends Eloquent... might end up keeping it for myself.

class Product extends Eloquent
{
    public function route($name, $extra_parameters = array(), $absolute = true)
    {
        $route = \Route::getRoutes()->get($name);
        $parameter_keys = $route->getParameterKeys();

        $compiled = array();

        foreach ($parameter_keys as $key) {
            $compiled[$key] = $this->$key;
        }

        $compiled = array_merge($compiled, $extra_parameters);

        return \URL::route($name, $compiled, $absolute);
    }
}
Route::get('asdf/{id}', array('as' => 'viewProduct', function()
{
    $product = new Product;
    $product->id = 3;

    echo $product->route('viewProduct', array('this' => 'that'));
    // http://example.dev/asdf/3?this=that
}));
dhrrgn commented 11 years ago

That code is potentially insecure. Any implementation must respect the $hidden attributes. This way you can't accidentily return a URL with sensitive information.

taylorotwell commented 11 years ago

Don't think we'll pursue this right now.