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

[5.4] Allow routes to be registered fluently #14354

Closed
wants to merge 5 commits into from

Conversation

JosephSilber
Copy link
Member

@JosephSilber JosephSilber commented Jul 18, 2016

This enables stuff like:

Route::name('users.index')->middleware('auth')->get('users', function () {
    // some closure action...
});

...so that the calls to name and middleware are not hanging off the end of the closure.


Also, you can now use the middleware method with groups:

Route::middleware('auth')->prefix('api')->group(function () {
    // register some routes...
});

Finally, you can now register middleware for resources directly:

Route::middleware('auth')->resource('photo', 'PhotoController');

@JosephSilber
Copy link
Member Author

/cc @hipsterjazzbo

@hipsterjazzbo
Copy link

@JosephSilber Did you want any particular comments from me? Cause my first impressions are:

  1. ❤️ 👍
  2. That RouteRegistrar is sexy. I'm gonna use it for this neat new thing I'm working on 😉

return $this->macroCall($method, $parameters);
}

return (new RouteRegistrar($this))->attribute($method, $parameters[0]);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be done like here

Copy link
Member Author

@JosephSilber JosephSilber Jul 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rkgrep why???

The resource registrar has a lot of logic as to how it registers everything, so in some cases it makes sense to want to override it.

This route registrar serves the singular purpose of collecting route configuration before the route is actually created. Doesn't make much sense to override it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, perhaps I made a hasty decision on that )

@GrahamCampbell GrahamCampbell changed the title [5.3] Allow routes to be registered fluently [5.4] Allow routes to be registered fluently Aug 23, 2016
@GrahamCampbell GrahamCampbell changed the base branch from 5.3 to master August 23, 2016 14:13
@garygreen
Copy link
Contributor

@JosephSilber Looks very nice. Though just out of curiosity how come there should be another class? Would it be more simple to have this contained in the Router class? That way you wouldn't need:

  1. passthru
  2. magic getter for the native router methods
  3. The 'aliases' at the moment just has as why not add a new method to Route for name() which calls as.

@JosephSilber
Copy link
Member Author

@garygreen what would those methods return? Take this example:

Route::name('users.index')->middleware('auth')->get('users', function () {
    // some closure action...
});

What class is that middleware method on?

@garygreen
Copy link
Contributor

middleware would be a problem.. how would you call the existing middleware method on Router with this current PR? It makes it seem tricky to call it now. Can't the current middleware command become middlewareAlias(), then when you call middleware it will just keep track of those attributes to pass onto the Route...

@JosephSilber
Copy link
Member Author

JosephSilber commented Aug 24, 2016

@garygreen I'm not following what you're saying.

If we want to be able to chain those methods, then the calls to middleware/name/as/prefix etc. must return some sort of object. That's why there's this new RouteRegistrar class.

What would you have it return instead?

@garygreen
Copy link
Contributor

@JosephSilber

If we want to be able to chain those methods, then the calls to middleware/name/as/prefix etc. must return some sort of object. That's why there's this new RouteRegistrar class. What would you have it return instead?

All I'm curious about is why there is need for a class for RouteRegistrar as it doesn't seem to do much except build list of attributes to pass onto a route, so why not move the code onto the current Router class?

@JosephSilber
Copy link
Member Author

@garygreen I don't understand what you're saying, and we seem to be going in circles.

Would you mind whipping up an example of the implementation you have in mind, so that we can look at actual code? Pseudo code should be enough. I think that way I'd have an easier time understanding this from your point of view.

@garygreen
Copy link
Contributor

@JosephSilber super quick code just as an example of what I'm going on about. Sorry for not being clear 😄

<?php namespace Illuminate\Routing\Router;

class Router {

    /**
    * The attributes to pass on to the router.
    *
    * @var array
    */
    protected $attributes = [];

    protected $passthruAttributes = [
        'name', 'as', 'middleware', ...
    ];

    /**
     * Create a new route instance.
     *
     * @param  array|string  $methods
     * @param  string  $uri
     * @param  mixed  $action
     * @return \Illuminate\Routing\Route
     */
    protected function createRoute($methods, $uri, $action)
    {
        // Existing code...

        // In here call the attributes on the route.

        return $route;
    }

    public function middlewareAlias($name, $middleware) // Renamed from middleware() so the magic method gets called and middleware passed onto route.
    {
        //...
    }

    /**
    * Dynamically handle calls into the route registrar.
    *
    * @param  string  $method
    * @param  array  $parameters
    * @return $this|void
    */
    public function __call($method, $parameters)
    {
        if (in_array($this->passthruAttributes, $method))
        {
            $this->attribute($method, $parameters[0]);
            return $this;
        }
    }

}

@JosephSilber
Copy link
Member Author

What is that $attributes property? The attributes have to be unique for each route.

You could reset it after every call to createRoute (similar to how we handle groups), but that's just so messy. Why would you ever want that over a dedicated class?

@garygreen
Copy link
Contributor

You could reset it after every call to createRoute (similar to how we handle groups), but that's just so messy.

That's kind of how I thought this was working, I didn't realise it was creating a new RouteRegistrar for each route, not sure why I thought it was a singleton, my bad.

@taylorotwell
Copy link
Member

Holding off on this for now.

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

Successfully merging this pull request may close these issues.

5 participants