From 752869e23b62d39da8976415126ca90d2451b0f6 Mon Sep 17 00:00:00 2001 From: Sam Minnee Date: Wed, 13 Oct 2010 03:55:34 +0000 Subject: [PATCH] BUGFIX Renamed Nested URLs are automatically redirected to their new location with 301 HTTP status code in ModelAsController/ContentController (fixes #5393, thanks cbarberis) (from r103461) git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/trunk@112144 467b73ca-7a2a-4603-9d3b-597d59a354a9 --- core/control/ContentController.php | 29 +++- core/control/ModelAsController.php | 42 ++++-- tests/control/ModelAsControllerTest.php | 168 +++++++++++++++++++++--- 3 files changed, 211 insertions(+), 28 deletions(-) diff --git a/core/control/ContentController.php b/core/control/ContentController.php index 185b95c6c6c..f32afe0f320 100755 --- a/core/control/ContentController.php +++ b/core/control/ContentController.php @@ -134,7 +134,7 @@ public function init() { * * @return SS_HTTPResponse */ - public function handleRequest(SS_HTTPRequest $request) { + public function handleRequest(SS_HTTPRequest $request) { $child = null; $action = $request->param('Action'); @@ -144,12 +144,39 @@ public function handleRequest(SS_HTTPRequest $request) { if($action && SiteTree::nested_urls() && !$this->hasAction($action)) { // See ModelAdController->getNestedController() for similar logic Translatable::disable_locale_filter(); + // look for a page with this URLSegment $child = DataObject::get_one('SiteTree', sprintf ( "\"ParentID\" = %s AND \"URLSegment\" = '%s'", $this->ID, Convert::raw2sql($action) )); Translatable::enable_locale_filter(); + + // if we can't find a page with this URLSegment try to find one that used to have + // that URLSegment but changed. See ModelAsController->getNestedController() for similiar logic. + if(!$child){ + $child = ModelAsController::find_old_page($action,$this->ID); + if($child){ + $response = new SS_HTTPResponse(); + $params = $request->getVars(); + if(isset($params['url'])) unset($params['url']); + $response->redirect( + Controller::join_links( + $child->Link( + Controller::join_links( + $request->param('ID'), // 'ID' is the new 'URLSegment', everything shifts up one position + $request->param('OtherID') + ) + ), + // Needs to be in separate join links to avoid urlencoding + ($params) ? '?' . http_build_query($params) : null + ), + 301 + ); + return $response; + } + } } + // we found a page with this URLSegment. if($child) { $request->shiftAllParams(); $request->shift(); diff --git a/core/control/ModelAsController.php b/core/control/ModelAsController.php index 857e0b7a5ee..354eaecbd01 100755 --- a/core/control/ModelAsController.php +++ b/core/control/ModelAsController.php @@ -62,7 +62,7 @@ public function handleRequest(SS_HTTPRequest $request) { try { $result = $this->getNestedController(); - + if($result instanceof RequestHandler) { $result = $result->handleRequest($this->request); } else if(!($result instanceof SS_HTTPResponse)) { @@ -86,7 +86,7 @@ public function getNestedController() { if(!$URLSegment = $request->param('URLSegment')) { throw new Exception('ModelAsController->getNestedController(): was not passed a URLSegment value.'); } - + // Find page by link, regardless of current locale settings Translatable::disable_locale_filter(); $sitetree = DataObject::get_one( @@ -101,11 +101,26 @@ public function getNestedController() { if(!$sitetree) { // If a root page has been renamed, redirect to the new location. - if($redirect = $this->findOldPage($URLSegment)) { + // See ContentController->handleRequest() for similiar logic. + $redirect = self::find_old_page($URLSegment); + if($redirect = self::find_old_page($URLSegment)) { + $params = $request->getVars(); + if(isset($params['url'])) unset($params['url']); $this->response = new SS_HTTPResponse(); - $this->response->redirect($redirect->Link ( - Controller::join_links($request->param('Action'), $request->param('ID'), $request->param('OtherID')) - )); + $this->response->redirect( + Controller::join_links( + $redirect->Link( + Controller::join_links( + $request->param('Action'), + $request->param('ID'), + $request->param('OtherID') + ) + ), + // Needs to be in separate join links to avoid urlencoding + ($params) ? '?' . http_build_query($params) : null + ), + 301 + ); return $this->response; } @@ -128,22 +143,27 @@ public function getNestedController() { } /** - * @param string $URLSegment + * @param string $URLSegment A subset of the url. i.e in /home/contact/ home and contact are URLSegment. + * @param int $parentID The ID of the parent of the page the URLSegment belongs to. * @return SiteTree */ - protected function findOldPage($URLSegment) { + static function find_old_page($URLSegment,$parentID = 0) { $URLSegment = Convert::raw2sql($URLSegment); // First look for a non-nested page that has a unique URLSegment and can be redirected to. - if(SiteTree::nested_urls() && $pages = DataObject::get('SiteTree', "\"URLSegment\" = '$URLSegment'")) { - if($pages->Count() == 1) return $pages->First(); + if(SiteTree::nested_urls()) { + $pages = DataObject::get( + 'SiteTree', + "\"URLSegment\" = '$URLSegment'" . ((SiteTree::nested_urls()) ? ' AND "ParentID" = ' . (int)$parentID : '') + ); + if($pages && $pages->Count() == 1) return $pages->First(); } // Get an old version of a page that has been renamed. $query = new SQLQuery ( '"RecordID"', '"SiteTree_versions"', - "\"URLSegment\" = '$URLSegment' AND \"WasPublished\" = 1" . (SiteTree::nested_urls() ? ' AND "ParentID" = 0' : null), + "\"URLSegment\" = '$URLSegment' AND \"WasPublished\" = 1" . (SiteTree::nested_urls() ? ' AND "ParentID" = ' . (int)$parentID : ''), '"LastEdited" DESC', null, null, diff --git a/tests/control/ModelAsControllerTest.php b/tests/control/ModelAsControllerTest.php index 9e4bc29dbce..a26ad10cbe8 100644 --- a/tests/control/ModelAsControllerTest.php +++ b/tests/control/ModelAsControllerTest.php @@ -3,32 +3,168 @@ * @package sapphire * @subpackage tests */ -class ModelAsControllerTest extends SapphireTest { +class ModelAsControllerTest extends FunctionalTest { protected $usesDatabase = true; - public function testFindOldPage() { + protected $autoFollowRedirection = false; + + protected function generateNestedPagesFixture() { + $level1 = new Page(); + $level1->Title = 'First Level'; + $level1->URLSegment = 'level1'; + $level1->write(); + $level1->publish('Stage', 'Live'); + + $level1->URLSegment = 'newlevel1'; + $level1->write(); + $level1->publish('Stage', 'Live'); + + $level2 = new Page(); + $level2->Title = 'Second Level'; + $level2->URLSegment = 'level2'; + $level2->ParentID = $level1->ID; + $level2->write(); + $level2->publish('Stage', 'Live'); + + $level2->URLSegment = 'newlevel2'; + $level2->write(); + $level2->publish('Stage', 'Live'); + + $level3 = New Page(); + $level3->Title = "Level 3"; + $level3->URLSegment = 'level3'; + $level3->ParentID = $level2->ID; + $level3->write(); + $level3->publish('Stage','Live'); + + $level3->URLSegment = 'newlevel3'; + $level3->write(); + $level3->publish('Stage','Live'); + } + + /** + * We're building up a page hierarchy ("nested URLs") and rename + * all the individual pages afterwards. The assumption is that + * all pages will be found by their old segments. + * + * Original: level1/level2/level3 + * Republished as: newlevel1/newlevel2/newlevel3 + */ + public function testRedirectsNestedRenamedPages(){ + $this->generateNestedPagesFixture(); + + // check a first level URLSegment + $response = $this->get('level1/action'); + $this->assertEquals($response->getStatusCode(),301); + $this->assertEquals( + Controller::join_links(Director::baseURL() . 'newlevel1/action'), + $response->getHeader('Location') + ); + + // check second level URLSegment + $response = $this->get('newlevel1/level2'); + $this->assertEquals($response->getStatusCode(),301 ); + $this->assertEquals( + Controller::join_links(Director::baseURL() . 'newlevel1/newlevel2/'), + $response->getHeader('Location') + ); + + // check third level URLSegment + $response = $this->get('newlevel1/newlevel2/level3'); + $this->assertEquals($response->getStatusCode(), 301); + $this->assertEquals( + Controller::join_links(Director::baseURL() . 'newlevel1/newlevel2/newlevel3/'), + $response->getHeader('Location') + ); + } + + function testDoesntRedirectToNestedChildrenOutsideOfOwnHierarchy() { + $this->generateNestedPagesFixture(); + + $otherParent = new Page(array( + 'URLSegment' => 'otherparent' + )); + $otherParent->write(); + $otherParent->publish('Stage', 'Live'); + + $response = $this->get('level1/otherparent'); + $this->assertEquals($response->getStatusCode(), 301); + + $response = $this->get('newlevel1/otherparent'); + $this->assertEquals( + $response->getStatusCode(), + 404, + 'Requesting an unrelated page on a renamed parent should be interpreted as a missing action, not a redirect' + ); + } + + function testRedirectsNestedRenamedPagesWithGetParameters() { + $this->generateNestedPagesFixture(); + + // check third level URLSegment + $response = $this->get('newlevel1/newlevel2/level3/?foo=bar&test=test'); + $this->assertEquals($response->getStatusCode(), 301); + $this->assertEquals( + Controller::join_links(Director::baseURL() . 'newlevel1/newlevel2/newlevel3/', '?foo=bar&test=test'), + $response->getHeader('Location') + ); + } + + function testDoesntRedirectToNestedRenamedPageWhenNewExists() { + $this->generateNestedPagesFixture(); + + $otherLevel1 = new Page(array( + 'Title' => "Other Level 1", + 'URLSegment' => 'level1' + )); + $otherLevel1->write(); + $otherLevel1->publish('Stage', 'Live'); + + $response = $this->get('level1'); + $this->assertEquals( + $response->getStatusCode(), + 200 + ); + + $response = $this->get('level1/newlevel2'); + $this->assertEquals( + $response->getStatusCode(), + 404, + 'The old newlevel2/ URLSegment is checked as an action on the new page, which shouldnt exist.' + ); + } + + function testFindOldPage(){ $page = new Page(); - $page->Title = 'Test Page'; - $page->URLSegment = 'test-page'; + $page->Title = 'First Level'; + $page->URLSegment = 'oldurl'; $page->write(); $page->publish('Stage', 'Live'); - $page->URLSegment = 'test'; + $page->URLSegment = 'newurl'; $page->write(); $page->publish('Stage', 'Live'); - $router = new ModelAsController(); - $request = new SS_HTTPRequest( - 'GET', 'test-page/action/id/otherid' - ); - $request->match('$URLSegment/$Action/$ID/$OtherID'); - $response = $router->handleRequest($request); + $response = ModelAsController::find_old_page('oldurl'); + $this->assertEquals('First Level',$response->Title); - $this->assertEquals ( - $response->getHeader('Location'), - Controller::join_links(Director::baseURL() . 'test/action/id/otherid') - ); + $page2 = new Page(); + $page2->Title = 'Second Level Page'; + $page2->URLSegment = 'oldpage2'; + $page2->ParentID = $page->ID; + $page2->write(); + $page2->publish('Stage', 'Live'); + + $page2->URLSegment = 'newpage2'; + $page2->write(); + $page2->publish('Stage', 'Live'); + + $response = ModelAsController::find_old_page('oldpage2',$page2->ParentID); + $this->assertEquals('Second Level Page',$response->Title); + + $response = ModelAsController::find_old_page('oldpage2',$page2->ID); + $this->assertEquals(false, $response ); } -} +} \ No newline at end of file