diff --git a/admin/code/LeftAndMain.php b/admin/code/LeftAndMain.php index d7ff2b56b..5f443cbc9 100644 --- a/admin/code/LeftAndMain.php +++ b/admin/code/LeftAndMain.php @@ -779,15 +779,53 @@ class LeftAndMain extends Controller implements PermissionProvider { $link = Controller::join_links($recordController->Link("show"), $child->ID); return LeftAndMain_TreeNode::create($child, $link, $controller->isCurrentPage($child))->forTemplate(); }; - $html = $obj->getChildrenAsUL( - "", - $titleFn, - singleton('CMSPagesController'), - true, - $childrenMethod, - $numChildrenMethod, - $nodeCountThreshold - ); + + // Limit the amount of nodes shown for performance reasons. + // Skip the check if we're filtering the tree, since its not clear how many children will + // match the filter criteria until they're queried (and matched up with previously marked nodes). + $nodeThresholdLeaf = Config::inst()->get('Hierarchy', 'node_threshold_leaf'); + if($nodeThresholdLeaf && !$filterFunction) { + $nodeCountCallback = function($parent, $numChildren) use($controller, $className, $nodeThresholdLeaf) { + if($className == 'SiteTree' && $parent->ID && $numChildren > $nodeThresholdLeaf) { + return sprintf( + '', + _t('LeftAndMain.TooManyPages', 'Too many pages'), + Controller::join_links( + $controller->LinkWithSearch($controller->Link()), ' + ?view=list&ParentID=' . $parent->ID + ), + _t( + 'LeftAndMain.ShowAsList', + 'show as list', + 'Show large amount of pages in list instead of tree view' + ) + ); + } + }; + } else { + $nodeCountCallback = null; + } + + // If the amount of pages exceeds the node thresholds set, use the callback + if($obj->ParentID && $nodeCountCallback) { + $html = $nodeCountCallback($obj, $obj->$numChildrenMethod()); + } + + // Otherwise return the actual tree (which might still filter leaf thresholds on children) + if(!$html) { + $html = $obj->getChildrenAsUL( + "", + $titleFn, + singleton('CMSPagesController'), + true, + $childrenMethod, + $numChildrenMethod, + $nodeCountThreshold, + $nodeCountCallback + ); + } // Wrap the root if needs be. if(!$rootID) { diff --git a/admin/css/screen.css b/admin/css/screen.css index 115b13177..954b63dc4 100644 --- a/admin/css/screen.css +++ b/admin/css/screen.css @@ -784,6 +784,9 @@ form.import-form label.left { width: 250px; } .tree-holder.jstree-apple li.Root > a .jstree-icon, .cms-tree.jstree-apple li.Root > a .jstree-icon { background-position: -56px -36px; } .tree-holder.jstree-apple li.deletedonlive .text, .cms-tree.jstree-apple li.deletedonlive .text { text-decoration: line-through; } .tree-holder.jstree-apple li.jstree-checked > a, .tree-holder.jstree-apple li.jstree-checked > a:link, .cms-tree.jstree-apple li.jstree-checked > a, .cms-tree.jstree-apple li.jstree-checked > a:link { background-color: #efe999; } +.tree-holder.jstree-apple li.readonly, .cms-tree.jstree-apple li.readonly { color: #aaaaaa; padding-left: 18px; } +.tree-holder.jstree-apple li.readonly a, .tree-holder.jstree-apple li.readonly a:link, .cms-tree.jstree-apple li.readonly a, .cms-tree.jstree-apple li.readonly a:link { margin: 0; padding: 0; } +.tree-holder.jstree-apple li.readonly .jstree-icon, .cms-tree.jstree-apple li.readonly .jstree-icon { display: none; } .tree-holder.jstree-apple a, .tree-holder.jstree-apple a:link, .cms-tree.jstree-apple a, .cms-tree.jstree-apple a:link { color: #0073c1; padding: 3px 6px 3px 3px; border: none; display: inline-block; margin-right: 5px; } .tree-holder.jstree-apple ins, .cms-tree.jstree-apple ins { background-color: transparent; background-image: url(../images/sitetree_ss_default_icons.png); } .tree-holder.jstree-apple span.badge, .cms-tree.jstree-apple span.badge { clear: both; text-transform: uppercase; display: inline-block; padding: 0px 3px; font-size: 0.75em; line-height: 1em; margin-left: 3px; margin-right: 6px; margin-top: -1px; -webkit-border-radius: 2px 2px; -moz-border-radius: 2px / 2px; border-radius: 2px / 2px; } diff --git a/admin/scss/_tree.scss b/admin/scss/_tree.scss index 8d938ca5e..6bbb8ce74 100644 --- a/admin/scss/_tree.scss +++ b/admin/scss/_tree.scss @@ -418,6 +418,19 @@ background-color: $color-cms-batchactions-menu-selected-background; } } + &.readonly { + color: $color-text-disabled; + padding-left: 18px; + + // Don't show drag icons or required spacing + a, a:link { + margin: 0; + padding: 0; + } + .jstree-icon { + display: none; + } + } } a, a:link { color: $color-text-blue-link; @@ -441,6 +454,7 @@ margin-right: 6px; margin-top: -1px; @include border-radius(2px, 2px); + &.modified, &.addedtodraft { color: #7E7470; border: 1px solid #C9B800; diff --git a/docs/en/changelogs/3.1.0.md b/docs/en/changelogs/3.1.0.md index 21b550cd4..7fc3890bf 100644 --- a/docs/en/changelogs/3.1.0.md +++ b/docs/en/changelogs/3.1.0.md @@ -443,4 +443,7 @@ you can enable those warnings and future-proof your code already. `<% if "Some...<% end_if %>` - This change was necessary in order to support inequality operators in comparisons in templates \ No newline at end of file + This change was necessary in order to support inequality operators in comparisons in templates + * Hard limit displayed pages in the CMS tree to `500`, and the number of direct children to `250`, + to avoid excessive resource usage. Configure through `Hierarchy.node_threshold_total` and ` + Hierarchy.node_threshold_leaf`. Set to `0` to show tree unrestricted. diff --git a/docs/en/reference/sitetree.md b/docs/en/reference/sitetree.md index d6e4a9922..ef1401359 100644 --- a/docs/en/reference/sitetree.md +++ b/docs/en/reference/sitetree.md @@ -74,7 +74,7 @@ Hierarchy operations (defined on `[api:Hierarchy]`: * `$page->AllHistoricalChildren()`: Return all the children this page had, including pages that were deleted from both stage & live. * `$page->AllChildrenIncludingDeleted()`: Return all children, including those that have been deleted but are still in live. -## Limiting Hierarchy +## Allowed Children, Default Child and Root-Level By default, any page type can be the child of any other page type. However, there are static properties that can be @@ -115,9 +115,20 @@ level. Note that there is no allowed_parents` control. To set this, you will need to specify the `allowed_children` of all other page types to exclude the page type in question. -## Permission Control +## Tree Limitations +SilverStripe limits the amount of initially rendered nodes in order to avoid +processing delays, usually to a couple of dozen. The value can be configured +through `[api:Hierarchy::$node_threshold_total]`. +If a website has thousands of pages, the tree UI metaphor can become an inefficient way +to manage them. The CMS has an alternative "list view" for this purpose, which allows +sorting and paging through large numbers of pages in a tabular view. + +To avoid exceeding performance constraints of both the server and browser, +SilverStripe places hard limits on the amount of rendered pages in +a specific tree leaf, typically a couple of hundred pages. +The value can be configured through `[api:Hierarchy::$node_threshold_leaf]`. ## Tree Display (Description, Icons and Badges) diff --git a/forms/TreeDropdownField.php b/forms/TreeDropdownField.php index 8c1981ff5..c793b9549 100644 --- a/forms/TreeDropdownField.php +++ b/forms/TreeDropdownField.php @@ -269,10 +269,50 @@ class TreeDropdownField extends FormField { . $this->keyField . '\" class=\"class-$child->class"' . ' . $child->markingClasses() . "\">ID\">" . $child->' . $this->labelField . ' . ""'; - if($isSubTree) { - return substr(trim($obj->getChildrenAsUL('', $eval, null, true, $this->childrenMethod)), 4, -5); + // Limit the amount of nodes shown for performance reasons. + // Skip the check if we're filtering the tree, since its not clear how many children will + // match the filter criteria until they're queried (and matched up with previously marked nodes). + $nodeThresholdLeaf = Config::inst()->get('Hierarchy', 'node_threshold_leaf'); + if($nodeThresholdLeaf && !$this->filterCallback && !$this->search) { + $className = $this->sourceObject; + $nodeCountCallback = function($parent, $numChildren) use($className, $nodeThresholdLeaf) { + if($className == 'SiteTree' && $parent->ID && $numChildren > $nodeThresholdLeaf) { + return sprintf( + '', + _t('LeftAndMain.TooManyPages', 'Too many pages') + ); + } + }; } else { - return $obj->getChildrenAsUL('class="tree"', $eval, null, true, $this->childrenMethod); + $nodeCountCallback = null; + } + + if($isSubTree) { + $html = $obj->getChildrenAsUL( + "", + $eval, + null, + true, + $this->childrenMethod, + 'numChildren', + true, // root call + null, + $nodeCountCallback + ); + return substr(trim($html), 4, -5); + } else { + $html = $obj->getChildrenAsUL( + 'class="tree"', + $eval, + null, + true, + $this->childrenMethod, + 'numChildren', + true, // root call + null, + $nodeCountCallback + ); + return $html; } } diff --git a/model/Hierarchy.php b/model/Hierarchy.php index fa863569e..d4f6c667c 100644 --- a/model/Hierarchy.php +++ b/model/Hierarchy.php @@ -15,6 +15,28 @@ class Hierarchy extends DataExtension { * @var Int */ protected $_cache_numChildren; + + /** + * @config + * @var integer The lower bounds for the amount of nodes to mark. If set, the logic will expand + * nodes until it reaches at least this number, and then stops. Root nodes will always + * show regardless of this settting. Further nodes can be lazy-loaded via ajax. + * This isn't a hard limit. Example: On a value of 10, with 20 root nodes, each having + * 30 children, the actual node count will be 50 (all root nodes plus first expanded child). + */ + private static $node_threshold_total = 50; + + /** + * @config + * @var integer Limit on the maximum children a specific node can display. + * Serves as a hard limit to avoid exceeding available server resources + * in generating the tree, and browser resources in rendering it. + * Nodes with children exceeding this value typically won't display + * any children, although this is configurable through the $nodeCountCallback + * parameter in {@link getChildrenAsUL()}. "Root" nodes will always show + * all children, regardless of this setting. + */ + private static $node_threshold_leaf = 250; public function augmentSQL(SQLQuery &$query) { } @@ -74,21 +96,29 @@ class Hierarchy extends DataExtension { * @param string $childrenMethod The name of the method used to get children from each object * @param boolean $rootCall Set to true for this first call, and then to false for calls inside the recursion. You * should not change this. - * @param int $nodeCountThreshold The lower bounds for the amount of nodes to mark. If set, the logic will expand - * nodes until it eaches at least this number, and then stops. Root nodes will always - * show regardless of this settting. Further nodes can be lazy-loaded via ajax. - * This isn't a hard limit. Example: On a value of 10, with 20 root nodes, each having - * 30 children, the actual node count will be 50 (all root nodes plus first expanded child). + * @param int $nodeCountThreshold See {@link self::$node_threshold_total} + * @param callable $nodeCountCallback Called with the node count, which gives the callback an opportunity + * to intercept the query. Useful e.g. to avoid excessive children listings (Arguments: $parent, $numChildren) * * @return string */ public function getChildrenAsUL($attributes = "", $titleEval = '"
  • " . $child->Title', $extraArg = null, $limitToMarked = false, $childrenMethod = "AllChildrenIncludingDeleted", - $numChildrenMethod = "numChildren", $rootCall = true, $nodeCountThreshold = 30) { + $numChildrenMethod = "numChildren", $rootCall = true, $nodeCountThreshold = null, $nodeCountCallback = null) { + + if(!is_numeric($nodeCountThreshold)) { + $nodeCountThreshold = Config::inst()->get('Hierarchy', 'node_threshold_total'); + } if($limitToMarked && $rootCall) { $this->markingFinished($numChildrenMethod); } + + if($nodeCountCallback) { + $nodeCountWarning = $nodeCountCallback($this->owner, $this->owner->$numChildrenMethod()); + if($nodeCountWarning) return $nodeCountWarning; + } + if($this->owner->hasMethod($childrenMethod)) { $children = $this->owner->$childrenMethod($extraArg); @@ -110,7 +140,6 @@ class Hierarchy extends DataExtension { $output .= (is_callable($titleEval)) ? $titleEval($child) : eval("return $titleEval;"); $output .= "\n"; - $numChildren = $child->$numChildrenMethod(); if( // Always traverse into opened nodes (they might be exposed as parents of search results) @@ -119,10 +148,16 @@ class Hierarchy extends DataExtension { // Otherwise, the remaining nodes are lazy loaded via ajax. && $child->isMarked() ) { - $output .= $child->getChildrenAsUL("", $titleEval, $extraArg, $limitToMarked, $childrenMethod, - $numChildrenMethod, false, $nodeCountThreshold); - } - elseif($child->isTreeOpened()) { + // Additionally check if node count requirements are met + $nodeCountWarning = $nodeCountCallback ? $nodeCountCallback($child, $numChildren) : null; + if($nodeCountWarning) { + $output .= $nodeCountWarning; + $child->markClosed(); + } else { + $output .= $child->getChildrenAsUL("", $titleEval, $extraArg, $limitToMarked, $childrenMethod, + $numChildrenMethod, false, $nodeCountThreshold); + } + } elseif($child->isTreeOpened()) { // Since we're not loading children, don't mark it as open either $child->markClosed(); } diff --git a/tests/model/HierarchyTest.php b/tests/model/HierarchyTest.php index d3c43957f..ca7e762bb 100644 --- a/tests/model/HierarchyTest.php +++ b/tests/model/HierarchyTest.php @@ -60,11 +60,11 @@ class HierarchyTest extends SapphireTest { $obj3 = Versioned::get_including_deleted("HierarchyTest_Object", "\"Title\" = 'Obj 3'")->First(); // Check that both obj 3 children are returned - $this->assertEquals(array("Obj 3a", "Obj 3b"), + $this->assertEquals(array("Obj 3a", "Obj 3b", "Obj 3c"), $obj3->AllHistoricalChildren()->column('Title')); // Check numHistoricalChildren - $this->assertEquals(2, $obj3->numHistoricalChildren()); + $this->assertEquals(3, $obj3->numHistoricalChildren()); } @@ -94,7 +94,7 @@ class HierarchyTest extends SapphireTest { public function testNumChildren() { $this->assertEquals($this->objFromFixture('HierarchyTest_Object', 'obj1')->numChildren(), 0); $this->assertEquals($this->objFromFixture('HierarchyTest_Object', 'obj2')->numChildren(), 2); - $this->assertEquals($this->objFromFixture('HierarchyTest_Object', 'obj3')->numChildren(), 2); + $this->assertEquals($this->objFromFixture('HierarchyTest_Object', 'obj3')->numChildren(), 3); $this->assertEquals($this->objFromFixture('HierarchyTest_Object', 'obj2a')->numChildren(), 2); $this->assertEquals($this->objFromFixture('HierarchyTest_Object', 'obj2b')->numChildren(), 0); $this->assertEquals($this->objFromFixture('HierarchyTest_Object', 'obj3a')->numChildren(), 2); @@ -200,29 +200,13 @@ class HierarchyTest extends SapphireTest { true, // rootCall $nodeCountThreshold ); - $parser = new CSSContentParser($html); - $node2 = $parser->getByXpath( - '//ul/li[@id="' . $obj2->ID . '"]' - ); - $this->assertTrue( - (bool)$node2, + $this->assertTreeContains($html, array($obj2), 'Contains root elements' ); - $node2a = $parser->getByXpath( - '//ul/li[@id="' . $obj2->ID . '"]' . - '/ul/li[@id="' . $obj2a->ID . '"]' - ); - $this->assertTrue( - (bool)$node2a, + $this->assertTreeContains($html, array($obj2, $obj2a), 'Contains child elements (in correct nesting)' ); - $node2aa = $parser->getByXpath( - '//ul/li[@id="' . $obj2->ID . '"]' . - '/ul/li[@id="' . $obj2a->ID . '"]' . - '/ul/li[@id="' . $obj2aa->ID . '"]' - ); - $this->assertTrue( - (bool)$node2aa, + $this->assertTreeContains($html, array($obj2, $obj2a, $obj2aa), 'Contains grandchild elements (in correct nesting)' ); } @@ -247,27 +231,13 @@ class HierarchyTest extends SapphireTest { true, $nodeCountThreshold ); - $parser = new CSSContentParser($html); - $node1 = $parser->getByXpath( - '//ul/li[@id="' . $obj1->ID . '"]' - ); - $this->assertTrue( - (bool)$node1, + $this->assertTreeContains($html, array($obj1), 'Contains root elements' ); - $node2 = $parser->getByXpath( - '//ul/li[@id="' . $obj2->ID . '"]' - ); - $this->assertTrue( - (bool)$node2, + $this->assertTreeContains($html, array($obj2), 'Contains root elements' ); - $node2a = $parser->getByXpath( - '//ul/li[@id="' . $obj2->ID . '"]' . - '/ul/li[@id="' . $obj2a->ID . '"]' - ); - $this->assertFalse( - (bool)$node2a, + $this->assertTreeNotContains($html, array($obj2, $obj2a), 'Does not contains child elements because they exceed minNodeCount' ); } @@ -296,20 +266,12 @@ class HierarchyTest extends SapphireTest { true, $nodeCountThreshold ); - $parser = new CSSContentParser($html); - $node2 = $parser->getByXpath( - '//ul/li[@id="' . $obj2->ID . '"]' - ); - $this->assertTrue( - (bool)$node2, + $this->assertTreeContains($html, array($obj2), 'Contains root elements' ); - $node2aa = $parser->getByXpath( - '//ul/li[@id="' . $obj2->ID . '"]' . - '/ul/li[@id="' . $obj2a->ID . '"]' . - '/ul/li[@id="' . $obj2aa->ID . '"]' + $this->assertTreeContains($html, array($obj2, $obj2a, $obj2aa), + 'Does contain marked children nodes regardless of configured threshold' ); - $this->assertTrue((bool)$node2aa); } public function testGetChildrenAsULMinNodeCountWithFilters() { @@ -343,21 +305,10 @@ class HierarchyTest extends SapphireTest { true, $nodeCountThreshold ); - $parser = new CSSContentParser($html); - $node1 = $parser->getByXpath( - '//ul/li[@id="' . $obj1->ID . '"]' - ); - $this->assertFalse( - (bool)$node1, + $this->assertTreeNotContains($html, array($obj1), 'Does not contain root elements which dont match the filter' ); - $node2aa = $parser->getByXpath( - '//ul/li[@id="' . $obj2->ID . '"]' . - '/ul/li[@id="' . $obj2a->ID . '"]' . - '/ul/li[@id="' . $obj2aa->ID . '"]' - ); - $this->assertTrue( - (bool)$node2aa, + $this->assertTreeContains($html, array($obj2, $obj2a, $obj2aa), 'Contains non-root elements which match the filter' ); } @@ -377,8 +328,6 @@ class HierarchyTest extends SapphireTest { $root->setMarkingFilterFunction(function($record) use($obj2, $obj2a, $obj2aa) { // Results need to include parent hierarchy, even if we just want to // match the innermost node. - // var_dump($record->Title); - // var_dump(in_array($record->ID, array($obj2->ID, $obj2a->ID, $obj2aa->ID))); return in_array($record->ID, array($obj2->ID, $obj2a->ID, $obj2aa->ID)); }); $root->markPartialTree($nodeCountThreshold); @@ -393,25 +342,83 @@ class HierarchyTest extends SapphireTest { true, $nodeCountThreshold ); - $parser = new CSSContentParser($html); - $node1 = $parser->getByXpath( - '//ul/li[@id="' . $obj1->ID . '"]' - ); - $this->assertFalse( - (bool)$node1, + $this->assertTreeNotContains($html, array($obj1), 'Does not contain root elements which dont match the filter' ); - $node2aa = $parser->getByXpath( - '//ul/li[@id="' . $obj2->ID . '"]' . - '/ul/li[@id="' . $obj2a->ID . '"]' . - '/ul/li[@id="' . $obj2aa->ID . '"]' - ); - $this->assertTrue( - (bool)$node2aa, + $this->assertTreeContains($html, array($obj2, $obj2a, $obj2aa), 'Contains non-root elements which match the filter' ); } + public function testGetChildrenAsULNodeThresholdLeaf() { + $obj1 = $this->objFromFixture('HierarchyTest_Object', 'obj1'); + $obj2 = $this->objFromFixture('HierarchyTest_Object', 'obj2'); + $obj2a = $this->objFromFixture('HierarchyTest_Object', 'obj2a'); + $obj3 = $this->objFromFixture('HierarchyTest_Object', 'obj3'); + $obj3a = $this->objFromFixture('HierarchyTest_Object', 'obj3a'); + + $nodeCountThreshold = 99; + + $root = new HierarchyTest_Object(); + $root->markPartialTree($nodeCountThreshold); + $nodeCountCallback = function($parent, $numChildren) { + // Set low enough that it the fixture structure should exceed it + if($parent->ID && $numChildren > 2) { + return 'Exceeded!'; + } + }; + + $html = $root->getChildrenAsUL( + "", + '"
  • ID . "\">" . $child->Title', + null, + true, // limit to marked + "AllChildrenIncludingDeleted", + "numChildren", + true, + $nodeCountThreshold, + $nodeCountCallback + ); + $this->assertTreeContains($html, array($obj1), + 'Does contain root elements regardless of count' + ); + $this->assertTreeContains($html, array($obj3), + 'Does contain root elements regardless of count' + ); + $this->assertTreeContains($html, array($obj2, $obj2a), + 'Contains children which do not exceed threshold' + ); + $this->assertTreeNotContains($html, array($obj3, $obj3a), + 'Does not contain children which exceed threshold' + ); + } + + /** + * @param String $html [description] + * @param array $nodes Breadcrumb path as array + * @param String $message + */ + protected function assertTreeContains($html, $nodes, $message = null) { + $parser = new CSSContentParser($html); + $xpath = '/'; + foreach($nodes as $node) $xpath .= '/ul/li[@id="' . $node->ID . '"]'; + $match = $parser->getByXpath($xpath); + self::assertThat((bool)$match, self::isTrue(), $message); + } + + /** + * @param String $html [description] + * @param array $nodes Breadcrumb path as array + * @param String $message + */ + protected function assertTreeNotContains($html, $nodes, $message = null) { + $parser = new CSSContentParser($html); + $xpath = '/'; + foreach($nodes as $node) $xpath .= '/ul/li[@id="' . $node->ID . '"]'; + $match = $parser->getByXpath($xpath); + self::assertThat((bool)$match, self::isFalse(), $message); + } + } class HierarchyTest_Object extends DataObject implements TestOnly { diff --git a/tests/model/HierarchyTest.yml b/tests/model/HierarchyTest.yml index 7dea96aa9..b3cdd8231 100644 --- a/tests/model/HierarchyTest.yml +++ b/tests/model/HierarchyTest.yml @@ -17,6 +17,9 @@ HierarchyTest_Object: obj3b: Parent: =>HierarchyTest_Object.obj3 Title: Obj 3b + obj3c: + Parent: =>HierarchyTest_Object.obj3 + Title: Obj 3c obj2aa: Parent: =>HierarchyTest_Object.obj2a Title: Obj 2aa