Skip to content

Commit

Permalink
Peer review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@chrispenny
chrispenny committed Apr 4, 2023
1 parent 92f4617 commit 0b66f71
Show file tree
Hide file tree
Showing 12 changed files with 224 additions and 192 deletions.
45 changes: 35 additions & 10 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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'
```
124 changes: 62 additions & 62 deletions src/Extension/Engine/SiteTreePublishingEngine.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -57,50 +57,41 @@ 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
{
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;
}

/**
Expand All @@ -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();
}
}

Expand All @@ -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();
}

Expand All @@ -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();
}

Expand All @@ -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()));
}
});
}

Expand All @@ -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([]);
}
}
}
41 changes: 25 additions & 16 deletions src/Extension/Publishable/PublishableSiteTree.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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;
Expand All @@ -98,23 +99,31 @@ 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) {
$list[] = $virtual;
}
}

// 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;
}
Expand Down
Loading

0 comments on commit 0b66f71

Please sign in to comment.