FIX do not show HiddenClass pages in allowed children (#1555)

* FIX do not show HiddenClass pages in allowed children

* Resolves #1553

* * Update SiteTreeTest to include HiddenClass checks
* Refactor testAllowedChildren to use a data provider

code/model/SiteTree.php

@@ -2694,7 +2694,10 @@ class SiteTree extends DataObject implements PermissionProvider,i18nEntityProvid
 				} else {
 					$subclasses = ClassInfo::subclassesFor($candidate);
 					foreach($subclasses as $subclass) {
-						if($subclass != "SiteTree_root") $allowedChildren[] = $subclass;
+						if ($subclass == 'SiteTree_root' || singleton($subclass) instanceof HiddenClass) {
+							continue;
+						}
+						$allowedChildren[] = $subclass;
 					}
 				}
 			}

tests/model/SiteTreeTest.php

@@ -4,13 +4,13 @@
  * @subpackage tests
  */
 class SiteTreeTest extends SapphireTest {
-	
+
 	protected static $fixture_file = 'SiteTreeTest.yml';
 
 	protected $illegalExtensions = array(
 		'SiteTree' => array('SiteTreeSubsites', 'Translatable')
 	);
-	
+
 	protected $extraDataObjects = array(
 		'SiteTreeTest_ClassA',
 		'SiteTreeTest_ClassB',
@@ -27,7 +27,7 @@ class SiteTreeTest extends SapphireTest {
 	public function logOut() {
 		if($member = Member::currentUser()) $member->logOut();
 	}
-	
+
 	public function testCreateDefaultpages() {
 			$remove = SiteTree::get();
 			if($remove) foreach($remove as $page) $page->delete();
@@ -513,7 +513,7 @@ class SiteTreeTest extends SapphireTest {
 		$this->assertFalse(singleton('SiteTreeTest_ClassA')->canCreate(null, array('Parent' => $parentB)));
 		$this->assertTrue(singleton('SiteTreeTest_ClassC')->canCreate(null, array('Parent' => $parentB)));
 	}
-	
+
 	public function testEditPermissionsOnDraftVsLive() {
 		// Create an inherit-permission page
 		$page = new Page();
@@ -709,7 +709,7 @@ class SiteTreeTest extends SapphireTest {
 		$this->assertEquals($sitetree->URLSegment, 'new-page',
 			'Sets based on default title on first save'
 		);
-		
+
 		$sitetree->Title = 'Changed';
 		$sitetree->write();
 		$this->assertEquals($sitetree->URLSegment, 'changed',
@@ -889,43 +889,73 @@ class SiteTreeTest extends SapphireTest {
 		);
 	}
 
-	public function testAllowedChildren() {
+    /**
+     * Tests that core subclasses of SiteTree are included in allowedChildren() by default, but not instances of
+     * HiddenClass
+     */
+    public function testAllowedChildrenContainsCoreSubclassesButNotHiddenClass()
+    {
 		$page = new SiteTree();
+        $allowedChildren = $page->allowedChildren();
+
 		$this->assertContains(
-			'VirtualPage',
+            'VirtualPage',
-			$page->allowedChildren(),
+            $allowedChildren,
-			'Includes core subclasses by default'
+            'Includes core subclasses by default'
-		);
+        );
 
-		$classA = new SiteTreeTest_ClassA();
+        $this->assertNotContains(
-		$this->assertEquals(
+            'SiteTreeTest_ClassE',
-			array('SiteTreeTest_ClassB'),
+            $allowedChildren,
-			$classA->allowedChildren(),
+            'HiddenClass instances should not be returned'
-			'Direct setting of allowed children'
+        );
-		);
+    }
 
-		$classB = new SiteTreeTest_ClassB();
+    /**
-		$this->assertEquals(
+     * Tests that various types of SiteTree classes will or will not be returned from the allowedChildren method
-			array('SiteTreeTest_ClassC', 'SiteTreeTest_ClassCext'),
+     * @dataProvider allowedChildrenProvider
-			$classB->allowedChildren(),
+     * @param string $className
-			'Includes subclasses'
+     * @param array  $expected
-		);
+     * @param string $assertionMessage
-
+     */
-		$classD = new SiteTreeTest_ClassD();
+	public function testAllowedChildren($className, $expected, $assertionMessage)
-		$this->assertEquals(
+    {
-			array('SiteTreeTest_ClassC'),
+		$class = new $className;
-			$classD->allowedChildren(),
+		$this->assertEquals($expected, $class->allowedChildren(), $assertionMessage);
-			'Excludes subclasses if class is prefixed by an asterisk'
-		);
-
-		$classC = new SiteTreeTest_ClassC();
-		$this->assertEquals(
-			array(),
-			$classC->allowedChildren(),
-			'Null setting'
-		);
 	}
 
+    /**
+     * @return array
+     */
+    public function allowedChildrenProvider()
+    {
+        return array(
+            array(
+                // Class name
+                'SiteTreeTest_ClassA',
+                // Expected
+                array('SiteTreeTest_ClassB'),
+                // Assertion message
+                'Direct setting of allowed children'
+            ),
+            array(
+                'SiteTreeTest_ClassB',
+                array('SiteTreeTest_ClassC', 'SiteTreeTest_ClassCext'),
+                'Includes subclasses'
+            ),
+            array(
+                'SiteTreeTest_ClassC',
+                array(),
+                'Null setting'
+            ),
+            array(
+                'SiteTreeTest_ClassD',
+                array('SiteTreeTest_ClassC'),
+                'Excludes subclasses if class is prefixed by an asterisk'
+            )
+        );
+    }
+
 	public function testAllowedChildrenValidation() {
 		$page = new SiteTree();
 		$page->write();
@@ -1075,7 +1105,7 @@ class SiteTreeTest extends SapphireTest {
 		$this->assertEquals($breadcrumbs->first()->Title, "Breadcrumbs 4", "First item should be Breadrcumbs 4.");
 		$this->assertEquals($breadcrumbs->last()->Title, "Breadcrumbs 5", "Breadcrumbs 5 should be last.");
 	}
-	
+
 	/**
 	 * Tests SiteTree::MetaTags
 	 * Note that this test makes no assumption on the closing of tags (other than <title></title>)
@@ -1193,11 +1223,11 @@ class SiteTreeTest_PageNode_Controller extends Page_Controller implements TestOn
 
 class SiteTreeTest_Conflicted extends Page implements TestOnly { }
 class SiteTreeTest_Conflicted_Controller extends Page_Controller implements TestOnly {
-	
+
 	private static $allowed_actions = array (
 		'conflicted-action'
 	);
-	
+
 	public function hasActionTemplate($template) {
 		if($template == 'conflicted-template') {
 			return true;
@@ -1205,7 +1235,7 @@ class SiteTreeTest_Conflicted_Controller extends Page_Controller implements Test
 			return parent::hasActionTemplate($template);
 		}
 	}
-	
+
 }
 
 class SiteTreeTest_NullHtmlCleaner extends HTMLCleaner {
@@ -1217,7 +1247,7 @@ class SiteTreeTest_NullHtmlCleaner extends HTMLCleaner {
 class SiteTreeTest_ClassA extends Page implements TestOnly {
 
 	private static $need_permission = array('ADMIN', 'CMS_ACCESS_CMSMain');
-	
+
 	private static $allowed_children = array('SiteTreeTest_ClassB');
 }
 
@@ -1235,6 +1265,10 @@ class SiteTreeTest_ClassD extends Page implements TestOnly {
 	private static $allowed_children = array('*SiteTreeTest_ClassC');
 }
 
+class SiteTreeTest_ClassE extends Page implements TestOnly, HiddenClass {
+
+}
+
 class SiteTreeTest_ClassCext extends SiteTreeTest_ClassC implements TestOnly {
 	// Override SiteTreeTest_ClassC definitions
 	private static $allowed_children = array('SiteTreeTest_ClassB');