FIX Unpublising parent pages should include child pages

phpunit.xml.dist

@@ -1,13 +1,17 @@
-<phpunit bootstrap="vendor/silverstripe/framework/tests/bootstrap.php" colors="true">
-    <testsuite name="Default">
-        <directory>tests/</directory>
-    </testsuite>
-    <filter>
-        <whitelist processUncoveredFilesFromWhitelist="true">
-            <directory suffix=".php">src/</directory>
-            <exclude>
-                <directory suffix=".php">tests/</directory>
-            </exclude>
-        </whitelist>
-    </filter>
+<?xml version="1.0"?>
+<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" bootstrap="vendor/silverstripe/framework/tests/bootstrap.php" colors="true" xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd">
+  <coverage processUncoveredFiles="true">
+    <include>
+      <directory suffix=".php">src/</directory>
+    </include>
+    <exclude>
+      <directory suffix=".php">tests/</directory>
+    </exclude>
+  </coverage>
+  <testsuite name="Default">
+    <directory>tests/</directory>
+  </testsuite>
+  <php>
+    <get name="flush" value="1"/>
+  </php>
 </phpunit>

src/DataObject/DataObjectBatchProcessor.php

@@ -31,6 +31,8 @@ public function removeDocuments(array $documents): array
         $this->run($job);
 
         foreach ($documents as $doc) {
+            // Indexer::METHOD_ADD as default parameter make sure we check first its related documents
+            // and decide whether we should delete or update them automatically.
             $childJob = RemoveDataObjectJob::create($doc, $timestamp);
             $this->run($childJob);
         }

src/DataObject/DataObjectDocument.php

@@ -5,6 +5,7 @@
 use Exception;
 use InvalidArgumentException;
 use LogicException;
+use SilverStripe\CMS\Model\SiteTree;
 use SilverStripe\Core\Config\Configurable;
 use SilverStripe\Core\Extensible;
 use SilverStripe\Core\Injector\Injectable;
@@ -371,6 +372,10 @@ public function getFieldValue(Field $field): mixed
     }
 
     /**
+     * Collects related documents based on the underlying relationship of their DataObjects.
+     * These data are typically joined by $has_many or $many_many configured on searcheable classes
+     * defined by their fields to be indexed. Other relationships are considered a rare case.
+     *
      * @return DocumentInterface[]
      */
     public function getDependentDocuments(): array
@@ -455,6 +460,10 @@ public function getDependentDocuments(): array
             }
         }
 
+        if ($ownedDataObject instanceof SiteTree && SiteTree::config()->get('enforce_strict_hierarchy')) {
+            $docs = array_merge($docs, $this->getChildDocuments($ownedDataObject));
+        }
+
         $dependentDocs = array_values($docs);
         $this->getDataObject()->invokeWithExtensions('updateSearchDependentDocuments', $dependentDocs);
 
@@ -636,4 +645,17 @@ public function onRemoveFromSearchIndexes(string $event): void
         }
     }
 
+    public function getChildDocuments(SiteTree $page): array
+    {
+        $docs = [];
+
+        foreach ($page->AllChildren() as $record) {
+            $document = DataObjectDocument::create($record);
+            $docs[$document->getIdentifier()] = $document;
+            $docs = array_merge($docs, $document->getDependentDocuments());
+        }
+
+        return $docs;
+    }
+
 }

src/Jobs/RemoveDataObjectJob.php

@@ -11,6 +11,10 @@
 use SilverStripe\Versioned\Versioned;
 
 /**
+ *  By virture of the default parameter, Index::METHOD_ADD, this does not remove the documents straight away.
+ *  It checks first the status of the underlaying DataObjects and decides whether to remove or add them to the index.
+ *  Then pass on to the parent's process() method to handle the job.
+ *
  * @property DataObjectDocument|null $document
  * @property int|null $timestamp
  */
