From 9bb7ad9508c24b7de1b9d0ffbf13a351989ddd0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Werner=20M=2E=20Krau=C3=9F?= Date: Wed, 29 Apr 2020 10:02:57 +0200 Subject: [PATCH 1/6] fix broken tests --- tests/SlugHandlerTest.php | 23 ----------------------- tests/SlugTest.php | 9 +-------- tests/Stubs/Journalist.php | 5 +++++ 3 files changed, 6 insertions(+), 31 deletions(-) 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..d8919da 100644 --- a/tests/SlugTest.php +++ b/tests/SlugTest.php @@ -58,13 +58,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 +69,7 @@ public function testSlugsWillSetAndSanitiseOnSave() public function testSlugKeepsParity() { $newArticle = Article::create(); - + $newArticle->update(['Title' => 'Second News'])->fakeWrite(); $this->assertEquals('second-news', $newArticle->URLSlug); 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")' + ]; } From 974fa9f80a97e11123611ec2f50fc2ca1d03a97c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Werner=20M=2E=20Krau=C3=9F?= Date: Wed, 29 Apr 2020 10:05:55 +0200 Subject: [PATCH 2/6] seperate slug and consecutive number with a dash --- src/Slug.php | 4 ++-- tests/SlugTest.php | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Slug.php b/src/Slug.php index 9f19ebe..ed36557 100644 --- a/src/Slug.php +++ b/src/Slug.php @@ -181,7 +181,7 @@ public function onBeforeWrite() $count = 1; while ($collisionList->filter($filter)->exists()) { - $owner->URLSlug = $owner->URLSlug . $count++; + $owner->URLSlug = implode('-', [$owner->URLSlug, $count++]); $filter['URLSlug'] = $owner->URLSlug; } } elseif ($slugHasChanged) { @@ -218,7 +218,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); diff --git a/tests/SlugTest.php b/tests/SlugTest.php index d8919da..19e727e 100644 --- a/tests/SlugTest.php +++ b/tests/SlugTest.php @@ -93,7 +93,7 @@ public function testSlugCollisionsCorrectThemselves() 'ParentID' => $newsPage->ID, ]); $newArticle->extend('onBeforeWrite'); - $this->assertEquals('first-news1', $newArticle->URLSlug); + $this->assertEquals('first-news-1', $newArticle->URLSlug); $this->assertEquals('First news', $newArticle->Title); } From 00cee2c1099d294110374b5f30f916134b4a6ef3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Werner=20M=2E=20Krau=C3=9F?= Date: Wed, 29 Apr 2020 10:13:52 +0200 Subject: [PATCH 3/6] slug should have a consecutive number fixes #11 1 12 123 isn't that consecutive, is it? --- src/Slug.php | 3 ++- tests/SlugTest.php | 10 +++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/Slug.php b/src/Slug.php index ed36557..79c5566 100644 --- a/src/Slug.php +++ b/src/Slug.php @@ -180,8 +180,9 @@ public function onBeforeWrite() } $count = 1; + $origSlug = $owner->URLSlug; while ($collisionList->filter($filter)->exists()) { - $owner->URLSlug = implode('-', [$owner->URLSlug, $count++]); + $owner->URLSlug = implode('-', [$origSlug, $count++]); $filter['URLSlug'] = $owner->URLSlug; } } elseif ($slugHasChanged) { diff --git a/tests/SlugTest.php b/tests/SlugTest.php index 19e727e..4b495c2 100644 --- a/tests/SlugTest.php +++ b/tests/SlugTest.php @@ -92,9 +92,17 @@ public function testSlugCollisionsCorrectThemselves() 'Title' => 'First news', 'ParentID' => $newsPage->ID, ]); - $newArticle->extend('onBeforeWrite'); + $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); } /** From bd5ebd43240c8b40d4d97884f6ce8034eaf92a86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Werner=20M=2E=20Krau=C3=9F?= Date: Wed, 29 Apr 2020 10:24:49 +0200 Subject: [PATCH 4/6] add failing test: slugs can collide when parent changes --- tests/SlugTest.php | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/tests/SlugTest.php b/tests/SlugTest.php index 4b495c2..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() { @@ -105,6 +101,25 @@ public function testSlugCollisionsCorrectThemselves() $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); + } + /** * Uses the database. */ From 8a924f6a2d4759b42fcaa58e6b837ab346c41924 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Werner=20M=2E=20Krau=C3=9F?= Date: Wed, 29 Apr 2020 10:36:31 +0200 Subject: [PATCH 5/6] fix slug collision when parent changes --- src/Slug.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Slug.php b/src/Slug.php index 79c5566..fcb59d8 100644 --- a/src/Slug.php +++ b/src/Slug.php @@ -157,10 +157,13 @@ public function onBeforeWrite() $owner = $this->getOwner(); $slugHasChanged = $owner->isChanged('URLSlug'); $fieldToSlugHasChanged = $owner->isChanged($this->fieldToSlug); + $relationHasChanged = $this->relationName && $owner->isChanged($this->relationName . 'ID'); + $updateSlug = ( empty($owner->URLSlug) || ($this->enforceParity && $fieldToSlugHasChanged) || (!$this->enforceParity && $slugHasChanged) + || $relationHasChanged ); if ($updateSlug) { From 75fd9c70432c4f1960884260b50ad1145b3c8689 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Werner=20M=2E=20Krau=C3=9F?= Date: Wed, 29 Apr 2020 10:46:37 +0200 Subject: [PATCH 6/6] refactoring onbeforewrite is a bit more readable added helper functions with better names --- src/Slug.php | 166 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 97 insertions(+), 69 deletions(-) diff --git a/src/Slug.php b/src/Slug.php index fcb59d8..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,76 +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); $relationHasChanged = $this->relationName && $owner->isChanged($this->relationName . 'ID'); - $updateSlug = ( + 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); + + $collisionList = DataObject::get(get_class($owner))->exclude('ID', $owner->ID); + $filter = ['URLSlug' => $owner->URLSlug]; - $count = 1; - $origSlug = $owner->URLSlug; - while ($collisionList->filter($filter)->exists()) { - $owner->URLSlug = implode('-', [$origSlug, $count++]); - $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) { @@ -234,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); } /**