New Router Feature #878

Merged
merged 3 commits into from Apr 13, 2013

Conversation

Projects
None yet
7 participants
Contributor

jails commented Apr 6, 2013

Hi guys I need your opinion on the following feature. Since my first li3 application, I always feel limited with URL & Routes managment in general. I started my reflexion with #416 and today I would like to share what I've done.

Why enhancing the Router class ?

A good practice of nowadays development is the separation of concerns. For example splitting a monolithic application to some dedicated libraries is a way to increase this separation of concerns. If Lithium already own a solid plugins system, The Router class still an hurdle in the road since routes can't be separated. Who never had conflicts among a couple of routes (especially between app & admin routes).

So to keep routes "isolated" in an application or between libaries this PR introduce the concept of scopes and attachments. This mean you can group a bunch of routes inside a scope and attach them to an arbitraty url.

What awesome would be if I can "mount" a kind of forum library on a 'forum' subdomain or another specific URL ?

So How does it works ?

Previously you connect routes using the following syntax :

Router::connect('/', 'Pages::view');
Router::connect('/pages/{:args}', 'Pages::view');

To take profit of the scopes, you need to use the following scoping notation:

Router::scope('app', function() {
    Router::connect('/ambiguous/{:action}');
    Router::connect('/{:controller}/{:action}/{:args}');
});

Router::scope('test', function() {
    Router::connect('/ambiguous/{:action}');
    Router::connect('/test/{:args}', array('controller' => 'lithium\test\Controller'));
    Router::connect('/test', array('controller' => 'lithium\test\Controller'));
});

Note : the above routes should be in different routes.php files, but it' ok for the example.

What does it change ?

Well nothing yet. Everything works and the only difference is that Router::scope() will now returns the scope of the dispatched route.

For example with '/controller/action', Router::scope() will return 'app',
with '/test/action', Router::scope() will return 'test', and with '/ambiguous/index' it'll return 'app'.

Indeed, since the app location is not defined right now, Router::process() can't resolve the ambiguity (i.e same behavior as before)

So what can i do with that ?

So the next step is to attach scopes to kind of "mount points". For example the following example will use a prefix for the 'app' scope:

Router::attach('app', array('prefix' => 'myprefix'));

This mean this routes will only match on urls like http://www.domain.com/myprefix.

But you can set the mount point based on subdomain using absolute location:

Router::attach('app', array(
    'absolute' => true,
    'host' => 'mysubdomain.mysite.com',
    'scheme' => 'http://'
));

You can also use variables for managing subdomains like : <username>.mysite.com for example:

Router::attach('app', array(
    'absolute' => true,
    'host' => '{:subdomain:[a-z]+}.mysite.com',
    'scheme' => 'http://'
));

How can i write a link from a location to a different location ?

Since Router::process() detect the scope, the links in your templates will be generated according the scope defined with Router::attach().

Suppose you browse '/controller/action' and you need a link to the 'test' scope in your view, you only need to set the 'scope' attribute :

$this->html->link('test', '/test', array('scope' => 'test'));

Ok but how does the variables in location works ?

Suppose you wan't to manage subdomains like : <username>.mysite.com with the following configuration :

Router::attach('app', array(
    'absolute' => true,
    'host' => '{:subdomain:[a-z]+}.mysite.com',
    'scheme' => 'http://'
));

with 'http://bob.mysite.com/controller/action', all your links will start with 'http://bob.mysite.com/' but you can override the default scope params with the following syntax :

$this->html->link('Max home', '/home/index', array(
    'scope' => array(
        'app' => array(
            'subdomain' => 'max'
        )
    )
));

Since here max is in current scope you could simply write :

$this->html->link('Max home', '/home/index', array(
    'scope' => array(
        'subdomain' => 'max'
    )
));

Media class wants in !

Of course separating routes is fine but you can also separate the Medias with the same scoping system :

Media::attach('cdncss', array(
    'absolute' => true,
    'host' => 'www.cdn.com',
    'scheme' => 'http://',
    'prefix' => 'web/assets/'
));

Media::attach('cdnimage', array(
    'absolute' => true,
    'host' => 'www.cdn.com',
    'scheme' => 'http://',
    'prefix' => 'web/assets/'
));

The attachement above create some usable scopes for views.

$this->html->style('style.css', array('scope' => 'cdncss'));
$this->html->image('test.gif', array('scope' => 'cdnimage'));

See tests for complete features

How I can play a bit with it ?

Pretty simple just run the following commands in your document root:

git clone https://github.com/jails/li3_bigbang myapp
cd myapp
composer install
chmod -R 777 atoms/app/resources
chmod -R 777 atoms/admin/resources

And open your browser at: http://localhost/myapp/

Feel free to comment and to give your opinion on this.

Thank you for reading !

Member

blainesch commented Apr 6, 2013

This will make managing link hosts and cdn's a ton easier! 👍

SayB commented Apr 6, 2013

this is brilliant ! ... yet more abstraction ... asset management would be a breeze

Member

nervetattoo commented Apr 6, 2013

Loving it! Major thumbs up.
Maybe it could be shorthanded for writing the routes initially as well?

Router::scope('test', [
    '/ambiguous/{:action}',
    '/test/{:args}' => array('controller' => 'lithium\test\Controller'),
    '/test' => array('controller' => 'lithium\test\Controller')
]);
Contributor

jails commented Apr 6, 2013

@nervetattoo right ! Since the third parmeter of Router::connect() is still rarely used, it could be an awesome shorthanded syntax !

Member

rapzo commented Apr 6, 2013

I love this!
This will avoid the messy if clauses i usually end up writing when setting up routes.
Way to go Guru 👍 awesome work!

One question: any special behavior for continuation routes? Will they only apply for the given scope (or for everything if no scope is given)?

Contributor

jails commented Apr 6, 2013

Right! continuation routes are scope based. If a scope is defined outside a scope it'll only be applied to all "non scoped routes".

Contributor

ericcholis commented Apr 8, 2013

If anything, this would make admin routes a breeze. I'm sure many LI3 devs would appreciate this change.

blainesch referenced this pull request Apr 8, 2013

Merged

Feature/router array cdn #879

@nateabele nateabele commented on an outdated diff Apr 9, 2013

net/http/Media.php
+ */
+ public static function attached($name = null) {
+ if (!isset(static::$_scopes)) {
+ static::_initScopes();
+ }
+ if ($name === false) {
+ $name = '__defaultScope__';
+ }
+ return static::$_scopes->get($name);
+ }
+
+ /**
+ * Initialize `static::$_scopes` with a `lithium\core\Configuration` instance.
+ */
+ protected static function _initScopes() {
+ $configuration = static::$_classes['configuration'];
@nateabele

nateabele Apr 9, 2013

Owner

This should be static::$_scopes = static::_instance('configuration').

@nateabele nateabele commented on an outdated diff Apr 9, 2013

net/http/Route.php
@@ -270,6 +274,12 @@ public function match(array $options = array(), $context = null) {
unset($options['?']);
}
}
+ if (isset($this->_meta['http:method']) &&
@nateabele

nateabele Apr 9, 2013

Owner

This should probably be reformatted.

@nateabele nateabele commented on the diff Apr 9, 2013

net/http/Router.php
* ));
* }}}
*
- * _Note_: Because formatters are copied to `Route` objects on an individual basis, make sure
@nateabele

nateabele Apr 9, 2013

Owner

This breaks proper formatting for li3_docs.

@nateabele nateabele commented on the diff Apr 9, 2013

net/http/Media.php
+ *
+ * Special use case: If `$closure` is not null executing the closure inside
+ * the specified scope.
+ *
+ * @param string $name Name of the scope to use.
+ * @param array $closure A closure to execute inside the scope.
+ * @return mixed Returns the previous scope if if `$name` is not null and `$closure` is null,
+ * returns the default used scope if `$name` is null, otherwise returns `null`.
+ */
+ public static function scope($name = null, Closure $closure = null) {
+ if ($name === null) {
+ return static::$_scope;
+ }
+
+ if ($closure === null) {
+ $former = static::$_scope;
@nateabele

nateabele Apr 9, 2013

Owner

What's the use case for arbitrarily changing the scope? Having a mutable global value like this makes me nervous...

@jails

jails Apr 9, 2013

Contributor

Haha, yeah, it has been done to allow the following syntax:

Router::scope('app', function() { //<- Global scope is changed here to `'app'`
    Router::connect('/ambiguous/{:action}'); //<- `Router::connect` attach the route to the `'app'` scope
});

The other option is to pass the scope option on each Router::connect:

Router::connect('/ambiguous/{:action}', array(), array('scope' => 'app'));

which is a little bit more verbose imo.

@nateabele

nateabele Apr 12, 2013

Owner

Okay, I guess that's fine, since it's unlikely that the closure passed to Router::scope() would call anything out-of-band to modify it.

@nateabele nateabele commented on an outdated diff Apr 9, 2013

net/http/Router.php
- continue;
+ foreach (static::$_configurations[$name] as $route) {
+ if (!$match = $route->parse($request, compact('url'))) {
+ continue;
+ }
+ $request = $match;
+ if ($route->canContinue() && isset($request->params['args'])) {
+ $url = '/' . join('/', $request->params['args']);
+ unset($request->params['args']);
+ continue;
+ }
+
+ if (isset($request->params['controller'])) {
+ $controller = $request->params['controller'];
+ if (isset($config['namespace']) && strpos($controller, '\\') === false) {
+ $controller = $config['namespace'] . '\\' . $controller;
@nateabele

nateabele Apr 9, 2013

Owner

I don't understand this rule. A controller can be any arbitrary class. For example, this seems like it would break Resource objects in li3_resources. This whole control structure seems very deeply-nested and contains many hard-coded rules that are hard to generalize or abstract.

@nateabele nateabele commented on an outdated diff Apr 9, 2013

net/http/Router.php
+ }
+ $request = $match;
+ if ($route->canContinue() && isset($request->params['args'])) {
+ $url = '/' . join('/', $request->params['args']);
+ unset($request->params['args']);
+ continue;
+ }
+
+ if (isset($request->params['controller'])) {
+ $controller = $request->params['controller'];
+ if (isset($config['namespace']) && strpos($controller, '\\') === false) {
+ $controller = $config['namespace'] . '\\' . $controller;
+ $request->params['controller'] = $controller . 'Controller';
+ }
+ if (isset($config['library'])) {
+ $request->params['library'] = $config['library'];
@nateabele

nateabele Apr 9, 2013

Owner

What makes 'library' special? If there's a list of keys that should be copied from $config to $request->params, shouldn't that list be configurable?

@nateabele nateabele and 1 other commented on an outdated diff Apr 9, 2013

net/http/Router.php
* @param array $options Options for the generation of the matched URL. Currently accepted
- * values are:
- * - `'absolute'` _boolean_: Indicates whether or not the returned URL should be an
- * absolute path (i.e. including scheme and host name).
- * - `'host'` _string_: If `'absolute'` is `true`, sets the host name to be used,
- * or overrides the one provided in `$context`.
- * - `'scheme'` _string_: If `'absolute'` is `true`, sets the URL scheme to be
- * used, or overrides the one provided in `$context`.
+ * values are:
+ * - `'absolute'` _boolean_: Indicates whether or not the returned URL should be an
+ * absolute path (i.e. including scheme and host name).
+ * - `'host'` _string_: If `'absolute'` is `true`, sets the host name to be used,
+ * or overrides the one provided in `$context`.
+ * - `'scheme'` _string_: If `'absolute'` is `true`, sets the URL scheme to be
+ * used, or overrides the one provided in `$context`.
* @return string Returns a generated URL, based on the URL template of the matched route, and
* prefixed with the base URL of the application.
*/
public static function match($url = array(), $context = null, array $options = array()) {
@nateabele

nateabele Apr 9, 2013

Owner

This method exceeds the cyclomatic complexity limit and should be refactored.

@blainesch

blainesch Apr 9, 2013

Member

What is the set max limit for complexity?

@nateabele

nateabele Apr 9, 2013

Owner

10 (it's in the Complexity test filter), but I don't count simple ternaries.

@blainesch

blainesch Apr 9, 2013

Member

Cyclomatic complexity can be determined via static analysis, maybe we should have a rule for it? We already have a class to calculate it via an object.

I'm still slightly confused about it however. I may be getting a little off-topic here but my understand is that the number is how many paths I can go through.

However in our complexity, and codesniffers they don't count "else" I assume this is because else has tokens T_ELSE and else if has both T_ELSE and T_IF which might result in double counting unless we check if it's followed immediately by a space and T_IF.

We are also counting all T_CASE tokens as a path, but every case may not be a different path:

switch(true) {
    case 1:
    case true:
        foo();
    break;
    default:
        baz();
    break();
}

This has 3 cases, but 2 paths to go through. Is a simple count 'good enough'? Or should we go a little further and also check context here as well?

@nateabele

nateabele Apr 9, 2013

Owner

@blainesch I think you're thinking of N-Path complexity, which actually counts the total number of different paths a method can take (including counting if ($a || $b) as two possible separate paths, since two different possibilities can cause the given code path to be taken).

Cyclomatic complexity, on the other hand, merely counts the number of times the code branches. Thus, T_ELSE (and T_DEFAULT, for that matter) aren't counted, because they're assumed to be the default code path. Take a look at the Complexity class in the test namespace. It implements a cyclomatic complexity counter already and is pretty straightforward.

@blainesch

blainesch Apr 9, 2013

Member

@nateabele too many complexity algorithms to keep track of. However, in Complexity we are checking T_DEFAULT should this be removed? Source

@nateabele

nateabele Apr 9, 2013

Owner

@blainesch Good catch. I suppose so, yeah.

@nateabele nateabele commented on the diff Apr 9, 2013

tests/cases/action/RequestTest.php
public function testUrlFromConstructor() {
$request = new Request(array('url' => 'posts/1'));
- $expected = 'posts/1';
+ $expected = '/posts/1';
@nateabele

nateabele Apr 9, 2013

Owner

Why do we always have leading slashes now?

@jails

jails Apr 9, 2013

Contributor

It was more for consistency since only some had a leading. And this way:

$url = $request->host . $request->env('base') . $request->url;

should work w/o using trim() to prevent '//' or missing slash.

jails added some commits Feb 15, 2013

@jails jails Change `action\Request` behavior.
- WARNING BC BREAK: Defining the url in `$_GET['url']` is no more supported.
- Enhance REQUEST_URI based request.
- Environmement variables are populated in attributes.
- If the option `'globals'` is set to `false`, $_SERVER, $_ENV, $_GET & $_POST are not added to the Request.
- Remove infinite loop on missed PHP_SELF & SCRIPT_FILENAME
bd82155
@jails jails Add scope to `lithium\net\http\Router` and `lithium\net\http\Media`.
- Routes can be scoped using the following notation:
  Routed::scope(); // Returns the current scope
  Router::scope('name'); // Use a new scope
  Router::scope('name'), function(){/* executed inside the scope */});
  Router::attach('name'), array(), array()); //Attach a mount point to a scope
  Router::attached() // Returns all attachments
  Router::attached('name', array()); // Returns the attached mount point configuration
- The Media class can be scoped using the following notation: (i.e. usefull for CDN or others media location).
  Media::scope(); // Returns the current scope
  Media::scope('name'); // Use a new scope
  Media::scope('name'), function(){/* executed inside the scope */});
  Media::attach('name'), array()); // Attach a mount point to a scope
  Media::attached(); // Returns all attachements
  Media::attached('name'); // Returns the attached mount point configuration
- The paths of assets paths now called 'paths' for consistency see `Media::_asset` (BC Break)
- Include #535 it's not a good practice to use `'http:method'` at route level but it may be better to not let it buggy.
67c450c
Contributor

jails commented Apr 9, 2013

@nateabele since it's a big PR, I added an extra commit to make it simpler for you to review the changes. But if it's ok I'll squash the two last commit. ;-)

nateabele merged commit b3b7c06 into UnionOfRAD:dev Apr 13, 2013

1 check passed

default The Travis build passed
Details

SayB commented on bd82155 Apr 30, 2013

This commit breaks any prefixes that start with the letter 'a' - Here is the issue I've files:

#908

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment