Skip to content

fix slug collisions and consecutive numbers #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wernerkrauss wants to merge 6 commits into
base: master
Choose a base branch
from
Open

fix slug collisions and consecutive numbers #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9bb7ad9
fix broken tests
wernerkrauss Apr 29, 2020
974fa9f
seperate slug and consecutive number with a dash
wernerkrauss Apr 29, 2020
00cee2c
slug should have a consecutive number
wernerkrauss Apr 29, 2020
bd5ebd4
add failing test: slugs can collide when parent changes
wernerkrauss Apr 29, 2020
8a924f6
fix slug collision when parent changes
wernerkrauss Apr 29, 2020
75fd9c7
refactoring
wernerkrauss Apr 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
170 changes: 101 additions & 69 deletions src/Slug.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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) {
Expand All @@ -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);
Expand All @@ -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);
}

/**
Expand Down
23 changes: 0 additions & 23 deletions tests/SlugHandlerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
56 changes: 36 additions & 20 deletions tests/SlugTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down Expand Up @@ -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');
Expand All @@ -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);

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

Expand Down
Loading