diff --git a/README.md b/README.md index 9f32a328..b0bdfd2f 100644 --- a/README.md +++ b/README.md @@ -20,6 +20,31 @@ It generates the cache files using the [QueuedJobs module](https://github.com/sy * "symbiote/silverstripe-queuedjobs": "^5", * "silverstripe/versioned": "^2" +## Migrating from v4 + +### `SiteTreePublishingEngine` changes + +The following methods have been removed: + +* `getToUpdate()` +* `setToUpdate()` +* `getToDelete()` +* `setToDelete()` + +These methods used to deal with `DataObjects`. We are now collecting URLs instead of `DataObjects`, and these URLs are +being stored and manipulated by the following methods. + +* `getUrlsToUpdate()` +* `addUrlsToUpdate()` +* `setUrlsToUpdate()` +* `getUrlsToDelete()` +* `addUrlsToDelete()` +* `setUrlsToDelete()` + +Why did this change happen? TL;DR: We realised that purging URLs for pages that change their URL (EG: their parent page +changes, or their `URLSegment` changes) was not functioning as expected. This was really the only way to enable this +functionality to work correctly while maintaining cache integrety. + ## Unit-testing with StaticPublisherState to disable queuedjobs for unit-tests You can use `StaticPublisherState` to disable queuejobs job queueing and logging in unit-testing to improve performance. @@ -28,15 +53,15 @@ Add the following yml to your project: ```yml ---- --Name: staticpublishqueue-tests --Only: -- classexists: -- - 'Symbiote\QueuedJobs\Tests\QueuedJobsTest\QueuedJobsTest_Handler' -- - 'SilverStripe\StaticPublishQueue\Test\QueuedJobsTestService' +Name: staticpublishqueue-tests +Only: + classexists: + - 'Symbiote\QueuedJobs\Tests\QueuedJobsTest\QueuedJobsTest_Handler' + - 'SilverStripe\StaticPublishQueue\Test\QueuedJobsTestService' ---- --SilverStripe\Core\Injector\Injector: -- SilverStripe\Dev\State\SapphireTestState: -- properties: -- States: -- staticPublisherState: '%$SilverStripe\StaticPublishQueue\Dev\StaticPublisherState' +SilverStripe\Core\Injector\Injector: + SilverStripe\Dev\State\SapphireTestState: + properties: + States: + staticPublisherState: '%$SilverStripe\StaticPublishQueue\Dev\StaticPublisherState' ``` diff --git a/src/Extension/Engine/SiteTreePublishingEngine.php b/src/Extension/Engine/SiteTreePublishingEngine.php index 23b1644e..d0154645 100644 --- a/src/Extension/Engine/SiteTreePublishingEngine.php +++ b/src/Extension/Engine/SiteTreePublishingEngine.php @@ -41,12 +41,12 @@ class SiteTreePublishingEngine extends SiteTreeExtension implements Resettable /** * Queues the urls to be flushed into the queue. */ - private array|SS_List $toUpdate = []; + private array $urlsToUpdate = []; /** * Queues the urls to be deleted as part of a next flush operation. */ - private array|SS_List $toDelete = []; + private array $urlsToDelete = []; public static function reset(): void { @@ -57,7 +57,6 @@ public static function reset(): void * Force inject queue service * Used for unit tests only to cover edge cases where Injector doesn't cover * - * * @param QueuedJobService $service */ public static function setQueueService(QueuedJobService $service): void @@ -65,42 +64,34 @@ public static function setQueueService(QueuedJobService $service): void static::$queueService = $service; } - /** - * @return array - */ - public function getToUpdate() + protected function getUrlsToUpdate(): array { - return $this->toUpdate; + return $this->urlsToUpdate; } - /** - * @return array - */ - public function getToDelete() + protected function getUrlsToDelete(): array { - return $this->toDelete; + return $this->urlsToDelete; } - /** - * @param array $toUpdate - * @return $this - */ - public function setToUpdate($toUpdate) + protected function addUrlsToUpdate(array $urlsToUpdate): void { - $this->toUpdate = $toUpdate; + $this->urlsToUpdate = array_unique(array_merge($this->getUrlsToUpdate(), $urlsToUpdate)); + } - return $this; + protected function addUrlsToDelete(array $urlsToDelete): void + { + $this->urlsToDelete = array_unique(array_merge($this->getUrlsToDelete(), $urlsToDelete)); } - /** - * @param array $toDelete - * @return $this - */ - public function setToDelete($toDelete) + protected function setUrlsToUpdate(array $urlsToUpdate): void { - $this->toDelete = $toDelete; + $this->urlsToUpdate = $urlsToUpdate; + } - return $this; + protected function setUrlsToDelete(array $urlsToDelete): void + { + $this->urlsToDelete = $urlsToDelete; } /** @@ -113,20 +104,20 @@ public function onBeforePublishRecursive($original) return; } + $owner = $this->getOwner(); + // We want to find out if the URL for this page has changed at all. That can happen 2 ways: Either the page is // moved in the SiteTree (ParentID changes), or the URLSegment is updated // Apparently ParentID can sometimes be string, so make sure we cast to (int) for our comparison - if ((int) $original->ParentID !== (int) $this->getOwner()->ParentID - || $original->URLSegment !== $this->getOwner()->URLSegment + if ((int) $original->ParentID !== (int) $owner->ParentID + || $original->URLSegment !== $owner->URLSegment ) { // We have detected a change to the URL. We need to purge the old URLs for this page and any children $context = [ 'action' => self::ACTION_UNPUBLISH, ]; + // We'll collect these changes now, but they won't be actioned until onAfterPublishRecursive() $original->collectChanges($context); - // We need to flushChanges() immediately so that Jobs are queued for the Pages while they still have their - // old URLs - $original->flushChanges(); } } @@ -138,18 +129,22 @@ public function onAfterPublishRecursive($original) $parentId = $original->ParentID ?? null; $urlSegment = $original->URLSegment ?? null; + $owner = $this->getOwner(); + // Apparently ParentID can sometimes be string, so make sure we cast to (int) for our comparison - $parentChanged = $parentId && (int) $parentId !== (int) $this->getOwner()->ParentID; - $urlChanged = $urlSegment && $original->URLSegment !== $this->getOwner()->URLSegment; + $parentChanged = $parentId && (int) $parentId !== (int) $owner->ParentID; + $urlSegmentChanged = $urlSegment && $original->URLSegment !== $owner->URLSegment; $context = [ 'action' => self::ACTION_PUBLISH, // If a URL change has been detected, then we need to force the recursive regeneration of all child // pages - 'urlChanged' => $parentChanged || $urlChanged, + 'urlSegmentChanged' => $parentChanged || $urlSegmentChanged, ]; + // Collect any additional changes (noting that some could already have been added in onBeforePublishRecursive()) $this->collectChanges($context); + // Flush any/all changes that we have detected $this->flushChanges(); } @@ -158,7 +153,13 @@ public function onBeforeUnpublish() $context = [ 'action' => self::ACTION_UNPUBLISH, ]; + // We'll collect these changes now, but they won't be actioned until onAfterUnpublish() $this->collectChanges($context); + } + + public function onAfterUnpublish() + { + // Flush any/all changes that we have detected $this->flushChanges(); } @@ -174,24 +175,32 @@ public function collectChanges(array $context): void // Collection of changes needs to happen within the context of our Published/LIVE state Versioned::set_stage(Versioned::LIVE); + $owner = $this->getOwner(); + // Re-fetch our page, now within a LIVE context - $page = DataObject::get($this->getOwner()->ClassName)->byID($this->getOwner()->ID); + $siteTree = DataObject::get($owner->ClassName)->byID($owner->ID); // This page isn't LIVE/Published, so there is nothing for us to do here - if (!$page?->exists()) { + if (!$siteTree?->exists()) { return; } // The page does not include the required extension, and it doesn't implement a Trigger - if (!$page->hasExtension(PublishableSiteTree::class) && !$page instanceof StaticPublishingTrigger) { + if (!$siteTree->hasExtension(PublishableSiteTree::class) && !$siteTree instanceof StaticPublishingTrigger) { return; } - $toUpdate = $page->objectsToUpdate($context); - $this->setToUpdate($toUpdate); + // Fetch our objects to be actioned + $objectsToUpdate = $siteTree->objectsToUpdate($context); + $objectsToDelete = $siteTree->objectsToDelete($context); - $toDelete = $page->objectsToDelete($context); - $this->setToDelete($toDelete); + foreach ($objectsToUpdate as $objectToUpdate) { + $this->addUrlsToUpdate(array_keys($objectToUpdate->urlsToCache())); + } + + foreach ($objectsToDelete as $objectToDelete) { + $this->addUrlsToDelete(array_keys($objectToDelete->urlsToCache())); + } }); } @@ -201,45 +210,36 @@ public function collectChanges(array $context): void public function flushChanges() { $queueService = static::$queueService ?? QueuedJobService::singleton(); + $owner = $this->getOwner(); + $urlsToUpdate = $this->getUrlsToUpdate(); + $urlsToDelete = $this->getUrlsToDelete(); - if ($this->toUpdate) { + if ($urlsToUpdate) { /** @var UrlBundleInterface $urlService */ $urlService = Injector::inst()->create(UrlBundleInterface::class); + $urlService->addUrls($urlsToUpdate); - foreach ($this->toUpdate as $item) { - $urls = $item->urlsToCache(); - ksort($urls); - $urls = array_keys($urls); - $urlService->addUrls($urls); - } - - $jobs = $urlService->getJobsForUrls(GenerateStaticCacheJob::class, 'Building URLs', $this->owner); + $jobs = $urlService->getJobsForUrls(GenerateStaticCacheJob::class, 'Building URLs', $owner); foreach ($jobs as $job) { $queueService->queueJob($job); } - $this->toUpdate = []; + $this->setUrlsToUpdate([]); } - if ($this->toDelete) { + if ($urlsToDelete) { /** @var UrlBundleInterface $urlService */ $urlService = Injector::inst()->create(UrlBundleInterface::class); + $urlService->addUrls($urlsToDelete); - foreach ($this->toDelete as $item) { - $urls = $item->urlsToCache(); - ksort($urls); - $urls = array_keys($urls); - $urlService->addUrls($urls); - } - - $jobs = $urlService->getJobsForUrls(DeleteStaticCacheJob::class, 'Purging URLs', $this->owner); + $jobs = $urlService->getJobsForUrls(DeleteStaticCacheJob::class, 'Purging URLs', $owner); foreach ($jobs as $job) { $queueService->queueJob($job); } - $this->toDelete = []; + $this->setUrlsToDelete([]); } } } diff --git a/src/Extension/Publishable/PublishableSiteTree.php b/src/Extension/Publishable/PublishableSiteTree.php index 44aae7ed..071f55cd 100644 --- a/src/Extension/Publishable/PublishableSiteTree.php +++ b/src/Extension/Publishable/PublishableSiteTree.php @@ -8,6 +8,7 @@ use SilverStripe\Control\Director; use SilverStripe\ORM\DataExtension; use SilverStripe\ORM\SS_List; +use SilverStripe\SiteConfig\SiteConfig; use SilverStripe\StaticPublishQueue\Contract\StaticallyPublishable; use SilverStripe\StaticPublishQueue\Contract\StaticPublishingTrigger; use SilverStripe\StaticPublishQueue\Extension\Engine\SiteTreePublishingEngine; @@ -42,15 +43,14 @@ public function getMyVirtualPages() public function objectsToUpdate(array $context) { $list = []; - $page = $this->getOwner(); - $urlChanged = $context['urlChanged'] ?? false; + $siteTree = $this->getOwner(); if ($context['action'] === SiteTreePublishingEngine::ACTION_PUBLISH) { // Trigger refresh of the page itself - $list[] = $page; + $list[] = $siteTree; // Refresh related virtual pages - $virtualPages = $page->getMyVirtualPages(); + $virtualPages = $siteTree->getMyVirtualPages(); if ($virtualPages->exists()) { foreach ($virtualPages as $virtual) { @@ -62,24 +62,25 @@ public function objectsToUpdate(array $context) // *not* include children // Note: This configuration is only relevant for the 'publish' action because for an 'unpublish' action // we specifically have to purge all children (we now that they have also been unpublished) - $childInclusion = $page->config()->get('regenerate_children'); + $childInclusion = $siteTree->config()->get('regenerate_children'); + $forceRecursiveInclusion = $context['urlSegmentChanged'] ?? false; - if ($childInclusion || $urlChanged) { - $recursive = $childInclusion === self::RELATION_INCLUDE_RECURSIVE || $urlChanged; + if ($childInclusion || $forceRecursiveInclusion) { + $recursive = $childInclusion === self::RELATION_INCLUDE_RECURSIVE || $forceRecursiveInclusion; - $this->addChildren($list, $page, $recursive); + $this->addChildren($list, $siteTree, $recursive); } } // For any of our defined actions, we will update parents when configured to do so. Config value or 0 // is to *not* include parents - $parentInclusion = $page->config()->get('regenerate_parents'); + $parentInclusion = $siteTree->config()->get('regenerate_parents'); if ($parentInclusion) { // You can also choose whether to update only the direct parent, or the entire tree $recursive = $parentInclusion === self::RELATION_INCLUDE_RECURSIVE; - $this->addParents($list, $page, $recursive); + $this->addParents($list, $siteTree, $recursive); } return $list; @@ -98,13 +99,13 @@ public function objectsToDelete(array $context) } $list = []; - $page = $this->getOwner(); + $siteTree = $this->getOwner(); // Trigger cache removal for this page - $list[] = $page; + $list[] = $siteTree; // Trigger removal of the related virtual pages - $virtualPages = $page->getMyVirtualPages(); + $virtualPages = $siteTree->getMyVirtualPages(); if ($virtualPages->exists()) { foreach ($virtualPages as $virtual) { @@ -112,9 +113,17 @@ public function objectsToDelete(array $context) } } - // When a parent is unpublished or the URL has changed, then all existing caches for all children have also - // been invalided and must be purged. In this context, this is not optional/configurable - $this->addChildren($list, $page, true); + // Check if you specifically want children included in all actions + $childInclusion = $siteTree->config()->get('regenerate_children'); + // Check to see if SiteTree enforces strict hierarchy (that being, parents must be published in order for + // children to be viewed) + $forceRecursiveInclusion = $siteTree->config()->get('enforce_strict_hierarchy'); + + if ($childInclusion || $forceRecursiveInclusion) { + $recursive = $childInclusion === self::RELATION_INCLUDE_RECURSIVE || $forceRecursiveInclusion; + + $this->addChildren($list, $siteTree, $recursive); + } return $list; } diff --git a/src/Job.php b/src/Job.php index f1e02b71..bd16fc45 100644 --- a/src/Job.php +++ b/src/Job.php @@ -7,11 +7,8 @@ use Symbiote\QueuedJobs\Services\AbstractQueuedJob; /** - * Class Job - * * @property array $URLsToProcess * @property array $ProcessedURLs - * @package SilverStripe\StaticPublishQueue */ abstract class Job extends AbstractQueuedJob { @@ -63,9 +60,6 @@ abstract class Job extends AbstractQueuedJob /** * Use this method to populate newly created job with data - * - * @param array $urls - * @param string|null $message */ public function hydrate(array $urls, ?string $message): void { @@ -88,8 +82,6 @@ public function hydrate(array $urls, ?string $message): void * as they always refer to live content * Including stage param in visiting URL is meant to bypass static cache and redirect to admin login * this is something we definitely don't want for statically cached pages - * - * @return int|null */ public function getRunAsMemberID(): ?int { @@ -125,9 +117,6 @@ public function process(): void $this->updateCompletedState(); } - /** - * @return int - */ public function getUrlsPerJob(): int { $urlsPerJob = (int) $this->config()->get('urls_per_job'); @@ -137,9 +126,6 @@ public function getUrlsPerJob(): int /** * Implement this method to process URL - * - * @param string $url - * @param int $priority */ abstract protected function processUrl(string $url, int $priority): void; @@ -148,8 +134,6 @@ abstract protected function processUrl(string $url, int $priority): void; * indication of progress is important for jobs which take long time to process * jobs that do not indicate progress may be identified as stalled by the queue * and may end up paused - * - * @param string $url */ protected function markUrlAsProcessed(string $url): void { @@ -172,9 +156,6 @@ protected function updateCompletedState(): void $this->isComplete = true; } - /** - * @return int - */ protected function getChunkSize(): int { $chunkSize = (int) $this->config()->get('chunk_size'); @@ -185,9 +166,6 @@ protected function getChunkSize(): int /** * This function can be overridden to handle the case of failure of specific URL processing * such case is not handled by default which results in all such errors being effectively silenced - * - * @param string $url - * @param array $meta */ protected function handleFailedUrl(string $url, array $meta) { diff --git a/src/Job/DeleteStaticCacheJob.php b/src/Job/DeleteStaticCacheJob.php index ee59febc..d82dd0d1 100644 --- a/src/Job/DeleteStaticCacheJob.php +++ b/src/Job/DeleteStaticCacheJob.php @@ -6,31 +6,17 @@ use SilverStripe\StaticPublishQueue\Publisher; /** - * Class DeleteStaticCacheJob - * remove pages from static cache based on list of URLs - * - * @package SilverStripe\StaticPublishQueue\Job + * Remove pages from static cache based on list of URLs */ class DeleteStaticCacheJob extends Job { - /** - * @var int - * @config - */ - private static $chunk_size = 2000; - - /** - * @return string - */ + private static int $chunk_size = 2000; + public function getTitle(): string { return 'Remove a set of static pages from the cache'; } - /** - * @param string $url - * @param int $priority - */ protected function processUrl(string $url, int $priority): void { $meta = Publisher::singleton()->purgeURL($url); diff --git a/src/Job/DeleteWholeCache.php b/src/Job/DeleteWholeCache.php index 27788018..94b02897 100644 --- a/src/Job/DeleteWholeCache.php +++ b/src/Job/DeleteWholeCache.php @@ -7,9 +7,6 @@ class DeleteWholeCache extends Job { - /** - * @return string - */ public function getTitle(): string { return 'Remove the entire cache'; diff --git a/src/Job/GenerateStaticCacheJob.php b/src/Job/GenerateStaticCacheJob.php index f348df8c..e701a2f5 100644 --- a/src/Job/GenerateStaticCacheJob.php +++ b/src/Job/GenerateStaticCacheJob.php @@ -6,25 +6,15 @@ use SilverStripe\StaticPublishQueue\Publisher; /** - * Class GenerateStaticCacheJob - * add pages to static cache based on list of URLs - * - * @package SilverStripe\StaticPublishQueue\Job + * Add pages to static cache based on list of URLs */ class GenerateStaticCacheJob extends Job { - /** - * @return string - */ public function getTitle(): string { return 'Generate a set of static pages from URLs'; } - /** - * @param string $url - * @param int $priority - */ protected function processUrl(string $url, int $priority): void { $meta = Publisher::singleton()->publishURL($url, true); diff --git a/src/Job/StaticCacheFullBuildJob.php b/src/Job/StaticCacheFullBuildJob.php index 9580219e..cfbc20b0 100644 --- a/src/Job/StaticCacheFullBuildJob.php +++ b/src/Job/StaticCacheFullBuildJob.php @@ -10,8 +10,6 @@ use SilverStripe\Versioned\Versioned; /** - * Class StaticCacheFullBuildJob - * * Adds all live pages to the queue for caching. Best implemented on a cron via StaticCacheFullBuildTask. * WARNING: this job assumes that there are not too many pages to process (dozens are fine, thousands are not) * as collecting URLs from all live pages will either eat up all available memory and / or stall @@ -20,7 +18,6 @@ * * @property array $URLsToCleanUp * @property array $publishedURLs - * @package SilverStripe\StaticPublishQueue\Job */ class StaticCacheFullBuildJob extends Job { @@ -32,9 +29,6 @@ public function getTitle() return 'Generate static pages for all URLs'; } - /** - * @return string - */ public function getSignature(): string { return md5(static::class); @@ -118,9 +112,6 @@ public function process(): void $this->updateCompletedState(); } - /** - * @return array - */ protected function getAllLivePageURLs(): array { $urls = []; @@ -140,10 +131,6 @@ protected function getAllLivePageURLs(): array return $urls; } - /** - * @param string $url - * @param int $priority - */ protected function processUrl(string $url, int $priority): void { $meta = Publisher::singleton()->publishURL($url, true); diff --git a/src/Service/UrlBundleService.php b/src/Service/UrlBundleService.php index 04929b9e..7d8d88b9 100644 --- a/src/Service/UrlBundleService.php +++ b/src/Service/UrlBundleService.php @@ -2,6 +2,7 @@ namespace SilverStripe\StaticPublishQueue\Service; +use SilverStripe\Core\Config\Configurable; use SilverStripe\Core\Extensible; use SilverStripe\Core\Injector\Injectable; use SilverStripe\Core\Injector\Injector; @@ -18,15 +19,16 @@ class UrlBundleService implements UrlBundleInterface { use Extensible; use Injectable; + use Configurable; + + private static bool $strip_stage_param = true; protected array $urls = []; public function addUrls(array $urls): void { foreach ($urls as $url) { - $safeUrl = $this->stripStageParam($url); - - $this->urls[$safeUrl] = $safeUrl; + $this->urls[$url] = $url; } } @@ -105,6 +107,10 @@ protected function getUrls(): array */ protected function formatUrl(string $url): ?string { + if ($this->config()->get('strip_stage_param')) { + $url = $this->stripStageParam($url); + } + // Use this extension point to reformat URLs, for example encode special characters $this->extend('updateFormatUrl', $url); diff --git a/tests/php/Extension/Engine/SiteTreePublishingEngineTest.php b/tests/php/Extension/Engine/SiteTreePublishingEngineTest.php index 82a67b29..5d239bd3 100644 --- a/tests/php/Extension/Engine/SiteTreePublishingEngineTest.php +++ b/tests/php/Extension/Engine/SiteTreePublishingEngineTest.php @@ -52,9 +52,10 @@ public function testPublishRecursive(): void $expectedUrls = [ 'http://example.com/page-1/page-2/page-4', ]; + $resultUrls = array_keys($updateJob->getJobData()->jobData->URLsToProcess); $this->assertInstanceOf(GenerateStaticCacheJob::class, $updateJob); - $this->assertEqualsCanonicalizing($expectedUrls, array_keys($updateJob->getJobData()->jobData->URLsToProcess)); + $this->assertEqualsCanonicalizing($expectedUrls, $resultUrls); } public function testPublishRecursiveUrlChange(): void @@ -94,18 +95,15 @@ public function testPublishRecursiveUrlChange(): void 'http://example.com/page-1/page-2/page-4/page-5/page-6', ]; + $resultUpdateUrls = array_keys($updateJob->getJobData()->jobData->URLsToProcess); + $resultPurgeUrls = array_keys($deleteJob->getJobData()->jobData->URLsToProcess); + // Test our update job $this->assertInstanceOf(GenerateStaticCacheJob::class, $updateJob); - $this->assertEqualsCanonicalizing( - $expectedUpdateUrls, - array_keys($updateJob->getJobData()->jobData->URLsToProcess) - ); + $this->assertEqualsCanonicalizing($expectedUpdateUrls, $resultUpdateUrls); // Test our delete job $this->assertInstanceOf(DeleteStaticCacheJob::class, $deleteJob); - $this->assertEqualsCanonicalizing( - $expectedPurgeUrls, - array_keys($deleteJob->getJobData()->jobData->URLsToProcess) - ); + $this->assertEqualsCanonicalizing($expectedPurgeUrls, $resultPurgeUrls); } public function testPublishRecursiveParentChange(): void @@ -147,18 +145,15 @@ public function testPublishRecursiveParentChange(): void 'http://example.com/page-1/page-2/page-4/page-5/page-6', ]; + $resultUpdateUrls = array_keys($updateJob->getJobData()->jobData->URLsToProcess); + $resultPurgeUrls = array_keys($deleteJob->getJobData()->jobData->URLsToProcess); + // Test our update job $this->assertInstanceOf(GenerateStaticCacheJob::class, $updateJob); - $this->assertEqualsCanonicalizing( - $expectedUpdateUrls, - array_keys($updateJob->getJobData()->jobData->URLsToProcess) - ); + $this->assertEqualsCanonicalizing($expectedUpdateUrls, $resultUpdateUrls); // Test our delete job $this->assertInstanceOf(DeleteStaticCacheJob::class, $deleteJob); - $this->assertEqualsCanonicalizing( - $expectedPurgeUrls, - array_keys($deleteJob->getJobData()->jobData->URLsToProcess) - ); + $this->assertEqualsCanonicalizing($expectedPurgeUrls, $resultPurgeUrls); } public function testDoUnpublish(): void @@ -189,8 +184,10 @@ public function testDoUnpublish(): void 'http://example.com/page-1/page-2/page-4/page-5/page-6', ]; + $resultUrls = array_keys($deleteJob->getJobData()->jobData->URLsToProcess); + $this->assertInstanceOf(DeleteStaticCacheJob::class, $deleteJob); - $this->assertEqualsCanonicalizing($expectedUrls, array_keys($deleteJob->getJobData()->jobData->URLsToProcess)); + $this->assertEqualsCanonicalizing($expectedUrls, $resultUrls); } protected function getJobByClassName(array $jobs, string $className): ?QueuedJob diff --git a/tests/php/Extension/Publishable/PublishableSiteTreeTest.php b/tests/php/Extension/Publishable/PublishableSiteTreeTest.php index 9a475829..d00c0290 100644 --- a/tests/php/Extension/Publishable/PublishableSiteTreeTest.php +++ b/tests/php/Extension/Publishable/PublishableSiteTreeTest.php @@ -158,7 +158,7 @@ public function testObjectsActionPublishUrlChangeNoInclusion(): void $context = [ 'action' => 'publish', - 'urlChanged' => true, + 'urlSegmentChanged' => true, ]; $toDelete = $page->objectsToDelete($context); @@ -202,7 +202,41 @@ public function testObjectsActionUnpublishNoInclusion(): void // There should be 4 objects being deleted (our $page, its virtual page, child, and grandchild) $this->assertCount(4, $toDelete); - // There should be no objects being updates + // There should be no objects being updated + $this->assertCount(0, $toUpdate); + + // Check that the expected Pages are represented in our objectsToDelete() (order doesn't matter) + $this->assertEqualsCanonicalizing($expectedDeleteIds, $this->getIdsFromArray($toDelete)); + } + + public function testObjectsActionUnpublishNoStrictHierarchy(): void + { + // Disable strict hierarchy, meaning that children can remain published even when a parent is unpublished + SiteTree::config()->set('enforce_strict_hierarchy', false); + // Test that only the actioned page and its virtual page are added + SiteTree::config()->set('regenerate_parents', PublishableSiteTree::RELATION_INCLUDE_NONE); + // We are disabling child inclusion through our config, as well as strict hierarchy being disabled + SiteTree::config()->set('regenerate_children', PublishableSiteTree::RELATION_INCLUDE_NONE); + + $page = $this->objFromFixture(SiteTree::class, 'page3'); + $virtualPage = $this->objFromFixture(VirtualPage::class, 'page1'); + + // We expect the following pages to be deleted + $expectedDeleteIds = [ + $page->ID, + $virtualPage->ID, + ]; + + $context = [ + 'action' => 'unpublish', + ]; + + $toDelete = $page->objectsToDelete($context); + $toUpdate = $page->objectsToUpdate($context); + + // There should be 2 objects being deleted (our $page and its virtual page) + $this->assertCount(2, $toDelete); + // There should be no objects being updated $this->assertCount(0, $toUpdate); // Check that the expected Pages are represented in our objectsToDelete() (order doesn't matter) diff --git a/tests/php/Service/UrlBundleServiceTest.php b/tests/php/Service/UrlBundleServiceTest.php index b1937ca7..2487c5bd 100644 --- a/tests/php/Service/UrlBundleServiceTest.php +++ b/tests/php/Service/UrlBundleServiceTest.php @@ -13,7 +13,6 @@ class UrlBundleServiceTest extends SapphireTest { /** - * @param string $jobClass * @dataProvider jobClasses */ public function testJobsFromDataDefault(string $jobClass): void @@ -47,7 +46,6 @@ public function testJobsFromDataDefault(string $jobClass): void } /** - * @param string $jobClass * @dataProvider jobClasses */ public function testJobsFromDataExplicitUrlsPerJob(string $jobClass): void @@ -66,8 +64,6 @@ public function testJobsFromDataExplicitUrlsPerJob(string $jobClass): void } /** - * @param string $jobClass - * @param int $urlsPerJob * @dataProvider urlsPerJobCases */ public function testUrlsPerJob(string $jobClass, int $urlsPerJob): void @@ -83,8 +79,6 @@ public function testUrlsPerJob(string $jobClass, int $urlsPerJob): void } /** - * @param string $jobClass - * @param int $chunkSize * @dataProvider chunkCases */ public function testChunkSize(string $jobClass, int $chunkSize): void @@ -99,9 +93,6 @@ public function testChunkSize(string $jobClass, int $chunkSize): void $this->assertEquals($chunkSize, $method->invoke($job)); } - /** - * @return array - */ public function jobClasses(): array { return [ @@ -110,9 +101,6 @@ public function jobClasses(): array ]; } - /** - * @return array - */ public function urlsPerJobCases(): array { return [ @@ -127,9 +115,6 @@ public function urlsPerJobCases(): array ]; } - /** - * @return array - */ public function chunkCases(): array { return [ @@ -144,6 +129,44 @@ public function chunkCases(): array ]; } + public function testGetUrls(): void + { + $urls = [ + 'http://www.test.com?stage=Stage', + 'https://www.test.com?test1=1&stage=Live&test2=2' + ]; + $expectedUrls = [ + 'http://www.test.com', + 'https://www.test.com?test1=1&test2=2', + ]; + + $urlService = UrlBundleService::create(); + $urlService->addUrls($urls); + $method = new ReflectionMethod($urlService, 'getUrls'); + $method->setAccessible(true); + $resultUrls = $method->invoke($urlService); + + $this->assertEqualsCanonicalizing($expectedUrls, $resultUrls); + } + + public function testGetUrlsDontStripStage(): void + { + UrlBundleService::config()->set('strip_stage_param', false); + + $urls = [ + 'http://www.test.com?stage=Stage', + 'https://www.test.com?test1=1&stage=Live&test2=2' + ]; + + $urlService = UrlBundleService::create(); + $urlService->addUrls($urls); + $method = new ReflectionMethod($urlService, 'getUrls'); + $method->setAccessible(true); + $resultUrls = $method->invoke($urlService); + + $this->assertEqualsCanonicalizing($urls, $resultUrls); + } + /** * @dataProvider provideStripStageParamUrls */