diff --git a/src/Slug.php b/src/Slug.php index 9f19ebe..ec238d2 100644 --- a/src/Slug.php +++ b/src/Slug.php @@ -3,12 +3,12 @@ namespace Nightjar\Slug; use InvalidArgumentException; -use UnexpectedValueException; -use SilverStripe\ORM\DataObject; +use SilverStripe\Control\Controller; use SilverStripe\Forms\FieldList; use SilverStripe\ORM\DataExtension; -use SilverStripe\Control\Controller; +use SilverStripe\ORM\DataObject; use SilverStripe\View\Parsers\URLSegmentFilter; +use UnexpectedValueException; /** * Adds a 'url slug' property to DataObject classes, in order for them to be able to be loaded via a URL @@ -76,8 +76,8 @@ class Slug extends DataExtension /** * Apply extension with configurable defaults * - * @param string $fieldToSlug The field on the owner to base the URL Slug from - defaults to 'Title' - * @param string $relationName Optional name of the has_one relationship to the owner's parent class/Page + * @param string $fieldToSlug The field on the owner to base the URL Slug from - defaults to 'Title' + * @param string $relationName Optional name of the has_one relationship to the owner's parent class/Page * @param boolean $enforceParity true to alter the URLSlug whenever the $fieldToSlug changes value (default: false) */ public function __construct($fieldToSlug = 'Title', $relationName = null, $enforceParity = false) @@ -132,72 +132,104 @@ public function setSlugActive($active) } /** - * Generate a url slug segment - * - * @param boolean $forceRegeneration - * @return string + * Check for collisions, if we need to update the slug and + * update the model with the confirmedsafe value before writing */ - public function getSlug($forceRegeneration = false) + public function onBeforeWrite() { - $owner = $this->getOwner(); - $field = $this->fieldToSlug; - $unfilteredSlug = $owner->URLSlug; - if (!$unfilteredSlug || $forceRegeneration) { - $unfilteredSlug = $owner->$field; + if ($this->slugIsOutdated()) { + $this->updateSlug(); + } elseif ($this->slugHasChanged()) { + // enforceParity is set, and the slug has changed... but the + // field to slug has not. We need to reset to keep parity. + // Ideally we could do this via the $original property through + // {@see DataObject::getChangedFields}, however the returned + // 'before' value for a changed field is unreliable :( + // https://github.com/silverstripe/silverstripe-framework/issues/8443 + throw new UnexpectedValueException( + 'URLSlug has been updated independently of the tracked field, ' . + 'but this has been disabled via Slug::enforceParity' + ); } - return URLSegmentFilter::create()->filter($unfilteredSlug); } /** - * Check for collisions, iff we need to update the slug and - * upate the model with the confirmedsafe value before writing + * Helper to decide if a slug is outdated + * + * @return bool */ - public function onBeforeWrite() + public function slugIsOutdated(): bool { $owner = $this->getOwner(); - $slugHasChanged = $owner->isChanged('URLSlug'); + $slugHasChanged = $this->slugHasChanged(); $fieldToSlugHasChanged = $owner->isChanged($this->fieldToSlug); - $updateSlug = ( + $relationHasChanged = $this->relationName && $owner->isChanged($this->relationName . 'ID'); + + return ( empty($owner->URLSlug) || ($this->enforceParity && $fieldToSlugHasChanged) || (!$this->enforceParity && $slugHasChanged) + || $relationHasChanged ); + } - if ($updateSlug) { - $owner->URLSlug = $this->getSlug($this->enforceParity); - - $collisionList = DataObject::get(get_class($owner))->exclude('ID', $owner->ID); - $filter = ['URLSlug' => $owner->URLSlug]; - if ($this->relationName) { - $parentIDField = $this->relationName . 'ID'; - $filter[$parentIDField] = $owner->$parentIDField; - // Also handle polymorphic relationships - $parentClassName = DataObject::getSchema()->hasOneComponent(get_class($owner), $this->relationName); - if ($parentClassName === DataObject::class) { - $parentClassField = $this->relationName . 'Class'; - $filter[$parentClassField] = $owner->$parentClassField; - } - } + /** + * the actual logic for updating the slug and fix collisions with other slugs that have the same parent + */ + public function updateSlug(): void + { + $owner = $this->getOwner(); + $owner->URLSlug = $this->getSlug($this->enforceParity); - $count = 1; - while ($collisionList->filter($filter)->exists()) { - $owner->URLSlug = $owner->URLSlug . $count++; - $filter['URLSlug'] = $owner->URLSlug; + $collisionList = DataObject::get(get_class($owner))->exclude('ID', $owner->ID); + $filter = ['URLSlug' => $owner->URLSlug]; + + if ($this->relationName) { + $parentIDField = $this->relationName . 'ID'; + $filter[$parentIDField] = $owner->$parentIDField; + // Also handle polymorphic relationships + $parentClassName = DataObject::getSchema()->hasOneComponent(get_class($owner), $this->relationName); + if ($parentClassName === DataObject::class) { + $parentClassField = $this->relationName . 'Class'; + $filter[$parentClassField] = $owner->$parentClassField; } - } elseif ($slugHasChanged) { - // enforceParity is set, and the slug has changed... but the - // field to slug has not. We need to reset to keep parity. - // Ideally we could do this via the $original property through - // {@see DataObject::getChangedFields}, however the returned - // 'before' value for a changed field is unreliable :( - // https://github.com/silverstripe/silverstripe-framework/issues/8443 - throw new UnexpectedValueException( - 'URLSlug has been updated independently of the tracked field, ' . - 'but this has been disabled via Slug::enforceParity' - ); + } + + $count = 1; + $origSlug = $owner->URLSlug; + while ($collisionList->filter($filter)->exists()) { + $owner->URLSlug = implode('-', [$origSlug, $count++]); + $filter['URLSlug'] = $owner->URLSlug; } } + /** + * Generate a url slug segment + * + * @param boolean $forceRegeneration + * @return string + */ + public function getSlug($forceRegeneration = false) + { + $owner = $this->getOwner(); + $field = $this->fieldToSlug; + $unfilteredSlug = $owner->URLSlug; + if (!$unfilteredSlug || $forceRegeneration) { + $unfilteredSlug = $owner->$field; + } + return URLSegmentFilter::create()->filter($unfilteredSlug); + } + + /** + * Helper to decide if the slug field has changed + * + * @return bool + */ + public function slugHasChanged(): bool + { + return $this->owner->isChanged('URLSlug'); + } + public function updateCMSFields(FieldList $fields) { if ($this->enforceParity) { @@ -218,7 +250,7 @@ public function Link($action = null) $link = null; $owner = $this->getOwner(); $action = ($action) ? Controller::join_links($owner->URLSlug, $action) : $owner->URLSlug; - + $relationName = $this->relationName; if ($relationName && ($parent = $owner->$relationName()) && $parent->hasMethod('Link')) { $link = $parent->Link($action); @@ -230,46 +262,46 @@ public function Link($action = null) } /** - * Returns true if this is the slugged object being used to handle this request. + * Return "link" or "section" depending on if this is the current viewing object. + * {@see isCurrent} * - * @return boolean + * @return string */ - public function isCurrent() + public function LinkOrCurrent() { - return $this->active === self::ACTIVE_CURRENT; + return $this->isCurrent() ? 'current' : 'link'; } /** - * Check if this slugged object is in the currently active section - * (i.e. it, or one of its children is currently being viewed). + * Returns true if this is the slugged object being used to handle this request. * * @return boolean */ - public function isSection() + public function isCurrent() { - return $this->isCurrent() || ($this->active === self::ACTIVE_SECTION); + return $this->active === self::ACTIVE_CURRENT; } /** - * Return "link" or "section" depending on if this is the current viewing object. - * {@see isCurrent} + * Return "link" or "section" depending on if this is the current section. + * {@see isSection} * * @return string */ - public function LinkOrCurrent() + public function LinkOrSection() { - return $this->isCurrent() ? 'current' : 'link'; + return $this->isSection() ? 'section' : 'link'; } /** - * Return "link" or "section" depending on if this is the current section. - * {@see isSection} + * Check if this slugged object is in the currently active section + * (i.e. it, or one of its children is currently being viewed). * - * @return string + * @return boolean */ - public function LinkOrSection() + public function isSection() { - return $this->isSection() ? 'section' : 'link'; + return $this->isCurrent() || ($this->active === self::ACTIVE_SECTION); } /** diff --git a/tests/SlugHandlerTest.php b/tests/SlugHandlerTest.php index 4beb108..27bb0d9 100644 --- a/tests/SlugHandlerTest.php +++ b/tests/SlugHandlerTest.php @@ -23,29 +23,6 @@ class SlugHandlerTest extends FunctionalTest Journalist::class, ]; - /** - * By all accounts this should be setUpBeforeClass... but it is too coarse a call to achieve getting - * Journalist.URLSlug into the database (modifying Conifg before calling parent::setUpBeforeClass is - * too soon, and calling after parent;:setUpBeforeClass is too late!). So we must hijack a call from - * within setUpBeforeClass to allow for this. This is the only viable option! - * - * The reason for this is because we cannot define TestOnly yaml configuration like we can with php - * fixture classes, and here we are also testing defining a service to apply the extension works. - * This is important for e.g. backwards compatiblity aliases - */ - public static function getExtraDataObjects() - { - $config = Config::modify(); - $config->set(Injector::class, 'JournalistSlug', [ - 'class' => Slug::class, - 'constructor' => ['Name', 'NewsPages'], - ]); - $config->merge(Journalist::class, 'extensions', ['JournalistSlug']); - - // Do what we actually came here for - return parent::getExtraDataObjects(); - } - protected function setUp() { parent::setUp(); diff --git a/tests/SlugTest.php b/tests/SlugTest.php index b1adbe0..fc1d8bf 100644 --- a/tests/SlugTest.php +++ b/tests/SlugTest.php @@ -2,35 +2,31 @@ namespace Nightjar\Slug\Tests; -use Nightjar\Slug\Slug; use InvalidArgumentException; -use UnexpectedValueException; -use SilverStripe\Dev\SapphireTest; -use SilverStripe\Core\Config\Config; +use Nightjar\Slug\Slug; use Nightjar\Slug\Tests\Stubs\Article; use Nightjar\Slug\Tests\Stubs\Blitzem; -use Nightjar\Slug\Tests\Stubs\NewsPage; -use SilverStripe\Core\Injector\Injector; use Nightjar\Slug\Tests\Stubs\Journalist; +use Nightjar\Slug\Tests\Stubs\NewsPage; +use SilverStripe\Core\Config\Config; +use SilverStripe\Dev\SapphireTest; +use UnexpectedValueException; class SlugTest extends SapphireTest { - protected $usesTransactions = false; - protected static $fixture_file = 'SlugTest.yml'; - protected static $extra_dataobjects = [ Article::class, Blitzem::class, NewsPage::class, Journalist::class, ]; - protected static $required_extensions = [ Journalist::class => [ Slug::class ] ]; + protected $usesTransactions = false; public function testGettingSlug() { @@ -58,13 +54,6 @@ public function testCannotAssociateToInvalidRelationshipType() public function testSlugsWillSetAndSanitiseOnSave() { - $config = Config::modify(); - $config->set(Injector::class, 'JournalistSlug', [ - 'class' => Slug::class, - 'constructor' => ['Name', 'NewsPages'], - ]); - $config->merge(Journalist::class, 'extensions', ['JournalistSlug']); - $journo = Journalist::create(); $journo->update(['Name' => 'Ash Katchum!'])->extend('onBeforeWrite'); $this->assertEquals('ash-katchum', $journo->URLSlug, 'initial write should sanitise'); @@ -76,7 +65,7 @@ public function testSlugsWillSetAndSanitiseOnSave() public function testSlugKeepsParity() { $newArticle = Article::create(); - + $newArticle->update(['Title' => 'Second News'])->fakeWrite(); $this->assertEquals('second-news', $newArticle->URLSlug); @@ -99,8 +88,35 @@ public function testSlugCollisionsCorrectThemselves() 'Title' => 'First news', 'ParentID' => $newsPage->ID, ]); - $newArticle->extend('onBeforeWrite'); - $this->assertEquals('first-news1', $newArticle->URLSlug); + $newArticle->write(); + $this->assertEquals('first-news-1', $newArticle->URLSlug); + $this->assertEquals('First news', $newArticle->Title); + + $anotherArticle = Article::create()->update([ + 'Title' => 'First news', + 'ParentID' => $newsPage->ID, + ]); + $anotherArticle->write(); + $this->assertEquals('first-news-2', $anotherArticle->URLSlug); + $this->assertEquals('First news', $anotherArticle->Title); + } + + public function testSlugCollisionFixesItselfWhenParentChanges() + { + $newsPage = $this->objFromFixture(NewsPage::class, 'holder'); + $newArticle = Article::create()->update([ + 'Title' => 'First news', + 'ParentID' => 0, + ]); + $newArticle->write(); + $this->assertEquals('first-news', $newArticle->URLSlug, + 'Slug doesn\'t need to be fixed when objects have different parents'); + $this->assertEquals('First news', $newArticle->Title); + + $newArticle->ParentID = $newsPage->ID; + $newArticle->write(); + $this->assertEquals('first-news-1', $newArticle->URLSlug, + 'Slug should fix when it collides with a slug in a new parent'); $this->assertEquals('First news', $newArticle->Title); } diff --git a/tests/Stubs/Journalist.php b/tests/Stubs/Journalist.php index 5a83081..c8b9f08 100644 --- a/tests/Stubs/Journalist.php +++ b/tests/Stubs/Journalist.php @@ -2,6 +2,7 @@ namespace Nightjar\Slug\Tests\Stubs; +use Nightjar\Slug\Slug; use SilverStripe\Dev\TestOnly; use SilverStripe\ORM\DataObject; @@ -16,4 +17,8 @@ class Journalist extends DataObject implements TestOnly private static $has_one = [ 'NewsPages' => NewsPage::class ]; + + private static $extensions = [ + Slug::class . '("Name", "NewsPages")' + ]; }