lithium icon indicating copy to clipboard operation
lithium copied to clipboard

RFC: the Dispatcher rule system

Open jails opened this issue 13 years ago • 6 comments

Take, for example, the following code :

//Rule 1 : camelize action
MockDispatcher::config(array('rules' => array(
    'action' => array('action' => function($params) {
        return Inflector::camelize(strtolower($params['action']), false);
    })
)));
//Rule 2 : prefix action
MockDispatcher::config(array('rules' => array(
    'admin' => array('action' => 'admin_{:action}')
)));

Router::connect('/', array('controller' => 'test', 'action' => 'my_test', 'admin' => true));
MockDispatcher::run(new Request(array('url' => '/')));
$result = end(MockDispatcher::$dispatched);

echo $result->params['action'];

Since the default route is an 'admin' route the expected value could be : "admin_my_test", "admin_myTest" or "adminMyTest" since this route match the two rules ('action' & 'admin'). But the result is "myTest". However I think it's more than a kind of "bug".

Indeed, the whole Dispatcher::applyRules() need to be refactored for the following reasons imo:

  • The order of rules are important : with a single 'admin' rule conflicts are rare, but an 'action' rule will be applied almost all the time.
  • The first rule inserted is the last rule applied
  • The last rule applied imposes its transformation to all previous transformations : with the previous point, if 'action' is the first rule inserted using Dispatcher::config(), it will override tranformations of all other rules.
  • Once Dispatcher::$_rules is configured as a callback you can't add no more rules
  • The rules involved to the Dispatcher : this mean if you configure your own rule 'action' all plugins must now respect the convention you set.

With a Dispatcher::reset() and having all the previous points in mind workaround can be putted in place. But if Dispatcher::applyRules() is intended to be used also by plugins, the way Dispatcher::applyRules() works still an issue imo.

Imo rules should be removed from the Dispatcher since each plugin should manage their own rules.

Imo this issue is in a way related to #416 since it shows a possible syntax for setting a specific configuration to a bunch of routes (whether related to the the way it must be parsed, matched etc.)

So for me the Dispatcher::applyRules() feature concern more the Router class.

jails avatar Jun 04 '12 18:06 jails

Hey @jails!

I'm finally recovering from having a kid, and getting sleep again, so I had a chance to look at this. One thing that would help here is to understand how the framework looks at these parameters.

At one level are the parameters return from the Router. These are the URL parameters that are used generally in the request, to compare locations, generate URLs, etc. The parameters produced by Dispatcher::applyRules() are a transformation of these that are distinct, and only used for dispatching. This distinction is important, since route (URL) parameters may be very complex and used for many different things, but when used in dispatching, they must ultimately be resolved to a class/method pair, and therefore exist at a different level of abstraction.

This is one of the reasons why the results of applyRules() are kept separate from the Router. If they weren't, we'd have to reverse them every time we wanted to generate a URL!

The order of rules are important / The first rule inserted is the last rule applied

I don't see an inherent problem with order being important, though I think it makes sense to execute rules in the order they are added. This could probably be fixed by changing how the arrays are merged in config().

If rules of the same name are stepping on each other, we can change the array format, i.e. array(array('config' => ...)) instead of array('config' => ...). Alternatively, we could stipulate that only plugins should add rules that way (or else recommend that plugins not add rules at all, and only recommend rules for the app developer to add).

Once Dispatcher::$_rules is configured as a callback you can't add no more rules

I think a good solution would be to store the callback as $_rules[0], and ensure that it is always executed last (or even allow multiple callbacks, if we introduce arrays of "rule groups", as in the above).

The rules involved to the Dispatcher

I guess I would have to see an example to fully understand what you're talking about, but in general, plugins should only have to care about route parameters, not transformed dispatch parameters. Or are you talking about plugins implementing admin_*() methods? Would implementing something like route scopes/nested routes solve this?

If you have a chance, please add a few code examples to demonstrate this.

nateabele avatar Sep 13 '12 17:09 nateabele

Hi @nateabele, sorry for the delay. Indeed I thought to a plugin which works using a specific naming convention for its controller's action (like the 'Action' suffix (convention used by some framework) or camelcased actions). But like you said it may be simpler to recommend that plugins not add rules at all.

jails avatar Sep 20 '12 17:09 jails

@jails In that plugin, would the admin rule be "scoped" to only apply to that plugin?

nateabele avatar Sep 21 '12 21:09 nateabele

If plugins are allowed to enable their own admin_*() methods, indeed rules need something like a "scope" but with something like:

Dispatcher::rules('app', array(
    'admin' => array('action' => 'admin_{:action}')
));
Dispatcher::rules('plugin', array(
    'admin' => array('action' => 'special_admin_{:action}')
));

routes should be "scoped" too. Indeed to apply the correct rule, the parsing step need to know the ownership (i.e the library name) of each route. But I'm not sure to understand the problem right. I'm biaised by the #416 where I introduced a way to "scope" routes for solving the location problem. So I only have this solution in mind.

jails avatar Sep 21 '12 23:09 jails

@jails Was this solved with the Router scope enhancement?

gwoo avatar May 05 '13 23:05 gwoo

It's kind of a separate thing.

nateabele avatar May 05 '13 23:05 nateabele