Load controllers with FQCN.

[rjd22]

Fixes part of #571

It's BC and works by the following syntax:

Route::set('default', 'controller-name/<action>')
    ->defaults(array(
        'controller' => '\Acme\Module\Controller\Demo'
    ));

The syntax won't work but I think that when using FQCN you should not use variable controller parameters.

@acoulton or @enov can you please review if this is okay?

[enov]

I think it is ok for full BC with the old routing system.

For consistancy between the two methods, however, I would have prefered the notion of the prefix as well as the directory to go away. Not sure though how it can be done.

[kemo]

Maybe it should rather check for existance of \ anywhere else except the first char because that can also be referring to a "namespaceless" class?

So strpos($controller, '\\') might work better, this also goes for checking anywhere else. Opinions?

[rjd22]

@kemo a namespaceless class should also begin with a \ like \Kohana. Because it's still in a name space only the global one.

[kemo]

Yes but as things stand right now, those won't support directories (unless written without the initial \)

[acoulton]

@rjd22 thanks for this, my thoughts:

• What you have is right for FQCN - it makes no sense for them to support directories.
• Ideally I'd like to see support for <controller> params too - with a separate namespace param
• I don't think we should drop directory and the Controller_ prefix for legacy routes in 3.4 - people will be using modules that define routes as well as their own, and it's too big a change to ask them to update them all.
• The throw should be in Route::set, not in the request client - so that it's guaranteed to trigger with a sensible trace as soon as the developer first defines an invalid route, not later when (if) they happen to visit a matching URL.
• Obviously this would need tests and also an update to the routing docs before merge.

[acoulton]

Actually, just a thought - we could satisfy several of these cases and keep BC with a controller_format parameter that defaults to \Controller_:directory:controller.

If <controller> begins with a backslash, treat it as a FQCN with no further processing.

Otherwise, build a class name by passing the format through strtr:

# /welcome => Controller_Welcome as now
Route::set('default', '<controller>/<action>')
    ->defaults(array());

# /welcome => Controller_Other_Welcome as now
Route::set('default', '<controller>/<action>')
    ->defaults(array(
        'directory' => 'Other'
    ));

# /welcome => \My\Controller\Welcome
Route::set('default', '<controller>/<action>')
    ->defaults(array(
        'controller_format' => '\My\Controller\:controller'
    ));

# /welcome => \My\Whatever\WelcomeController
Route::set('default', '<controller>/<action>')
    ->defaults(array(
        'controller_format' => '\My\Whatever\:controllerController'
    ));

# /directory/welcome => \My\Whatever\Directory_WelcomeController - yuk, but supported
Route::set('default', '<directory>/<controller>/<action>')
    ->defaults(array(
        'controller_format' => '\My\Whatever\:directory:controllerController'
    ));

That would be fully BC, but would also support url-based controllers in namespaces with any (or no) prefix/suffix/whatever and directories if you really need them.

Thoughts?

[loonies]

@acoulton This will add another layer of complexity in the route
definition. Lets keep it simple.

I would like to see the Controller_ prefix and directory parameter
go away. The Controller_ prefix forces naming your controller classes
in a certain way, which is not flexible.

What if we introduce the namespace parameter having NULL value by
default and we just glue namespace and controller together?

Basically we use namespace instead of directory when using FQCN.

If we want to drop directory parameter and the Controller_ prefix we
should raise E_DEPRECATED in 3.4 and remove them later (e.g. 4.0).

Not tested though, but should be fully BC.

# /welcome => Controller_Welcome (BC)
Route::set('default', '<controller>/<action>')
    ->defaults(array())

# /welcome => Foo_Controller_Welcome (BC)
Route::set('default', '<controller>/<action>')
    ->defaults(array(
        'directory' => 'Foo'
    ))

# /welcome => Controller_MyWelcome (BC)
Route::set('default', 'welcome/<action>')
    ->defaults(array(
        'controller' => 'MyWelcome'
    ))

