From 907234511ee1deaf04e37ff109ba29b8c41f85aa Mon Sep 17 00:00:00 2001 From: Maxime Rainville Date: Thu, 18 Jul 2024 22:26:34 +1200 Subject: [PATCH] FIX Don't double up the breadcrumb on list views --- code/Controllers/CMSMain.php | 115 ++++++++++++++++-------- code/Controllers/CMSPagesController.php | 31 ------- tests/php/Controllers/CMSMainTest.php | 91 +++++++++++++++++-- tests/php/Controllers/CMSMainTest.yml | 4 + 4 files changed, 166 insertions(+), 75 deletions(-) diff --git a/code/Controllers/CMSMain.php b/code/Controllers/CMSMain.php index 143140880c..0a033c4a9c 100644 --- a/code/Controllers/CMSMain.php +++ b/code/Controllers/CMSMain.php @@ -1040,54 +1040,93 @@ public function getBreadcrumbsBackLink() public function Breadcrumbs($unlinked = false) { - $items = new ArrayList(); + $items = ArrayList::create(); - if ($this->TreeIsFiltered()) { - $items->push(new ArrayData([ - 'Title' => CMSPagesController::menu_title(), - 'Link' => ($unlinked) ? false : $this->LinkPages() - ])); - $items->push(new ArrayData([ - 'Title' => _t('SilverStripe\\CMS\\Controllers\\CMSMain.SEARCHRESULTS', 'Search results'), - 'Link' => ($unlinked) ? false : $this->LinkPages() - ])); - - $this->extend('updateBreadcrumbs', $items); + if (($this->getAction() !== 'index') && ($record = $this->currentPage())) { + // The page is being edited + $this->buildEditFormBreadcrumb($items, $record, $unlinked); + } else { + // Ensure we always have the "Pages" crumb first + $this->pushCrumb( + $items, + CMSPagesController::menu_title(), + $unlinked ? false : $this->LinkPages() + ); - return $items; + if ($this->TreeIsFiltered()) { + // Showing search results + $this->pushCrumb( + $items, + _t(CMSMain::class . '.SEARCHRESULTS', 'Search results'), + ($unlinked) ? false : $this->LinkPages() + ); + } elseif ($parentID = $this->getRequest()->getVar('ParentID')) { + // We're navigating the listview. ParentID is the page whose + // children are currently displayed. + if ($page = SiteTree::get()->byID($parentID)) { + $this->buildListViewBreadcrumb($items, $page); + } + } } - // Check if we are editing a page - /** @var SiteTree $record */ - $record = $this->currentPage(); - if (!$record) { - $items->push(new ArrayData([ - 'Title' => CMSPagesController::menu_title(), - 'Link' => ($unlinked) ? false : $this->LinkPages() - ])); + $this->extend('updateBreadcrumbs', $items); - $this->extend('updateBreadcrumbs', $items); + return $items; + } - return $items; - } + /** + * Push the provided an extra breadcrumb crumb at the end of the provided List + */ + private function pushCrumb(ArrayList $items, string $title, string|false $link): void + { + $items->push(ArrayData::create([ + 'Title' => $title, + 'Link' => $link + ])); + } - // Add all ancestors - $ancestors = $record->getAncestors(); - $ancestors = new ArrayList(array_reverse($ancestors->toArray() ?? [])); - $ancestors->push($record); - /** @var SiteTree $ancestor */ + /** + * Build Breadcrumb for the Edit page form. Each crumb links back to its own edit form. + */ + private function buildEditFormBreadcrumb(ArrayList $items, SiteTree $page, bool $unlinked): void + { + // Find all ancestors of the provided page + $ancestors = $page->getAncestors(true); + $ancestors = array_reverse($ancestors->toArray() ?? []); foreach ($ancestors as $ancestor) { - $items->push(new ArrayData([ - 'Title' => $ancestor->getMenuTitle(), - 'Link' => ($unlinked) - ? false - : $ancestor->CMSEditLink() - ])); + // Link to the ancestor's edit form + $this->pushCrumb( + $items, + $ancestor->getMenuTitle(), + $unlinked ? false : $ancestor->CMSEditLink() + ); } + } - $this->extend('updateBreadcrumbs', $items); - - return $items; + /** + * Build Breadcrumb for the List view. Each crumb links to the list view for that page. + */ + private function buildListViewBreadcrumb(ArrayList $items, SiteTree $page): void + { + // Find all ancestors of the provided page + $ancestors = $page->getAncestors(true); + $ancestors = array_reverse($ancestors->toArray() ?? []); + + //turns the title and link of the breadcrumbs into template-friendly variables + $params = array_filter([ + 'view' => $this->getRequest()->getVar('view'), + 'q' => $this->getRequest()->getVar('q') + ]); + + foreach ($ancestors as $ancestor) { + // Link back to the list view for the current ancestor + $params['ParentID'] = $ancestor->ID; + $this->pushCrumb( + $items, + $ancestor->getMenuTitle(), + Controller::join_links($this->Link(), '?' . http_build_query($params ?? [])) + ); + } } /** diff --git a/code/Controllers/CMSPagesController.php b/code/Controllers/CMSPagesController.php index 5fc962a977..9d30509da6 100644 --- a/code/Controllers/CMSPagesController.php +++ b/code/Controllers/CMSPagesController.php @@ -31,35 +31,4 @@ public function isCurrentPage(DataObject $record) { return false; } - - public function Breadcrumbs($unlinked = false) - { - $this->beforeExtending('updateBreadcrumbs', function (ArrayList $items) { - //special case for building the breadcrumbs when calling the listchildren Pages ListView action - if ($parentID = $this->getRequest()->getVar('ParentID')) { - $page = SiteTree::get()->byID($parentID); - - //build a reversed list of the parent tree - $pages = []; - while ($page) { - array_unshift($pages, $page); //add to start of array so that array is in reverse order - $page = $page->Parent; - } - - //turns the title and link of the breadcrumbs into template-friendly variables - $params = array_filter([ - 'view' => $this->getRequest()->getVar('view'), - 'q' => $this->getRequest()->getVar('q') - ]); - foreach ($pages as $page) { - $params['ParentID'] = $page->ID; - $item = new stdClass(); - $item->Title = $page->Title; - $item->Link = Controller::join_links($this->Link(), '?' . http_build_query($params ?? [])); - $items->push(new ArrayData($item)); - } - } - }); - return parent::Breadcrumbs($unlinked); - } } diff --git a/tests/php/Controllers/CMSMainTest.php b/tests/php/Controllers/CMSMainTest.php index 0722db4e88..6c67b6fa18 100644 --- a/tests/php/Controllers/CMSMainTest.php +++ b/tests/php/Controllers/CMSMainTest.php @@ -357,20 +357,99 @@ public function testCreationOfRestrictedPage() public function testBreadcrumbs() { - $page3 = $this->objFromFixture(SiteTree::class, 'page3'); $page31 = $this->objFromFixture(SiteTree::class, 'page31'); $this->logInAs('admin'); $response = $this->get('admin/pages/edit/show/' . $page31->ID); + $parser = new CSSContentParser($response->getBody()); + $this->assertCrumbs( + ['Page 3', 'Page 3.1'], + $response, + 'Edit breadcrumb includes all pages up to the one being edited without a tob level Page' + ); + } + + public function testBreadcrumbsListView() + { + $page311 = $this->objFromFixture(SiteTree::class, 'page311'); + $this->logInAs('admin'); + + $response = $this->get('admin/pages?ParentID=' . $page311->ID); + $this->assertCrumbs( + ['Pages', 'Page 3', 'Page 3.1', 'Page 3.1.1'], + $response, + 'List view breadcrumb includes all pages and a Page link back to the root level' + ); + } + + public function testBreadcrumbsListViewTopLevel() + { + $page311 = $this->objFromFixture(SiteTree::class, 'page311'); + $this->logInAs('admin'); + + $response = $this->get('admin/pages'); + $this->assertCrumbs( + ['Pages'], + $response, + 'Top level of list view includes only a Page crumb' + ); + } + + public function testBreadcrumbsListViewWithPjax() + { + $page311 = $this->objFromFixture(SiteTree::class, 'page311'); + $this->logInAs('admin'); + + $response = $this->get('admin/pages?ParentID=' . $page311->ID); + $this->assertCrumbs( + ['Pages', 'Page 3', 'Page 3.1', 'Page 3.1.1'], + $response, + 'List view breadcrumb includes all pages and a Page link back to the root level' + ); + } + + public function testBreadcrumbsSearchView() + { + $page311 = $this->objFromFixture(SiteTree::class, 'page311'); + $this->logInAs('admin'); + + $response = $this->get( + 'admin/pages?ParentID=' . $page311->ID, + null, + [ + 'X-Pjax' => 'ListViewForm,Breadcrumbs', + 'X-Requested-With' => 'XMLHttpRequest' + ] + ); + $jsonStr = $response->getBody(); + $data = json_decode($jsonStr, true); + + $parser = new CSSContentParser($data['Breadcrumbs']); + $crumbs = $parser->getBySelector('.breadcrumbs-wrapper .crumb'); + + $crumbs = array_map(function ($crumb) { + return (string)$crumb; + }, $crumbs); + + $this->assertNotNull($crumbs, 'Should have found some crumbs'); + $this->assertEquals( + ['Pages', 'Page 3', 'Page 3.1', 'Page 3.1.1'], + $crumbs, + 'List view breadcrumb includes all pages and a Page link back to the root level when access wia PJAX' + ); + } + + private function assertCrumbs(array $expectedCrumbs, $response, string $message): void + { $parser = new CSSContentParser($response->getBody()); $crumbs = $parser->getBySelector('.breadcrumbs-wrapper .crumb'); - $this->assertNotNull($crumbs); - $this->assertEquals(2, count($crumbs ?? [])); - $this->assertEquals('Page 3', (string)$crumbs[0]); - $this->assertEquals('Page 3.1', (string)$crumbs[1]); + $crumbs = array_map(function ($crumb) { + return (string)$crumb; + }, $crumbs); - Security::setCurrentUser(null); + $this->assertNotNull($crumbs, $message); + $this->assertEquals($expectedCrumbs, $crumbs, $message); } public function testGetNewItem() diff --git a/tests/php/Controllers/CMSMainTest.yml b/tests/php/Controllers/CMSMainTest.yml index cef69d84c9..891172ea84 100644 --- a/tests/php/Controllers/CMSMainTest.yml +++ b/tests/php/Controllers/CMSMainTest.yml @@ -12,6 +12,10 @@ SilverStripe\CMS\Model\SiteTree: Title: Page 3.1 Parent: =>SilverStripe\CMS\Model\SiteTree.page3 Sort: 1 + page311: + Title: Page 3.1.1 + Parent: =>SilverStripe\CMS\Model\SiteTree.page31 + Sort: 1 page32: Title: Page 3.2 Parent: =>SilverStripe\CMS\Model\SiteTree.page3