@@ -19,6 +23,8 @@ class RemoveDataObjectJob extends IndexJob
 
     public function __construct(?DataObjectDocument $document = null, ?int $timestamp = null, ?int $batchSize = null)
     {
+        // Indexer::METHOD_ADD as default parameter make sure we check first its related documents
+        // whether we should delete or update them automatically.
         parent::__construct([], Indexer::METHOD_ADD, $batchSize);
 
         if ($document !== null) {
@@ -46,26 +52,46 @@ public function getTitle(): string
      */
     public function setup(): void
     {
-        // Set the documents in setup to ensure async
+        /** @var DBDatetime $datetime - set the documents in setup to ensure async */
         $datetime = DBField::create_field('Datetime', $this->getTimestamp());
         $archiveDate = $datetime->format($datetime->getISOFormat());
         $documents = Versioned::withVersionedMode(function () use ($archiveDate) {
             Versioned::reading_archived_date($archiveDate);
 
+            $currentDocument = $this->getDocument();
             // Go back in time to find out what the owners were before unpublish
-            $dependentDocs = $this->document->getDependentDocuments();
+            $dependentDocs = $currentDocument->getDependentDocuments();
 
             // refetch everything on the live stage
             Versioned::set_stage(Versioned::LIVE);
 
-            return array_map(function (DataObjectDocument $doc) {
-                return DataObjectDocument::create(
-                    DataObject::get_by_id(
-                        $doc->getSourceClass(),
-                        $doc->getDataObject()->ID
-                    )
-                );
-            }, $dependentDocs);
+            return array_reduce(
+                $dependentDocs,
+                function (array $carry, DataObjectDocument $doc) {
+                    $record = DataObject::get_by_id($doc->getSourceClass(), $doc->getDataObject()->ID);
+
+                    // Since SiteTree::onBeforeDelete recursively deletes the child pages,
+                    // they end up not found on a live environment which breaks DataObjectDocument::_constructor
+                    if ($record) {
+                        $document = DataObjectDocument::create($record);
+                        $carry[$document->getIdentifier()] = $document;
+
+                        return $carry;
+                    }
+
+                    // Taking into account that this queued job has a reference of existing child pages
+                    // We need to make sure that we are able to send these pages to ElasticSearch etc. for removal
+                    $oldRecord = $doc->getDataObject();
+
+                    if ($oldRecord->isArchived() || $oldRecord->isOnDraft()) {
+                        $document = DataObjectDocument::create($oldRecord);
+                        $carry[$document->getIdentifier()] = $document;
+                    }
+
+                    return $carry;
+                },
+                []
+            );
         });
 
         $this->setDocuments($documents);

src/Service/Indexer.php

@@ -124,6 +124,8 @@ function (DocumentInterface $dependentDocument) use ($document) {
                 );
 
                 if ($dependentDocs) {
+                    // Indexer::METHOD_ADD as default parameter make sure we check first its related documents
+                    // and decide whether we should delete or update them automatically.
                     $child = Indexer::create($dependentDocs, self::METHOD_ADD, $this->getBatchSize());
 
                     while (!$child->finished()) {

tests/DataObject/DataObjectDocumentTest.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\SearchService\Tests\DataObject;
 
 use Page;
+use SilverStripe\ORM\ArrayList;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\ORM\FieldType\DBDatetime;
 use SilverStripe\ORM\RelationList;
@@ -16,6 +17,7 @@
 use SilverStripe\SearchService\Tests\Fake\DataObjectFakeVersioned;
 use SilverStripe\SearchService\Tests\Fake\DataObjectSubclassFake;
 use SilverStripe\SearchService\Tests\Fake\ImageFake;
+use SilverStripe\SearchService\Tests\Fake\PageFake;
 use SilverStripe\SearchService\Tests\Fake\TagFake;
 use SilverStripe\SearchService\Tests\SearchServiceTest;
 use SilverStripe\Security\Member;
@@ -44,6 +46,7 @@ class DataObjectDocumentTest extends SearchServiceTest
         DataObjectSubclassFake::class,
         DataObjectFakeVersioned::class,
         DataObjectFakePrivate::class,
+        PageFake::class,
     ];
 
     public function testGetIdentifier(): void
@@ -531,6 +534,7 @@ public function testGetDependentDocuments(): void
         $config->set('getSearchableClasses', [
             DataObjectFake::class,
             ImageFake::class,
+            Page::class,
         ]);
         $config->set('getFieldsForClass', [
             DataObjectFake::class => [
@@ -543,6 +547,10 @@ public function testGetDependentDocuments(): void
             ImageFake::class => [
                 new Field('tagtitles', 'Tags.Title'),
             ],
+            Page::class => [
+                new Field('title'),
+                new Field('content'),
+            ],
         ]);
 
         $dataobject = $this->objFromFixture(TagFake::class, 'one');
@@ -576,6 +584,33 @@ public function testGetDependentDocuments(): void
         }
 
         $this->assertEqualsCanonicalizing($expectedDocuments, $resultDocuments);
+
+        $pageOne = $this->objFromFixture(Page::class, 'page1');
+        $pageDoc = DataObjectDocument::create($pageOne);
+        /** @var DataObjectDocument[] $dependentPages */
+        $dependentPages = $pageDoc->getDependentDocuments();
+
+        // Grab all the expected pages
+        $pageTwo = $this->objFromFixture(Page::class, 'page2');
+        $pageThree = $this->objFromFixture(Page::class, 'page3');
+        $pageSeven = $this->objFromFixture(Page::class, 'page7');
+        $pageEight = $this->objFromFixture(Page::class, 'page8');
+
+        $expectedPages = [
+            sprintf('%s-%s', Page::class, $pageTwo->ID),
+            sprintf('%s-%s', Page::class, $pageThree->ID),
+            sprintf('%s-%s', Page::class, $pageSeven->ID),
+            sprintf('%s-%s', Page::class, $pageEight->ID),
+        ];
+
+        $resultPages = [];
+
+        // Now let's check that each Document represents the Pages that we expected
+        foreach ($dependentPages as $document) {
+            $resultPages[] = sprintf('%s-%s', $document->getSourceClass(), $document->getDataObject()?->ID);
+        }
+
+        $this->assertEqualsCanonicalizing($expectedPages, $resultPages);
     }
 
     public function testExtensionRequired(): void
@@ -626,4 +661,28 @@ public function testDeletedDataObject(): void
         unserialize(serialize($doc));
     }
 
+    public function testGetChildDocuments(): void
+    {
+        $pageOne = $this->objFromFixture(Page::class, 'page1');
+        $pageTwo = $this->objFromFixture(Page::class, 'page2');
+        $pageThree = $this->objFromFixture(Page::class, 'page3');
+        $pageSeven = $this->objFromFixture(Page::class, 'page7');
+        $pageEight = $this->objFromFixture(Page::class, 'page8');
+
+        $parentDocument = DataObjectDocument::create($pageOne);
+        $identifierPrefix = preg_replace('/\d$/', '', $parentDocument->getIdentifier());
+        $childDocs = $parentDocument->getChildDocuments($pageOne);
+
+        $expectedIdentifiers = [
+            $identifierPrefix . $pageTwo->ID,
+            $identifierPrefix . $pageThree->ID,
+            $identifierPrefix . $pageSeven->ID,
+            $identifierPrefix . $pageEight->ID,
+        ];
+
+        $resultIdentifiers = ArrayList::create($childDocs)->column('getIdentifier');
+
+        $this->assertEqualsCanonicalizing($expectedIdentifiers, $resultIdentifiers);
+    }
+
 }

tests/Fake/ImageFake.php

@@ -10,11 +10,13 @@ class ImageFake extends DataObject implements TestOnly
 {
 
     private static array $db = [
+        'Title' => 'Varchar',
         'URL' => 'Varchar',
     ];
 
     private static array $has_one = [
         'Parent' => DataObjectFake::class,
+        'PageFake' => PageFake::class,
     ];
 
     private static array $many_many = [
@@ -27,4 +29,10 @@ class ImageFake extends DataObject implements TestOnly
 
     private static string $table_name = 'ImageFake';
 
+    // For DataObjects that are not versioned, canView is needed and must return true
+    public function canView($member = null) // phpcs:ignore SlevomatCodingStandard.TypeHints
+    {
+        return true;
+    }
+
 }

tests/Fake/PageFake.php

@@ -0,0 +1,31 @@
+<?php
+
+namespace SilverStripe\SearchService\Tests\Fake;
+
+use Page;
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\SearchService\Extensions\SearchServiceExtension;
+
+class PageFake extends Page implements TestOnly
+{
+
+    private static array $many_many = [
+        'Tags' => TagFake::class,
+    ];
+
+    private static array $has_many = [
+        'Images' => ImageFake::class,
+    ];
+
+    private static array $owns = [
+        'Tags',
+        'Images',
+    ];
+
+    private static string $table_name = 'PageFake';
+
+    private static array $extensions = [
+        SearchServiceExtension::class,
+    ];
+
+}