# /welcome => \Acme\Module\Ctnl\Welcome
Route::set('default', '<controller>/<action>')
    ->defaults(array(
        'namespace' =>'\Acme\Module\Ctnl'
    ))

# /welcome => \Acme\Module\Ctnl\MyWelcome
# $namespace !== NULL
Route::set('default', 'welcome/<action>')
    ->defaults(array(
        'namespace'  => '\Acme\Module\Ctnl',
        'controller' => 'MyWelcome
    ))

# /welcome => \Acme\Module\Ctnl\MyWelcome
# strpos($controller, '\\')
Route::set('default', 'welcome/<action>')
    ->defaults(array(
        'controller' => '\Acme\Module\Ctnl\MyWelcome'
    ))

[rjd22]

Shouldn't we just keep things simple for now? More complex implementations can always be implemented if there is a need for it.

I will move the check to the router and add documentation for it.

[acoulton]

@rjd22 sorry, you're right - let's just start by supporting explicit FQCN in the definition. As a side-effect that might encourage more people to use at least a route per controller, which would be no bad thing...

And we can add more complex implementations later if we need them. Obviously there's also scope for people to do that with filters now.

I think let's merge this once you've done the test/docs/moved the exception.

@loonies that would at least satisfy your "get rid of the prefix", "drop directory" and "keep it simple"?

[rjd22]

@acoulton sorry I haven't made the time to split this up. I will try to do so next weekend. Just giving a heads up.

[acoulton]

@rjd22 no worries, I hadn't made the time to even reply :)

[loonies]

Keep it simple as it is currently, without controller_format parameter
proposed by you.

We need the directory, and the Controller parameter only for BC, but
they are not needed with FQCN.

[rjd22]

@acoulton After looking I think the check is on the right spot. The router does not care about the directory or the FQCN the Kohana internals do because it has to locate the file. Maybe we should change the message the Route class just doesn't have a appropriate spot for it.

[acoulton]

@rjd22 I think objects should be responsible for validating their own state. If it's not valid for a Route to have both a FQCN controller param and a directory param, then it should refuse to accept them.

Absolutely Route shouldn't care about file location, but directory here doesn't really mean that, it means (part of the) controller name prefix. The fact that Kohana happens to look for a Controller_Directory_ class in a Controller/Directory/Controller.php path is incidental. It's not valid to specify a complete class name and a class name prefix for the same route.

If anything, the misplaced logic is where Kohana request client assembles the class name, after the Route object separately handles ucwordsing the controller and directory params. If I was building from scratch, the Route class would have complete responsibility for getting from URL + definition to class name, method name and parameters. The request client would then just be responsible for checking that class/method combination exists and handling dispatch.

From a pragmatic point of view, what's more useful if you define a route with an FQCN and a directory (classname prefix):

• an exception thrown on every request, with a trace back to the Route::set that created the invalid state?
• an exception thrown by the request client when the request happens to match, with a trace that goes nowhere near the offending code?

[rjd22]

The problem is that we're talking about default values. So we won't be able to check in route set. The only way is the check if both directory and a FQCN is set when calling defaults(). The a exception will be thrown when setting the routes. Maybe that is better?

[rjd22]

@enov & @acoulton I've moved the checks and tests. Please see if this can be merged

[acoulton]

@rjd22 looks great, but shouldn't there also be a test that Request_Client_Internal is loading the correct controller? Also would you be able to squash this to a single commit now please?

[rjd22]

@enov @acoulton it will be quite hard to test Request_Client_Internal as it is now. I swuashed the commits. Can someone please check and merge?

[acoulton]

@rjd22 I've added tests, what do you think?

[acoulton]

@rjd22 PS, very happy with your commit, all looks good to me. If we're happy with the new test style we can in future add coverage of more of the untested Request_Client_Internal behaviour.

If you or @enov are happy with the test go ahead and merge.

[rjd22]

Loons great.