deceze / Kunststube-Router

A flexible router/reverse router for PHP.
53 stars 8 forks source link

More complete reverse routing #4

Open deceze opened 11 years ago

deceze commented 11 years ago

The router should have more complete reverse routing functionality, being able to assemble full URLs including path prefix, query parameters, host etc.

K-Phoen commented 11 years ago

I implemented named routes reverse routing, check out this commit if you are interested by this feature.

The following is now possible:

<?php
$url = $router->reverseRoute('homepage');
$url = $router->reverseRoute('news', array('id' => 42));
deceze commented 11 years ago

That functionality is already available through the regular dispatcher array:

$r->add('/', ['homepage']);
$r->add('/news/\d+:id', ['news']);

$r->reverseRoute(['homepage']);
$r->reverseRoute(['news', 'id' => 42]);

Not sure it's worth complicating the API just to have an explicitly named $name. It arguably lets you separate the dispatcher information from the name though. Let me think about it more. :)

K-Phoen commented 11 years ago

The problem with the current implementation is that you have to give the full dispatcher array. In my case, all the data in the dispatcher array is not useful in order to build the url.

deceze commented 11 years ago

I have tentatively pulled your changes into a separate branch to test them in some real world scenario. I have scaled the changes back a bit, I don't think the implementation of $ignored_dispatch_keys is quite as necessary or elegant as it could be.

Overall though I came around to seeing named routes are a useful addition.

K-Phoen commented 11 years ago

Thanks for reviewing this feature :)

This is why I used $ignored_dispatch_keys:

survey_delete:
  pattern: /surveys/:survey_id/delete
  controller: \PollMe\Controller\SurveysController::deleteAction

Considering this route, the only useful parameters when reversing it is the :survey_id one. The controller one is almost an implementation detail. But in order to reverse the route, I still need to give both of them. To bypass this behavior, I added a way to ignore a set of dispatch parameters when reversing the route.

deceze commented 11 years ago

Yeah, I understand why you did it, but that's a pretty inelegant solution. With this you need to repeat irrelevant parameters when calling reverse route methods, which kind of makes reverse routing moot. It's supposed to decouple URLs/routing configuration from code that needs URLs. When just using a dispatcher array, that is your canonical identifier for a URL/route; when you use it like that you decouple URLs from controllers and use the dispatcher array as the unique identifier. For named routes, the name becomes the unique identifier, and you'd expect that to decouple the use of the name from the dispatcher array. But with your solution, you have to use the name plus the dispatcher array (in negated form) as the unique identifier for a route. That kind of seems to defeat the purpose.

When using named routes, the dispatcher array needs to be ignored when checking for matches. Only mandatory placeholders in the URL need to be supplied. It needs to work something like this:

$r->addNamed('fooBar', '/\d+:id', ['controller' => 'foo', 'action' => 'bar']);
$r->reverseRouteNamed('fooBar', ['id' => 42]);

If not, we may as well not have named routes.

K-Phoen commented 11 years ago

I totally agree with this. My code was more a quick-fix than a real implementation.