ENHANCEMENT: Updated the SiteTree URLSegment conflict resolver to work with nested URLs.

MINOR: Added tests for SiteTree->validURLSegment().

From: Andrew Short <andrewjshort@gmail.com>

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

core/model/SiteTree.php

@@ -1288,17 +1288,11 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid
 		
 		DataObject::set_context_obj($this);
 		
-		// Ensure URLSegment is unique
-		$count = 1;
-		$otherpage = false;
-		while((class_exists($this->URLSegment) && is_subclass_of($this->URLSegment, 'RequestHandler')) || $otherpage = SiteTree::get_by_link($this->URLSegment)) {
-			if($otherpage && $otherpage->ID == $this->ID) {
-				break;
-			}
-			
-			$otherpage = false;
+		// Ensure that this object has a non-conflicting URLSegment value.
+		$count = 2;
+		while(!$this->validURLSegment()) {
+			$this->URLSegment = preg_replace('/-[0-9]+$/', null, $this->URLSegment) . '-' . $count;
 			$count++;
-			$this->URLSegment = ereg_replace('-[0-9]+$','', $this->URLSegment) . "-$count";
 		}
 		
 		DataObject::set_context_obj(null);
@@ -1339,6 +1333,56 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid
 		parent::onAfterDelete();
 	}
 	
+	/**
+	 * Returns TRUE if this object has a URLSegment value that does not conflict with any other objects. This methods
+	 * checks for:
+	 *   - A page with the same URLSegment that has a conflict.
+	 *   - Conflicts with actions on the parent page.
+	 *   - A conflict caused by a root page having the same URLSegment as a class name.
+	 *   - Conflicts with action-specific templates on the parent page.
+	 *
+	 * @return bool
+	 */
+	public function validURLSegment() {
+		if(self::nested_urls() && $parent = $this->Parent()) {
+			if($this->URLSegment == 'index') return false;
+			
+			if($controller = ModelAsController::controller_for($parent)) {
+				$actions = Object::combined_static($controller->class, 'allowed_actions', 'RequestHandler');
+				
+				// check for a conflict with an entry in $allowed_actions
+				if(is_array($actions)) {
+					if(array_key_exists($this->URLSegment, $actions) || in_array($this->URLSegment, $actions)) {
+						return false;
+					}
+				}
+				
+				// check for a conflict with an action-specific template
+				if($controller->hasMethod('hasActionTemplate') && $controller->hasActionTemplate($this->URLSegment)) {
+					return false;
+				}
+			}
+		}
+		
+		if(!self::nested_urls() || !$this->ParentID) {
+			if(class_exists($this->URLSegment) && is_subclass_of($this->URLSegment, 'RequestHandler')) return false;
+		}
+		
+		$IDFilter     = ($this->ID) ? "AND \"SiteTree\".\"ID\" <> $this->ID" :  null;
+		$parentFilter = null;
+		
+		if(self::nested_urls()) {
+			if($this->ParentID) {
+				$parentFilter = " AND \"SiteTree\".\"ParentID\" = $this->ParentID";
+			} else {
+				$parentFilter = ' AND "SiteTree"."ParentID" = 0';
+			}
+		}
+		
+		return DB::query (
+			"SELECT COUNT(ID) FROM \"SiteTree\" WHERE \"URLSegment\" = '$this->URLSegment' $IDFilter $parentFilter"
+		)->value() < 1;
+	}
 	
 	/**
 	 * Generate a URL segment based on the title provided.

tests/SiteTreeTest.php

@@ -476,11 +476,94 @@ class SiteTreeTest extends SapphireTest {
 		$this->assertTrue($ceo->isSection());
 	}
 	
+	/**
+	 * @covers SiteTree::validURLSegment()
+	 */
+	public function testValidURLSegmentURLSegmentConflicts() {
+		$sitetree = new SiteTree();
+		SiteTree::disable_nested_urls();
+		
+		$sitetree->URLSegment = 'home';
+		$this->assertFalse($sitetree->validURLSegment(), 'URLSegment conflicts are recognised');
+		$sitetree->URLSegment = 'home-noconflict';
+		$this->assertTrue($sitetree->validURLSegment());
+		
+		$sitetree->ParentID   = $this->idFromFixture('Page', 'about');
+		$sitetree->URLSegment = 'home';
+		$this->assertFalse($sitetree->validURLSegment(), 'Conflicts are still recognised with a ParentID value');
+		
+		SiteTree::enable_nested_urls();
+		
+		$sitetree->ParentID   = 0;
+		$sitetree->URLSegment = 'home';
+		$this->assertFalse($sitetree->validURLSegment(), 'URLSegment conflicts are recognised');
+		
+		$sitetree->ParentID = $this->idFromFixture('Page', 'about');
+		$this->assertTrue($sitetree->validURLSegment(), 'URLSegments can be the same across levels');
+		
+		$sitetree->URLSegment = 'my-staff';
+		$this->assertFalse($sitetree->validURLSegment(), 'Nested URLSegment conflicts are recognised');
+		$sitetree->URLSegment = 'my-staff-noconflict';
+		$this->assertTrue($sitetree->validURLSegment());
 	}
 	
-// We make these extend page since that's what all page types are expected to do
+	/**
+	 * @covers SiteTree::validURLSegment()
+	 */
+	public function testValidURLSegmentClassNameConflicts() {
+		$sitetree = new SiteTree();
+		$sitetree->URLSegment = 'Controller';
+		
+		$this->assertFalse($sitetree->validURLSegment(), 'Class name conflicts are recognised');
+	}
+	
+	/**
+	 * @covers SiteTree::validURLSegment()
+	 */
+	public function testValidURLSegmentControllerConflicts() {
+		SiteTree::enable_nested_urls();
+		
+		$sitetree = new SiteTree();
+		$sitetree->ParentID = $this->idFromFixture('SiteTreeTest_Conflicted', 'parent');
+		
+		$sitetree->URLSegment = 'index';
+		$this->assertFalse($sitetree->validURLSegment(), 'index is not a valid URLSegment');
+		
+		$sitetree->URLSegment = 'conflicted-action';
+		$this->assertFalse($sitetree->validURLSegment(), 'allowed_actions conflicts are recognised');
+		
+		$sitetree->URLSegment = 'conflicted-template';
+		$this->assertFalse($sitetree->validURLSegment(), 'Action-specific template conflicts are recognised');
+		
+		$sitetree->URLSegment = 'valid';
+		$this->assertTrue($sitetree->validURLSegment(), 'Valid URLSegment values are allowed');
+	}
+	
+}
+
+/**#@+
+ * @ignore
+ */
+
 class SiteTreeTest_PageNode extends Page implements TestOnly { }
 class SiteTreeTest_PageNode_Controller extends Page_Controller implements TestOnly {
 }
 
-?>
+class SiteTreeTest_Conflicted extends Page implements TestOnly { }
+class SiteTreeTest_Conflicted_Controller extends Page_Controller implements TestOnly {
+	
+	public static $allowed_actions = array (
+		'conflicted-action'
+	);
+	
+	public function hasActionTemplate($template) {
+		if($template == 'conflicted-template') {
+			return true;
+		} else {
+			return parent::hasActionTemplate($template);
+		}
+	}
+	 
+}
+
+/**#@-*/

tests/SiteTreeTest.yml

@@ -72,6 +72,10 @@ Page:
     numericonly:
         Title: 1930
 
+SiteTreeTest_Conflicted:
+    parent:
+        Title: Parent
+
 ErrorPage:
     404:
         Title: Page not Found