Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Request] Object routing #10

Closed
franzliedke opened this issue Jan 11, 2013 · 29 comments
Closed

[Request] Object routing #10

franzliedke opened this issue Jan 11, 2013 · 29 comments

Comments

@franzliedke
Copy link
Contributor

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).

@taylorotwell
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

JoostK commented Jan 11, 2013

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 😃

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
Copy link
Contributor Author

@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
Copy link
Contributor

JoostK commented Jan 11, 2013

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
Copy link
Contributor Author

Can you expand on the contracts approach?

@JoostK
Copy link
Contributor

JoostK commented Jan 12, 2013

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

@JoostK
Copy link
Contributor

JoostK commented Jan 12, 2013

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
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Member

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

@JoostK
Copy link
Contributor

JoostK commented Jan 12, 2013

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

@stephenfrank
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

JoostK commented Jan 13, 2013

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

@bencorlett
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

dhrrgn commented Jan 14, 2013

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
Copy link
Contributor Author

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
Copy link
Contributor

dhrrgn commented Jan 14, 2013

@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
Copy link
Contributor

dhrrgn commented Jan 14, 2013

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
Copy link
Contributor Author

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

@dhrrgn
Copy link
Contributor

dhrrgn commented Jan 14, 2013

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
Copy link
Contributor Author

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

@JoostK
Copy link
Contributor

JoostK commented Jan 19, 2013

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

dhrrgn commented Jan 27, 2013

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
Copy link
Member

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

taylorotwell pushed a commit that referenced this issue Jan 15, 2019
joelharkes pushed a commit to joelharkes/framework_old that referenced this issue Mar 7, 2019
…figuration-file

Add eurides configuration file
dbpolito pushed a commit to dbpolito/framework that referenced this issue Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants