Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Route: support request method in route matching and fix _match/_meta in compile #535

Closed
wants to merge 3 commits into from

5 participants

@bquilitz

Two things in this pull request

Added request method to route matching ( match() )

route matching works as follows:

  • routes defined without a request method match for all request methods
  • routes with request method GET

    Router::connect( '/get', array("http:method" => "GET", 'controller' => 'mycontroller', 'action'=> 'do_get'));

    //both match for route definition (GET is used as default)
    Router::match(array('controller' => 'mycontroller', 'action' => 'do_get', 'http:method' => 'GET'))
    Router::match(array('controller' => 'mycontroller', 'action' => 'do_get ))

  • routes with other request methods, e.g. POST

    Router::connect( '/post, array("http:method" => "POST", 'controller' => 'mycontroller', 'action'=> 'do_post'));

    // matches route definition
    Router::match(array('controller' => 'mycontroller', 'action' => 'do_post', 'http:method' => 'POST'))
    // does not match
    Router::match(array('controller' => 'mycontroller', 'action' => 'do_post'))

I could imagine some people find it nice not to pass http:method for POST etc. but I think this way of matching is more consistent with route definition. Also i actually needed it in one case where I wanted two URLS for get and post pointing to one action.

It clearly can break existing applications that do make use of http:method in the route definition and do not pass it to match().

Fixed different behavior of for _match/_meta depending of the presence of parameters in route templates.

In cases the route template contained parameters _meta (i.e. options with ':' were handled differently than if no parameters we used ( Route->compile() )

Also created tests for routes with specified request method.

First change clearly can break existing applications that do make use of http:method in the route definition and do not pass it to match(). It should be discussed . Second change (_meta) should be applied anyways as it seems to be a bug.

bquilitz added some commits
@bquilitz bquilitz added request method to route matching and fixed different behaviour …
…of for _match/_meta depending of the presence of parameters in route templates.
5c61d35
@bquilitz bquilitz added additional tests to route f793c4f
net/http/Route.php
@@ -273,6 +273,12 @@ public function match(array $options = array(), $context = null) {
}
}
+ // check http method
+ if (isset($this->_meta['http:method']) && $options['http:method'] != $this->_meta['http:method'] ) {
@nateabele Owner

Please remove the inline comment, and the extra space here: ['http:method'] ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nateabele nateabele commented on the diff
tests/cases/net/http/RouteTest.php
((61 lines not shown))
+ //success
+ $result = $route->match(array('controller' => 'resource', 'action' => 'create', 'http:method' => 'GET'));
+ $this->assertEqual('/resource', $result);
+
+ // success even with missing GET because it's the default
+ $result = $route->match(array('controller' => 'resource', 'action' => 'create'));
+ $this->assertEqual('/resource', $result);
+
+ // other request methods should fail
+ $result = $route->match(array('controller' => 'resource', 'action' => 'create', 'http:method' => 'POST'));
+ $this->assertEqual(false, $result);
+ $result = $route->match(array('controller' => 'resource', 'action' => 'create', 'http:method' => 'PUT'));
+ $this->assertEqual(false, $result);
+
+
+ }
@nateabele Owner

Extra newlines?

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

thanks for the feedback. Read your comment on the tests after working on the other annotations, will break the tests out into separate methods

@nateabele
Owner

Oh, also: please submit this PR against the dev branch for integration. Thanks!

@gwoo
Owner

@bquilitz have you had a chance to update this PR? If so, can you change the base to dev instead of master. Thanks!

@mehlah

Related to #79

@bquilitz

I'll do some minor changes and resubmit the next days.

@nateabele
Owner

@jails After thinking about it for a minute, I actually think we should just close this. It is already possible to match on the HTTP request method, and based on my work on li3_resources, I've come to realize that handling HTTP methods at the routing level is conceptually wrong.

@jails jails closed this
@jails jails referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@jails jails referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@jails jails referenced this pull request from a commit in jails/lithium
@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
- Modifiying the Media::webroot() signature (BC Break)
- 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.
648e172
@jails jails referenced this pull request from a commit in jails/lithium
@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
- Modifiying the Media::webroot() signature (BC Break)
- 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.
9963274
@jails jails referenced this pull request from a commit in jails/lithium
@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
- Modifiying the Media::webroot() signature (BC Break)
- 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.
ddfa316
@jails jails referenced this pull request from a commit in jails/lithium
@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
- Modifiying the Media::webroot() signature (BC Break)
- 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.
bd5b2b2
@jails jails referenced this pull request from a commit in jails/lithium
@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.
66bd5d3
@jails jails referenced this pull request from a commit in jails/lithium
@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.
720ded6
@jails jails referenced this pull request from a commit in jails/lithium
@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
@Ciaro Ciaro referenced this pull request from a commit
@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.
42e326b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 11, 2012
  1. @bquilitz

    added request method to route matching and fixed different behaviour …

    bquilitz authored
    …of for _match/_meta depending of the presence of parameters in route templates.
  2. @bquilitz
Commits on Jun 13, 2012
  1. @bquilitz

    reformat code

    bquilitz authored
This page is out of date. Refresh to see the latest.
Showing with 81 additions and 3 deletions.
  1. +8 −3 net/http/Route.php
  2. +73 −0 tests/cases/net/http/RouteTest.php
View
11 net/http/Route.php
@@ -260,7 +260,7 @@ public function parse($request, array $options = array()) {
* @return mixed
*/
public function match(array $options = array(), $context = null) {
- $defaults = array('action' => 'index');
+ $defaults = array('action' => 'index', 'http:method' => 'GET');
$query = null;
if (!$this->_config['continue']) {
@@ -273,6 +273,11 @@ public function match(array $options = array(), $context = null) {
}
}
+ if (isset($this->_meta['http:method']) && $options['http:method'] != $this->_meta['http:method']) {
+ return false;
+ }
+ unset($options['http:method']);
+
if (!$options = $this->_matchKeys($options)) {
return false;
}
@@ -402,8 +407,6 @@ public function export() {
* @return void
*/
public function compile() {
- $this->_match = $this->_params;
-
foreach ($this->_params as $key => $value) {
if (!strpos($key, ':')) {
continue;
@@ -412,6 +415,8 @@ public function compile() {
$this->_meta[$key] = $value;
}
+ $this->_match = $this->_params;
+
if ($this->_template === '/' || $this->_template === '') {
$this->_pattern = '@^/*$@';
return;
View
73 tests/cases/net/http/RouteTest.php
@@ -543,6 +543,79 @@ public function testMatchingEmptyRoute() {
}
/**
+ * Test route matching for routes with specified request method (http:method)
+ */
+ public function testMatchWithRequestMethod() {
+ $parameters = array('controller' => 'resource', 'action' => 'create');
+
+ $route = new Route(array(
+ 'template' => '/resource',
+ 'params' => $parameters + array('http:method' => 'POST')
+ ));
+
+ //success
+ $result = $route->match(array('controller' => 'resource', 'action' => 'create', 'http:method' => 'POST'));
+ $this->assertEqual('/resource', $result);
+
+ // no match because of missing POST
+ $result = $route->match(array('controller' => 'resource', 'action' => 'create'));
+ $this->assertEqual(false, $result);
+
+
+ // we test params in route here because this made a difference in the original li3 routing implementation
+ $route = new Route(array(
+ 'template' => '/{:param}',
+ 'params' => $parameters + array('http:method' => 'POST')
+ ));
+
+ $result = $route->match(array('controller' => 'resource', 'action' => 'create', 'param' => 'value', 'http:method' => 'POST'));
+ $this->assertEqual('/value', $result);
+
+ // no match because of missing POST
+ $result = $route->match(array('controller' => 'resource', 'action' => 'create', 'param' => 'value'));
+ $this->assertEqual(false, $result);
+
+
+ // route without request method matches all
+ $route = new Route(array(
+ 'template' => '/resource',
+ 'params' => $parameters
+ ));
+
+ $result = $route->match(array('controller' => 'resource', 'action' => 'create'));
+ $this->assertEqual('/resource', $result);
+ $result = $route->match(array('controller' => 'resource', 'action' => 'create', 'http:method' => 'GET'));
+ $this->assertEqual('/resource', $result);
+ $result = $route->match(array('controller' => 'resource', 'action' => 'create', 'http:method' => 'POST'));
+ $this->assertEqual('/resource', $result);
+ $result = $route->match(array('controller' => 'resource', 'action' => 'create', 'http:method' => 'PUT'));
+ $this->assertEqual('/resource', $result);
+
+
+ // Test route with request method GET
+ $route = new Route(array(
+ 'template' => '/resource',
+ 'params' => $parameters + array('http:method' => 'GET')
+ ));
+
+ //success
+ $result = $route->match(array('controller' => 'resource', 'action' => 'create', 'http:method' => 'GET'));
+ $this->assertEqual('/resource', $result);
+
+ // success even with missing GET because it's the default
+ $result = $route->match(array('controller' => 'resource', 'action' => 'create'));
+ $this->assertEqual('/resource', $result);
+
+ // other request methods should fail
+ $result = $route->match(array('controller' => 'resource', 'action' => 'create', 'http:method' => 'POST'));
+ $this->assertEqual(false, $result);
+ $result = $route->match(array('controller' => 'resource', 'action' => 'create', 'http:method' => 'PUT'));
+ $this->assertEqual(false, $result);
+
+
+ }
@nateabele Owner

Extra newlines?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ /**
* Tests that routes with optional trailing elements have unnecessary slashes trimmed.
*/
public function testTrimmingEmptyPathElements() {
Something went wrong with that request. Please try again.