BUGFIX Renamed Nested URLs are automatically redirected to their new location with 301 HTTP status code in ModelAsController/ContentController (fixes #5393, thanks cbarberis)

git-svn-id: svn://svn.silverstripe.com/silverstripe/open/modules/sapphire/branches/2.4@103461 467b73ca-7a2a-4603-9d3b-597d59a354a9

core/control/ContentController.php

@@ -144,12 +144,39 @@ class ContentController extends Controller {
 		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();

core/control/ModelAsController.php

@@ -101,11 +101,26 @@ class ModelAsController extends Controller implements NestedController {
 		
 		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 (
+				$this->response->redirect(
-					Controller::join_links($request->param('Action'), $request->param('ID'), $request->param('OtherID'))
+					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 @@ class ModelAsController extends Controller implements NestedController {
 	}
 	
 	/**
-	 * @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(SiteTree::nested_urls()) {
-			if($pages->Count() == 1) return $pages->First();
+			$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,

tests/control/ModelAsControllerTest.php

@@ -3,30 +3,166 @@
  * @package sapphire
  * @subpackage tests
  */
-class ModelAsControllerTest extends SapphireTest {
+class ModelAsControllerTest extends FunctionalTest {
 	
-	public function testFindOldPage() {
+	protected $autoFollowRedirection = false;
-		$page = new Page();
-		$page->Title      = 'Test Page';
-		$page->URLSegment = 'test-page';
-		$page->write();
-		$page->publish('Stage', 'Live');
 
-		$page->URLSegment = 'test';
+	protected function generateNestedPagesFixture() {
-		$page->write();
+		$level1 = new Page();
-		$page->publish('Stage', 'Live');
+		$level1->Title      = 'First Level';
+		$level1->URLSegment = 'level1';
+		$level1->write();
+		$level1->publish('Stage', 'Live');
 		
-		$router   = new ModelAsController();
+		$level1->URLSegment = 'newlevel1';
-		$request  = new SS_HTTPRequest(
+		$level1->write();
-			'GET', 'test-page/action/id/otherid'
+		$level1->publish('Stage', 'Live');
-		);
-		$request->match('$URLSegment/$Action/$ID/$OtherID');
-		$response = $router->handleRequest($request);
 		
+		$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(
-			$response->getHeader('Location'),
+			Controller::join_links(Director::baseURL() . 'newlevel1/action'),
-			Controller::join_links(Director::baseURL() . 'test/action/id/otherid')
+			$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      = 'First Level';
+		$page->URLSegment = 'oldurl';
+		$page->write();
+		$page->publish('Stage', 'Live');
+		
+		$page->URLSegment = 'newurl';
+		$page->write();
+		$page->publish('Stage', 'Live');
+		
+		$response = ModelAsController::find_old_page('oldurl');
+		$this->assertEquals('First Level',$response->Title);
+		
+		$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 );
+	}
+	
 }