Skip to content

Commit

Permalink
Removing inflections out from the router, this also means that default
Browse files Browse the repository at this point in the history 
routes need to change or be treated differently
  • Loading branch information
@lorenzo
lorenzo committed Jun 9, 2014
1 parent d46bdf1 commit 5a0a796
Show file tree
Hide file tree
Showing 7 changed files with 34 additions and 35 deletions.
2 changes: 1 addition & 1 deletion src/Console/Command/Task/ViewTask.php
View file
Expand Up @@ -431,7 +431,7 @@ protected function _associations(Table $model) {
'primaryKey' => (array)$target->primaryKey(),
'displayField' => $target->displayField(),
'foreignKey' => $assoc->foreignKey(),
'controller' => Inflector::underscore($alias),
'controller' => $alias,
'fields' => $target->schema()->columns(),
];
}
Expand Down
6 changes: 3 additions & 3 deletions src/Routing/Filter/ControllerFactoryFilter.php
View file
Expand Up @@ -59,13 +59,13 @@ protected function _getController($request, $response) {
$pluginPath = $controller = null;
$namespace = 'Controller';
if (!empty($request->params['plugin'])) {
$pluginPath = Inflector::camelize($request->params['plugin']) . '.';
$pluginPath = $request->params['plugin'] . '.';
}
if (!empty($request->params['controller'])) {
$controller = Inflector::camelize($request->params['controller']);
$controller = $request->params['controller'];
}
if (!empty($request->params['prefix'])) {
$namespace .= '/' . Inflector::camelize($request->params['prefix']);
$namespace .= '/' . ucfirst($request->params['prefix']);
}
$className = false;
if ($pluginPath . $controller) {
Expand Down
12 changes: 6 additions & 6 deletions tests/TestCase/Routing/DispatcherTest.php
View file
Expand Up @@ -322,7 +322,7 @@ public function testDispatchActionReturnsResponse() {
$request = new Request([
'url' => 'some_pages/responseGenerator',
'params' => [
'controller' => 'some_pages',
'controller' => 'SomePages',
'action' => 'responseGenerator',
'pass' => []
]
Expand All @@ -345,8 +345,8 @@ public function testPrefixDispatch() {
$request = new Request([
'url' => 'admin/posts/index',
'params' => [
'prefix' => 'admin',
'controller' => 'posts',
'prefix' => 'Admin',
'controller' => 'Posts',
'action' => 'index',
'pass' => [],
'return' => 1
Expand Down Expand Up @@ -375,9 +375,9 @@ public function testPrefixDispatchPlugin() {
$request = new Request([
'url' => 'admin/test_plugin/comments/index',
'params' => [
'plugin' => 'test_plugin',
'prefix' => 'admin',
'controller' => 'comments',
'plugin' => 'TestPlugin',
'prefix' => 'Admin',
'controller' => 'Comments',
'action' => 'index',
'pass' => [],
'return' => 1
Expand Down
6 changes: 2 additions & 4 deletions tests/TestCase/Routing/Filter/CacheFilterTest.php
View file
Expand Up @@ -63,9 +63,7 @@ public static function cacheActionProvider() {
return array(
array('/'),
array('test_cached_pages/index'),
array('TestCachedPages/index'),
array('test_cached_pages/test_nocache_tags'),
array('TestCachedPages/test_nocache_tags'),
array('test_cached_pages/view/param/param'),
array('test_cached_pages/view?q=cakephp'),
);
Expand All @@ -84,8 +82,8 @@ public function testFullPageCachingDispatch($url) {
Configure::write('debug', true);

Router::reload();
Router::connect('/', array('controller' => 'test_cached_pages', 'action' => 'index'));
Router::connect('/:controller/:action/*');
Router::connect('/', array('controller' => 'TestCachedPages', 'action' => 'index'));
Router::connect('/test_cached_pages/:action/*', ['controller' => 'TestCachedPages']);

$dispatcher = new Dispatcher();
$dispatcher->addFilter(new RoutingFilter());
Expand Down
38 changes: 20 additions & 18 deletions tests/TestCase/Routing/RequestActionTraitTest.php
View file
Expand Up @@ -44,6 +44,8 @@ public function setUp() {
DispatcherFactory::add('Routing');
DispatcherFactory::add('ControllerFactory');
$this->object = $this->getObjectForTrait('Cake\Routing\RequestActionTrait');
Router::connect('/request_action/:action/*', ['controller' => 'RequestAction']);
Router::connect('/tests_apps/:action/*', ['controller' => 'TestsApps']);
}

/**
Expand All @@ -54,6 +56,7 @@ public function setUp() {
public function tearDown() {
parent::tearDown();
DispatcherFactory::clear();
Router::reload();
}

/**
Expand Down Expand Up @@ -105,6 +108,7 @@ public function testRequestAction() {
public function testRequestActionPlugins() {
Plugin::load('TestPlugin');
Router::reload();
Router::connect('/test_plugin/tests/:action/*', ['controller' => 'Tests', 'plugin' => 'TestPlugin']);

$result = $this->object->requestAction('/test_plugin/tests/index', array('return'));
$expected = 'test plugin index';
Expand All @@ -115,7 +119,7 @@ public function testRequestActionPlugins() {
$this->assertEquals($expected, $result);

$result = $this->object->requestAction(
array('controller' => 'tests', 'action' => 'index', 'plugin' => 'test_plugin'), array('return')
array('controller' => 'Tests', 'action' => 'index', 'plugin' => 'TestPlugin'), array('return')
);
$expected = 'test plugin index';
$this->assertEquals($expected, $result);
Expand All @@ -125,7 +129,7 @@ public function testRequestActionPlugins() {
$this->assertEquals($expected, $result);

$result = $this->object->requestAction(
array('controller' => 'tests', 'action' => 'some_method', 'plugin' => 'test_plugin')
array('controller' => 'Tests', 'action' => 'some_method', 'plugin' => 'TestPlugin')
);
$expected = 25;
$this->assertEquals($expected, $result);
Expand All @@ -140,41 +144,41 @@ public function testRequestActionArray() {
Plugin::load(array('TestPlugin'));

$result = $this->object->requestAction(
array('controller' => 'request_action', 'action' => 'test_request_action')
array('controller' => 'RequestAction', 'action' => 'test_request_action')
);
$expected = 'This is a test';
$this->assertEquals($expected, $result);

$result = $this->object->requestAction(
array('controller' => 'request_action', 'action' => 'another_ra_test'),
array('controller' => 'RequestAction', 'action' => 'another_ra_test'),
array('pass' => array('5', '7'))
);
$expected = 12;
$this->assertEquals($expected, $result);

$result = $this->object->requestAction(
array('controller' => 'tests_apps', 'action' => 'index'), array('return')
array('controller' => 'TestsApps', 'action' => 'index'), array('return')
);
$expected = 'This is the TestsAppsController index view ';
$this->assertEquals($expected, $result);

$result = $this->object->requestAction(array('controller' => 'tests_apps', 'action' => 'some_method'));
$result = $this->object->requestAction(array('controller' => 'TestsApps', 'action' => 'some_method'));
$expected = 5;
$this->assertEquals($expected, $result);

$result = $this->object->requestAction(
array('controller' => 'request_action', 'action' => 'normal_request_action')
array('controller' => 'RequestAction', 'action' => 'normal_request_action')
);
$expected = 'Hello World';
$this->assertEquals($expected, $result);

$result = $this->object->requestAction(
array('controller' => 'request_action', 'action' => 'paginate_request_action')
array('controller' => 'RequestAction', 'action' => 'paginate_request_action')
);
$this->assertNull($result);

$result = $this->object->requestAction(
array('controller' => 'request_action', 'action' => 'paginate_request_action'),
array('controller' => 'RequestAction', 'action' => 'paginate_request_action'),
array('pass' => array(5))
);
$this->assertNull($result);
Expand All @@ -201,7 +205,7 @@ public function testRequestActionParamParseAndPass() {
$result = $this->object->requestAction('/request_action/params_pass');
$result = json_decode($result, true);
$this->assertEquals('request_action/params_pass', $result['url']);
$this->assertEquals('request_action', $result['params']['controller']);
$this->assertEquals('RequestAction', $result['params']['controller']);
$this->assertEquals('params_pass', $result['params']['action']);
$this->assertNull($result['params']['plugin']);
}
Expand All @@ -216,12 +220,12 @@ public function testRequestActionNoPostPassing() {
$_POST = array(
'item' => 'value'
);
$result = $this->object->requestAction(array('controller' => 'request_action', 'action' => 'post_pass'));
$result = $this->object->requestAction(array('controller' => 'RequestAction', 'action' => 'post_pass'));
$result = json_decode($result, true);
$this->assertEmpty($result);

$result = $this->object->requestAction(
array('controller' => 'request_action', 'action' => 'post_pass'),
array('controller' => 'RequestAction', 'action' => 'post_pass'),
array('post' => $_POST)
);
$result = json_decode($result, true);
Expand All @@ -235,18 +239,16 @@ public function testRequestActionNoPostPassing() {
* @return void
*/
public function testRequestActionWithQueryString() {
Router::reload();
require CAKE . 'Config/routes.php';
$query = ['page' => 1, 'sort' => 'title'];
$result = $this->object->requestAction(
['controller' => 'request_action', 'action' => 'query_pass'],
['controller' => 'RequestAction', 'action' => 'query_pass'],
['query' => $query]
);
$result = json_decode($result, true);
$this->assertEquals($query, $result);

$result = $this->object->requestAction([
'controller' => 'request_action',
'controller' => 'RequestAction',
'action' => 'query_pass',
'?' => $query
]);
Expand All @@ -271,7 +273,7 @@ public function testRequestActionPostWithData() {
'Post' => array('id' => 2)
);
$result = $this->object->requestAction(
array('controller' => 'request_action', 'action' => 'post_pass'),
array('controller' => 'RequestAction', 'action' => 'post_pass'),
array('post' => $data)
);
$result = json_decode($result, true);
Expand All @@ -298,7 +300,7 @@ public function testRequestActionGetParameters() {
$this->assertEquals('value', $result['query']['get']);

$result = $this->object->requestAction(
array('controller' => 'request_action', 'action' => 'params_pass'),
array('controller' => 'RequestAction', 'action' => 'params_pass'),
array('query' => array('get' => 'value', 'limit' => 5))
);
$result = json_decode($result, true);
Expand Down
3 changes: 2 additions & 1 deletion tests/TestCase/Routing/RouterTest.php
View file
Expand Up @@ -38,7 +38,6 @@ class RouterTest extends TestCase {
public function setUp() {
parent::setUp();
Configure::write('Routing', array('admin' => null, 'prefixes' => []));
Router::reload();
Router::fullbaseUrl('');
Configure::write('App.fullBaseUrl', 'http://localhost');
}
Expand All @@ -53,6 +52,7 @@ public function tearDown() {
Plugin::unload();
Router::fullBaseUrl('');
Configure::write('App.fullBaseUrl', 'http://localhost');
Router::reload();
}

/**
Expand Down Expand Up @@ -1507,6 +1507,7 @@ public function testRouteSymmetry() {
* @dataProvider parseReverseSymmetryData
*/
public function testParseReverseSymmetry($url) {
require CAKE . 'Config/routes.php';
$this->assertSame($url, Router::reverse(Router::parse($url) + array('url' => [])));
}

Expand Down
2 changes: 0 additions & 2 deletions tests/test_app/TestApp/Config/routes.php
View file
Expand Up @@ -26,5 +26,3 @@
Router::connect('/some_alias', array('controller' => 'tests_apps', 'action' => 'some_method'));

Router::connect('/', ['controller' => 'pages', 'action' => 'display', 'home']);

require CAKE . 'Config/routes.php';

7 comments on commit 5a0a796

@spiliot
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this, if in routes.php you have controller=>'pages' instead of controller=>'Pages' the message is "Error: PagesController could not be found" with instructions to "Create the class PagesController below in file: App/Controller/PagesController.php". But obviously the file is there and the error message can prove quite misleading :)

@markstory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the error message isn't ideal. The exception class is inflecting the routing param which is going to be misleading, I can remove the un-necessary inflection in the error message.

@spiliot
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I read you right, that means the suggestion would then be to create a pagesController instead. This could lead to more confusion IMO.

@ADmad
Copy link
Member

@ADmad ADmad commented on 5a0a796 Jun 19, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So add a google style "Did you mean ...."? :)

@spiliot
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I follow correctly, this exception will fire if

  1. A user mistypes a url or a correct typed redirect is given but autorouting can't find the controller (the message is correct)
  2. Routes or redirects are typed erroneously (the message is misleading)

I think it's simpler to add a check for a lowercase controller name and display an appropriate message, which will catch number 2 above. This way inflection can be removed from calls of type $this->redirect(['controller'=>'test']) (if not removed already).

@markstory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spiliot That is not what I meant at all about creating a pagesController that doesn't follow any conventions that CakePHP has. I meant that the error page is incorrect and should handle situations where developers make mistakes defining their routes.

A 404 - which missing controller error is - will be thrown in the situations you described, however the redirect() and url() calls should be caught and converted into exceptions before a URL is generated. This work is not yet complete, but I'm planning on starting it very soon, along with other changes to Router/routing.

@spiliot
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent! Thanks

Please sign in to comment.