Merge pull request #1703 from robbieaverill/feature/psr2-compliant-controller-names

API Allow controller discovery without underscore (PSR-2 compliance)

.upgrade.yml

@@ -35,9 +35,9 @@ mappings:
   SiteTreeURLSegmentField_Readonly: SilverStripe\CMS\Forms\SiteTreeURLSegmentField_Readonly
   CurrentPageIdentifier: SilverStripe\CMS\Model\CurrentPageIdentifier
   ErrorPage: SilverStripe\CMS\Model\ErrorPage
-  ErrorPage_Controller: SilverStripe\CMS\Model\ErrorPage_Controller
+  ErrorPage_Controller: SilverStripe\CMS\Model\ErrorPageController
   RedirectorPage: SilverStripe\CMS\Model\RedirectorPage
-  RedirectorPage_Controller: SilverStripe\CMS\Model\RedirectorPage_Controller
+  RedirectorPage_Controller: SilverStripe\CMS\Model\RedirectorPageController
   SiteTree: SilverStripe\CMS\Model\SiteTree
   SiteTreeExtension: SilverStripe\CMS\Model\SiteTreeExtension
   SiteTreeFileExtension: SilverStripe\CMS\Model\SiteTreeFileExtension
@@ -45,7 +45,6 @@ mappings:
   SiteTreeLinkTracking: SilverStripe\CMS\Model\SiteTreeLinkTracking
   SiteTreeLinkTracking_Parser: SilverStripe\CMS\Model\SiteTreeLinkTracking_Parser
   VirtualPage: SilverStripe\CMS\Model\VirtualPage
-  VirtualPage_Controller: SilverStripe\CMS\Model\VirtualPage_Controller
   BrokenFilesReport: SilverStripe\CMS\Reports\BrokenFilesReport
   SideReport_BrokenFiles: SilverStripe\CMS\Reports\SideReport_BrokenFiles
   BrokenLinksReport: SilverStripe\CMS\Reports\BrokenLinksReport

code/Model/ErrorPageController.php

@@ -4,13 +4,13 @@ namespace SilverStripe\CMS\Model;
 use SilverStripe\ORM\DataModel;
 use SilverStripe\Control\HTTPRequest;
 use SilverStripe\Control\HTTPResponse;
-use Page_Controller;
+use PageController;
 
 
 /**
  * Controller for ErrorPages.
  */
-class ErrorPage_Controller extends Page_Controller
+class ErrorPageController extends PageController
 {
 
 	/**

code/Model/RedirectorPageController.php

@@ -1,12 +1,12 @@
 <?php
 namespace SilverStripe\CMS\Model;
 
-use Page_Controller;
+use PageController;
 
 /**
  * Controller for the {@link RedirectorPage}.
  */
-class RedirectorPage_Controller extends Page_Controller
+class RedirectorPageController extends PageController
 {
 
 	public function init()

code/Model/SiteTree.php

@@ -2722,7 +2722,7 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid
 	}
 
 	/**
-	 * Find the controller name by our convention of {$ModelClass}_Controller
+	 * Find the controller name by our convention of {$ModelClass}Controller
 	 *
 	 * @return string
 	 */
@@ -2738,8 +2738,17 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid
 			if ($class == SiteTree::class) {
 				break;
 			}
-			//if we have a class of "{$ClassName}_Controller" then we found our controller
+			// If we have a class of "{$ClassName}Controller" then we found our controller
-			if (class_exists($candidate = sprintf('%s_Controller', $class))) {
+			if (class_exists($candidate = sprintf('%sController', $class))) {
+				$controller = $candidate;
+				break;
+			} elseif (class_exists($candidate = sprintf('%s_Controller', $class))) {
+				// Support the legacy underscored filename, but raise a deprecation notice
+				Deprecation::notice(
+					'5.0',
+					'Underscored controller class names are deprecated. Use "MyController" instead of "My_Controller".',
+					Deprecation::SCOPE_GLOBAL
+				);
 				$controller = $candidate;
 				break;
 			}

tests/bootstrap/fixtures/Page.php.fixture

@@ -7,6 +7,14 @@ class Page extends SiteTree
 {
 }
 
-class Page_Controller extends ContentController
+class PageController extends ContentController
 {
 }
+
+/**
+ * Deprecated: Used for backwards compatibility between modules
+ */
+class Page_Controller extends ContentController
+{
+
+}

tests/controller/ContentControllerTest.php

@@ -198,7 +198,7 @@ class ContentControllerTest extends FunctionalTest {
 
 class ContentControllerTest_Page extends Page {  }
 
-class ContentControllerTest_Page_Controller extends Page_Controller {
+class ContentControllerTest_PageController extends PageController {
 
 	private static $allowed_actions = array (
 		'second_index'
@@ -218,10 +218,10 @@ class ContentControllerTest_Page_Controller extends Page_Controller {
 class ContentControllerTestPageWithoutController extends Page { }
 
 class ContentControllerTestPage extends Page { }
-class ContentControllerTestPage_Controller extends Page_Controller {
+class ContentControllerTestPageController extends PageController {
 	private static $allowed_actions = array(
-		"test",
+		'test',
-		"testwithouttemplate"
+		'testwithouttemplate'
 	);
 
 	function testwithouttemplate() {

tests/model/RedirectorPageTest.php

@@ -1,7 +1,7 @@
 <?php
 
 use SilverStripe\CMS\Model\RedirectorPage;
-use SilverStripe\CMS\Model\RedirectorPage_Controller;
+use SilverStripe\CMS\Model\RedirectorPageController;
 use SilverStripe\Control\Director;
 use SilverStripe\Dev\FunctionalTest;
 use SilverStripe\Core\Extension;
@@ -81,17 +81,17 @@ class RedirectorPageTest extends FunctionalTest {
 	}
 
 	/**
-	 * Test that we can trigger a redirection before RedirectorPage_Controller::init() is called
+	 * Test that we can trigger a redirection before RedirectorPageController::init() is called
 	 */
 	public function testRedirectRespectsFinishedResponse() {
 		$page = $this->objFromFixture('SilverStripe\\CMS\\Model\\RedirectorPage', 'goodinternal');
-		RedirectorPage_Controller::add_extension('RedirectorPageTest_RedirectExtension');
+		RedirectorPageController::add_extension('RedirectorPageTest_RedirectExtension');
 
 		$response = $this->get($page->regularLink());
 		$this->assertEquals(302, $response->getStatusCode());
 		$this->assertEquals('/foo', $response->getHeader('Location'));
 
-		RedirectorPage_Controller::remove_extension('RedirectorPageTest_RedirectExtension');
+		RedirectorPageController::remove_extension('RedirectorPageTest_RedirectExtension');
 	}
 
 }

tests/model/SiteTreeTest.php

@@ -22,8 +22,6 @@ use SilverStripe\Dev\TestOnly;
 use SilverStripe\View\Parsers\HTMLCleaner;
 use SilverStripe\View\Parsers\Diff;
 
-
-
 /**
  * @package cms
  * @subpackage tests
@@ -1334,6 +1332,25 @@ class SiteTreeTest extends SapphireTest {
 		$this->assertFalse($page->canPublish());
 	}
 
+	/**
+	 * Test that the controller name for a SiteTree instance can be gathered by appending "Controller" to the SiteTree
+	 * class name in a PSR-2 compliant manner.
+	 */
+	public function testGetControllerName()
+	{
+		$class = new Page;
+		$this->assertSame('PageController', $class->getControllerName());
+	}
+
+	/**
+	 * Test that underscored class names (legacy) are still supported (deprecation notice is issued though).
+	 */
+	public function testGetControllerNameWithUnderscoresIsSupported()
+	{
+		$class = new SiteTreeTest_LegacyControllerName;
+		$this->assertSame('SiteTreeTest_LegacyControllerName_Controller', $class->getControllerName());
+	}
+
 }
 
 /**#@+
@@ -1341,18 +1358,18 @@ class SiteTreeTest extends SapphireTest {
  */
 
 class SiteTreeTest_PageNode extends Page implements TestOnly { }
-class SiteTreeTest_PageNode_Controller extends Page_Controller implements TestOnly {
+class SiteTreeTest_PageNodeController extends PageController implements TestOnly {
 }
 
 class SiteTreeTest_Conflicted extends Page implements TestOnly { }
-class SiteTreeTest_Conflicted_Controller extends Page_Controller implements TestOnly {
+class SiteTreeTest_ConflictedController extends PageController implements TestOnly {
 
 	private static $allowed_actions = array (
 		'conflicted-action'
 	);
 
 	public function hasActionTemplate($template) {
-		if($template == 'conflicted-template') {
+		if ($template == 'conflicted-template') {
 			return true;
 		} else {
 			return parent::hasActionTemplate($template);
@@ -1468,3 +1485,16 @@ class SiteTreeTest_AdminDeniedExtension extends DataExtension implements TestOnl
 	public function canAddChildren() { return false; }
 	public function canView() { return false; }
 }
+
+/**
+ * An empty SiteTree instance with a controller to test that legacy controller names can still be loaded
+ */
+class SiteTreeTest_LegacyControllerName extends Page implements TestOnly
+{
+
+}
+
+class SiteTreeTest_LegacyControllerName_Controller extends PageController implements TestOnly
+{
+
+}

tests/model/VirtualPageTest.php

@@ -611,7 +611,7 @@ class VirtualPageTest extends FunctionalTest {
 		$virtualPage = $this->objFromFixture('SilverStripe\\CMS\\Model\\VirtualPage', 'vp4');
 		$controller = ModelAsController::controller_for($virtualPage);
 
-		$this->assertInstanceOf('VirtualPageTest_ClassA_Controller', $controller);
+		$this->assertInstanceOf('VirtualPageTest_ClassAController', $controller);
 		$this->assertTrue($controller->hasMethod('testMethod'));
 		$this->assertEquals('hello', $controller->testMethod());
 		$this->assertTrue($controller->hasMethod('modelMethod'));
@@ -641,7 +641,7 @@ class VirtualPageTest_ClassA extends Page implements TestOnly {
 	}
 }
 
-class VirtualPageTest_ClassA_Controller extends Page_Controller implements TestOnly {
+class VirtualPageTest_ClassAController extends PageController implements TestOnly {
 	private static $allowed_actions = [
 		'testaction'
 	];