ENH Don't use filterByCallback() in CMSSiteTreeFilter classes (#3057)

silverstripe · Feb 20, 2025 · 1956792 · 1956792
1 parent f9a910d
commit 1956792
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 26 deletions.
diff --git a/code/Controllers/CMSSiteTreeFilter_StatusDeletedPages.php b/code/Controllers/CMSSiteTreeFilter_StatusDeletedPages.php
@@ -35,13 +35,7 @@ public static function title()
      */
     public function getFilteredPages()
     {
-        $pages = Versioned::get_including_deleted(SiteTree::class);
-        $pages = $this->applyDefaultFilters($pages);
-
-        $pages = $pages->filterByCallback(function (SiteTree $page) {
-            // Doesn't exist on either stage or live
-            return $page->isArchived();
-        });
-        return $pages;
+        $pages = Versioned::getArchivedOnly(SiteTree::class);
+        return $this->applyDefaultFilters($pages);
     }
 }
diff --git a/code/Controllers/CMSSiteTreeFilter_StatusDraftPages.php b/code/Controllers/CMSSiteTreeFilter_StatusDraftPages.php
@@ -25,12 +25,14 @@ public static function title()
      */
     public function getFilteredPages()
     {
-        $pages = Versioned::get_by_stage(SiteTree::class, 'Stage');
-        $pages = $this->applyDefaultFilters($pages);
-        $pages = $pages->filterByCallback(function (SiteTree $page) {
-            // If page exists on stage but not on live
-            return $page->isOnDraftOnly();
-        });
-        return $pages;
+        $pages = SiteTree::get();
+        // Get all pages existing in draft but not live
+        // Don't just use withVersionedMode - that would just get the latest draft versions
+        // including records which have since been published.
+        $pages = $pages->setDataQueryParam([
+            'Versioned.mode' => 'stage_unique',
+            'Versioned.stage' => Versioned::DRAFT,
+        ]);
+        return $this->applyDefaultFilters($pages);
     }
 }
diff --git a/code/Controllers/CMSSiteTreeFilter_StatusRemovedFromDraftPages.php b/code/Controllers/CMSSiteTreeFilter_StatusRemovedFromDraftPages.php
@@ -24,12 +24,14 @@ public static function title()
      */
     public function getFilteredPages()
     {
-        $pages = Versioned::get_including_deleted(SiteTree::class);
-        $pages = $this->applyDefaultFilters($pages);
-        $pages = $pages->filterByCallback(function (SiteTree $page) {
-            // If page is removed from stage but not live
-            return $page->isOnLiveOnly();
-        });
-        return $pages;
+        $pages = SiteTree::get();
+        // Get all pages removed from stage but not live
+        // Don't just use withVersionedMode - that would just get the latest live versions
+        // including records which were not removed from draft.
+        $pages = $pages->setDataQueryParam([
+            'Versioned.mode' => 'stage_unique',
+            'Versioned.stage' => Versioned::LIVE,
+        ]);
+        return $this->applyDefaultFilters($pages);
     }
 }
diff --git a/tests/php/Controllers/CMSSiteTreeFilterTest.php b/tests/php/Controllers/CMSSiteTreeFilterTest.php
@@ -152,12 +152,12 @@ public function testStatusDraftPagesFilter()
 
         // Check filter respects parameters
         $f = new CMSSiteTreeFilter_StatusDraftPages(['Term' => 'No Match']);
-        $this->assertEmpty($f->isRecordIncluded($draftPage));
+        $this->assertFalse($f->isRecordIncluded($draftPage));
 
         // Ensures empty array returned if no data to show
         $f = new CMSSiteTreeFilter_StatusDraftPages();
         $draftPage->delete();
-        $this->assertEmpty($f->isRecordIncluded($draftPage));
+        $this->assertFalse($f->isRecordIncluded($draftPage));
     }
 
     public function testDateFromToLastSameDate()
@@ -193,12 +193,12 @@ public function testStatusRemovedFromDraftFilter()
 
         // Check filter is respected
         $f = new CMSSiteTreeFilter_StatusRemovedFromDraftPages(['LastEditedTo' => '1999-01-01 00:00']);
-        $this->assertEmpty($f->isRecordIncluded($removedDraftPage));
+        $this->assertFalse($f->isRecordIncluded($removedDraftPage));
 
         // Ensures empty array returned if no data to show
         $f = new CMSSiteTreeFilter_StatusRemovedFromDraftPages();
         $removedDraftPage->delete();
-        $this->assertEmpty($f->isRecordIncluded($removedDraftPage));
+        $this->assertFalse($f->isRecordIncluded($removedDraftPage));
     }
 
     public function testStatusDeletedFilter()