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

Added named routes (@routeName/my/route) and URL Generator #34

Closed
wants to merge 1 commit into from

Conversation

nebulousGirl
Copy link
Contributor

To help with routes management across an application, I have added support for named routes using the syntax @route-name/the-route.

The URL generator can then be used like this UrlGenerator->generate("route-name").

@nebulousGirl
Copy link
Contributor Author

I'm glad you like it.

Yeah, I'm thinking that the URL generator is not optimal right now. I wanted to keep it separated from the RouteCollection, but maybe a full integration would be better.

Also, the named routes feature was just the first step to attempt to make the routes multilingual. So, if you have an idea on how to implement multilingual routes, I am open to suggestions. It could be something like that:

    $routes->addRoute("GET", "@named-route/my-route");
    $routes->localize("named-route", "fr", "ma-route");

If you don't mind, I'd like to have feedback on my coding style, just so I don't make the same mistakes next time.

@nebulousGirl
Copy link
Contributor Author

If it's too much trouble or too complicated for the scope of the package. The localization part can also happen before dispatching the request to the router so no worries.

I would use a middleware to translate the request to the default language before sending it to the router.

@philipobenito
Copy link
Member

Localising routes sounds like a great idea, possibly as a sub package of this though that could be used as a middleware or an adapter, we can have a more in-depth discussion about that down the line.

As for coding standards I'll have a more in-depth look when I get a bit of spare time the next few days. My instant reaction though was clean and clear code. There are just some preferences of mine such as naming conventions around interfaces RouteParserInterface vs IRouteParser, again though, that is a preference thing only.

The only issue I see with the PR at first glance is that this iteration nebulousGirl@551dd57#diff-62198537a4f82be3c79d52fa00df4281R78 is a little too complex. I will provide a more constructive comment on this when I have a little more time. But the Scrutinizer score drops from what would probably be a grade A or B class to a grade F simply on how complex these few lines are, this is down to the amount of paths the conditionals create.

Again, overall this is the perfect solution to something I've been racking my brains about for some time now. There may just be a small amount of refactoring around naming and to build up on the URLGenerator to probably be an object describing the whole URL around a named or active route with a few conveniences like redirection etc.

Thanks again 👍

@philipobenito philipobenito added this to the v2.0.0 milestone Aug 24, 2015
@philipobenito
Copy link
Member

@nebulousGirl I'm using this as a base for the grouped and named route functionality in v2.0.0, unfortunately I think it's a little out of the scope for v1.x.

@philipobenito
Copy link
Member

@nebulousGirl ended up going a slightly different route with this but v2 has named routes via the fluent api and will be providing a service provider to add them via config.

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.

None yet

2 participants