NEW Refactor diff API and add managed limits.

Deprecate the getDiffedChanges method to get rid of confusing
fullHistory parameter. Add a getDiff method to fetch just a single diff
and a more versatile getDiffList method which allows specifying a limit.

We now also cap the amount of items coming from the diffing process
to a default value that we hope will result in generation times no
greater than a few seconds. This can be reconfigured by developers.
This commit is contained in:
Mateusz Uzdowski 2015-03-30 11:28:42 +13:00
parent 2d6337dad9
commit 8a7d2ec9da
3 changed files with 74 additions and 17 deletions

View File

@ -22,6 +22,14 @@ class VersionFeed extends SiteTreeExtension {
*/ */
private static $allchanges_enabled = true; private static $allchanges_enabled = true;
/**
* Allchanges feed limit of items.
*
* @config
* @var int
*/
private static $allchanges_limit = 20;
/** /**
* Enables RSS feed for page-specific changes * Enables RSS feed for page-specific changes
* *
@ -30,20 +38,31 @@ class VersionFeed extends SiteTreeExtension {
*/ */
private static $changes_enabled = true; private static $changes_enabled = true;
/**
* Changes feed limit of items.
*
* @config
* @var int
*/
private static $changes_limit = 100;
/** /**
* Compile a list of changes to the current page, excluding non-published and explicitly secured versions. * Compile a list of changes to the current page, excluding non-published and explicitly secured versions.
* *
* @param int $highestVersion Top version number to consider. * @param int $highestVersion Top version number to consider.
* @param boolean $fullHistory Whether to get the full change history or just the previous version. * @param int $limit Limit to the amount of items returned.
* *
* @returns ArrayList List of cleaned records. * @returns ArrayList List of cleaned records.
*/ */
public function getDiffedChanges($highestVersion = null, $fullHistory = true) { public function getDiffList($highestVersion = null, $limit = 100) {
// This can leak secured content if it was protected via inherited setting. // This can leak secured content if it was protected via inherited setting.
// For now the users will need to be aware about this shortcoming. // For now the users will need to be aware about this shortcoming.
$offset = $highestVersion ? "AND \"SiteTree_versions\".\"Version\"<='".(int)$highestVersion."'" : ''; $offset = $highestVersion ? "AND \"SiteTree_versions\".\"Version\"<='".(int)$highestVersion."'" : '';
$limit = $fullHistory ? null : 2; // Get just enough elements for diffing. We need one more than desired to have something to compare to.
$versions = $this->owner->allVersions("\"WasPublished\"='1' AND \"CanViewType\" IN ('Anyone', 'Inherit') $offset", "\"LastEdited\" DESC", $limit); $qLimit = (int)$limit + 1;
$versions = $this->owner->allVersions(
"\"WasPublished\"='1' AND \"CanViewType\" IN ('Anyone', 'Inherit') $offset", "\"LastEdited\" DESC", $qLimit
);
// Process the list to add the comparisons. // Process the list to add the comparisons.
$changeList = new ArrayList(); $changeList = new ArrayList();
@ -87,18 +106,54 @@ class VersionFeed extends SiteTreeExtension {
$previous = $version; $previous = $version;
} }
// Push the first version on to the list - only if we're looking at the full history or if it's the first // Make sure enough diff items have been generated to satisfy the $limit. If we ran out, add the final,
// version in the version history. // non-diffed item (the initial version). This will also work for a single-diff request: if we are requesting
if ($previous && ($fullHistory || $versions->count() == 1)) { // a diff on the initial version we will just get that version, verbatim.
if ($previous && $versions->count()<$qLimit) {
$first = clone($previous); $first = clone($previous);
$first->DiffContent = new HTMLText(); $first->DiffContent = new HTMLText();
$first->DiffContent->setValue('<div>' . $first->obj('Content')->forTemplate() . '</div>'); $first->DiffContent->setValue('<div>' . $first->obj('Content')->forTemplate() . '</div>');
// Copy the link so it can be cached by SS_Cache.
$first->GeneratedLink = $first->AbsoluteLink();
$changeList->push($first); $changeList->push($first);
} }
return $changeList; return $changeList;
} }
/**
* Return a single diff representing this version.
* Returns the initial version if there is nothing to compare to.
*
* @returns DataObject Object with relevant fields diffed.
*/
public function getDiff() {
$changes = $this->getDiffList($this->owner->Version, 1);
if ($changes && $changes->Count()) {
return $changes->First();
}
return null;
}
/**
* Compile a list of changes to the current page, excluding non-published and explicitly secured versions.
*
* @deprecated 2.0.0 Use VersionFeed::getDiffList instead
*
* @param int $highestVersion Top version number to consider.
* @param boolean $fullHistory Set to true to get the full change history, set to false for a single diff.
* @param int $limit Limit to the amount of items returned.
*
* @returns ArrayList List of cleaned records.
*/
public function getDiffedChanges($highestVersion = null, $fullHistory = true, $limit = 100) {
return $this->getDiffList(
$highestVersion,
$fullHistory ? $limit : 1
);
}
public function updateSettingsFields(FieldList $fields) { public function updateSettingsFields(FieldList $fields) {
if(!Config::inst()->get(get_class(), 'changes_enabled')) return; if(!Config::inst()->get(get_class(), 'changes_enabled')) return;

View File

@ -56,7 +56,7 @@ class VersionFeed_Controller extends Extension {
$target = $this->owner; $target = $this->owner;
$key = implode('_', array('changes', $this->owner->ID, $this->owner->Version)); $key = implode('_', array('changes', $this->owner->ID, $this->owner->Version));
$entries = $this->filterContent($key, function() use ($target) { $entries = $this->filterContent($key, function() use ($target) {
return $target->getDiffedChanges(); return $target->getDiffList(null, Config::inst()->get('VersionFeed', 'changes_limit'));
}); });
// Generate the output. // Generate the output.
@ -77,13 +77,14 @@ class VersionFeed_Controller extends Extension {
return $this->owner->httpError(404, 'Global history not viewable'); return $this->owner->httpError(404, 'Global history not viewable');
} }
$limit = (int)Config::inst()->get('VersionFeed', 'allchanges_limit');
$latestChanges = DB::query(' $latestChanges = DB::query('
SELECT * FROM "SiteTree_versions" SELECT * FROM "SiteTree_versions"
WHERE "WasPublished" = \'1\' WHERE "WasPublished" = \'1\'
AND "CanViewType" IN (\'Anyone\', \'Inherit\') AND "CanViewType" IN (\'Anyone\', \'Inherit\')
AND "ShowInSearch" = 1 AND "ShowInSearch" = 1
AND ("PublicHistory" IS NULL OR "PublicHistory" = \'1\') AND ("PublicHistory" IS NULL OR "PublicHistory" = \'1\')
ORDER BY "LastEdited" DESC LIMIT 20' ORDER BY "LastEdited" DESC LIMIT ' . $limit
); );
$lastChange = $latestChanges->record(); $lastChange = $latestChanges->record();
$latestChanges->rewind(); $latestChanges->rewind();
@ -110,8 +111,9 @@ class VersionFeed_Controller extends Extension {
// Get the diff to the previous version. // Get the diff to the previous version.
$version = new Versioned_Version($record); $version = new Versioned_Version($record);
$changes = $version->getDiffedChanges($version->Version, false); if ($diff = $version->getDiff()) {
if ($changes && $changes->Count()) $changeList->push($changes->First()); $changeList->push($diff);
}
} }
return $changeList; return $changeList;

View File

@ -39,13 +39,13 @@ class VersionFeedTest extends SapphireTest {
$this->assertContains( $this->assertContains(
str_replace(' ' ,'',_t('RSSHistory.TITLECHANGED', 'Title has changed:') . 'My Changed Title'), str_replace(' ' ,'',_t('RSSHistory.TITLECHANGED', 'Title has changed:') . 'My Changed Title'),
array_map($cleanDiffOutput, $page->getDiffedChanges()->column('DiffTitle')), array_map($cleanDiffOutput, $page->getDiffList()->column('DiffTitle')),
'Detects published title changes' 'Detects published title changes'
); );
$this->assertNotContains( $this->assertNotContains(
str_replace(' ' ,'',_t('RSSHistory.TITLECHANGED', 'Title has changed:') . 'My Unpublished Changed Title'), str_replace(' ' ,'',_t('RSSHistory.TITLECHANGED', 'Title has changed:') . 'My Unpublished Changed Title'),
array_map($cleanDiffOutput, $page->getDiffedChanges()->column('DiffTitle')), array_map($cleanDiffOutput, $page->getDiffList()->column('DiffTitle')),
'Ignores unpublished title changes' 'Ignores unpublished title changes'
); );
} }