Skip to content

Update to Silverstripe 6 #20

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 1 commit into
base: master
Choose a base branch
from
Open

Update to Silverstripe 6 #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,11 @@
"slack": "https://silverstripe-users.slack.com"
},
"require": {
"silverstripe/framework": "^4||^5"
"php": "^8.3",
"silverstripe/framework": "^6"
},
"require-dev": {
"phpunit/phpunit": "^5.7",
"phpunit/phpunit": "^11",
"squizlabs/php_codesniffer": "^3.0"
},
"autoload": {
Expand Down
97 changes: 40 additions & 57 deletions src/Slug.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,101 +3,82 @@
namespace Nightjar\Slug;

use InvalidArgumentException;
use UnexpectedValueException;
use SilverStripe\ORM\DataObject;
use SilverStripe\Forms\FieldList;
use SilverStripe\ORM\DataExtension;
use Override;
use SilverStripe\Control\Controller;
use SilverStripe\Core\Extension;
use SilverStripe\Forms\FieldList;
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
* through a{@see SilverStripe\Control\Controller}. The typical use case for this is to view records related
* to a {@see Page}, to be able to have a controller action that doesn't need to reference the ID ofthe record.
* through a{@see \SilverStripe\Control\Controller}. The typical use case for this is to view records related
* to a {@see Page}, to be able to have a controller action that doesn't need to reference the ID of the record.
* E.g. /products/nice-jacket - where this is /pageType/relatedObject-notAPage
*
* To simplify the controller section of this purpose {@see SlugHandler}
*/
class Slug extends DataExtension
class Slug extends Extension
{
/**
* Used to set {@see $active} to inactive, or 'link'
*/
const ACTIVE_NONE = null;
const null ACTIVE_NONE = null;

/**
* Used to set and check {@see $active} as a section (ancestor of 'current')
*/
const ACTIVE_SECTION = false;
const false ACTIVE_SECTION = false;

/**
* Used to set and check {@see $active} as the current object active in a request
*/
const ACTIVE_CURRENT = true;
const true ACTIVE_CURRENT = true;

private static $db = [
private static array $db = [
'URLSlug' => 'Varchar(255)',
];

private static $indexes = [
private static array $indexes = [
'URLSlug' => true,
];

/**
* The field on the owner that we should take the value from in order to generate a slug
*
* @var string
*/
protected $fieldToSlug;

/**
* If we should restrict the slugging uniqueness to a certain subset of the owner class,
* this will be the name of the relation to filter by to detect uniqueness
*
* @var string|null
*/
protected $relationName;

/**
* Whether or not we should update the URLSlug field when the field to slug changes
*
* @var boolean
*/
protected $enforceParity;

/**
* The owner has been accessed via a route involving the URLSlug
* Tri state; current, section, none
*
* @var null|boolean {@see setSlugActive}
*/
protected $active = null;
protected ?bool $active = null;

/**
* 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 that we should take the value from to generate a slug - 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)
{
public function __construct(
protected $fieldToSlug = 'Title',
/**
* If we should restrict the slugging uniqueness to a certain subset of the owner class,
* this will be the name of the relation to filter by to detect uniqueness
*/
protected $relationName = null,
protected $enforceParity = false
) {
parent::__construct();
$this->fieldToSlug = $fieldToSlug;
$this->relationName = $relationName;
$this->enforceParity = $enforceParity;
}

#[Override]
public function setOwner($owner)
{
// parent method is set in a try, with a finally to clearOwner - so it is important we set it first,
// otherwise our InvalidArgumentException will be swallowed by a BadMethodCallException!
parent::setOwner($owner);

// throw an exception if the $relationName is invalid or not has_one
if ($this->relationName) {
$ownerClass = get_class($owner);
$valid = DataObject::getSchema()->hasOneComponent($ownerClass, $this->relationName);
if ($this->relationName && $owner) {
$ownerClass = $owner::class;
$valid = DataObject::getSchema()?->hasOneComponent($ownerClass, $this->relationName);
if (!$valid) {
throw new InvalidArgumentException("$this->relationName is an invalid has_one on $ownerClass");
}
Expand Down Expand Up @@ -134,7 +115,7 @@ public function setSlugActive($active)
/**
* Generate a url slug segment
*
* @param boolean $forceRegeneration
* @param boolean $forceRegeneration
* @return string
*/
public function getSlug($forceRegeneration = false)
Expand Down Expand Up @@ -166,13 +147,13 @@ public function onBeforeWrite()
if ($updateSlug) {
$owner->URLSlug = $this->getSlug($this->enforceParity);

$collisionList = DataObject::get(get_class($owner))->exclude('ID', $owner->ID);
$collisionList = DataObject::get($owner::class)->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);
$parentClassName = DataObject::getSchema()->hasOneComponent($owner::class, $this->relationName);
if ($parentClassName === DataObject::class) {
$parentClassField = $this->relationName . 'Class';
$filter[$parentClassField] = $owner->$parentClassField;
Expand All @@ -181,7 +162,7 @@ public function onBeforeWrite()

$count = 1;
while ($collisionList->filter($filter)->exists()) {
$owner->URLSlug = $owner->URLSlug . $count++;
$owner->URLSlug .= $count++;

@NightJar NightJar Mar 10, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated because this is just syntactic change, but it occurs to me we might end up with slug12345 instead of slug5 😂 - should probably write a test at some point (no need to do it for this PR though).

$filter['URLSlug'] = $owner->URLSlug;
}
} elseif ($slugHasChanged) {
Expand Down Expand Up @@ -218,11 +199,11 @@ 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);
} elseif (Controller::has_curr()) {
} elseif (Controller::curr() === null) {

@NightJar NightJar Mar 10, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the exact opposite of what we want 😉
The next statement is $link = (null)->Link($action) in this case.

Since has_curr was deleted I guess it should be !== instead.
It could use assignment as to not call the function twice, but I'll leave it for you to decide.

// Quite the assumption, but sufficient in most cases.
$link = Controller::curr()->Link($action);
}
Expand Down Expand Up @@ -282,10 +263,12 @@ public function LinkingMode()
{
if ($this->isCurrent()) {
return 'current';
} elseif ($this->isSection()) {
}

if ($this->isSection()) {
return 'section';
} else {
return 'link';
}

return 'link';
}
}
28 changes: 10 additions & 18 deletions src/SlugHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,18 @@
namespace Nightjar\Slug;

use LogicException;
use SilverStripe\Core\Extension;
use SilverStripe\ORM\DataObject;
use SilverStripe\Control\Controller;
use SilverStripe\Control\HTTPRequest;
use SilverStripe\Control\HTTPResponse_Exception;
use SilverStripe\Core\Extension;

/**
* This is for handling a request for a slugged DataObject, and should be applied to a Controller.
*
* One can either supply the method name (to get slugs from) as a string to the constructor,
* or define a slug_trails {@see SilverStripe\Core\Config} property on the Controller.
* or define a slug_trails {@see \SilverStripe\Core\Config} property on the Controller.
* The latter allows support for multiple slugged relationships to reside on the same
* {@see SilverStripe\ORM\DataObject}, and the `slug_trails` property is an array map
* {@see \SilverStripe\ORM\DataObject}, and the `slug_trails` property is an array map
* in the format of
* [ $Trail => $RelationshipName, ... ]
* where $Trail is the URL segment {@see $url_handlers} it is loaded over,
Expand Down Expand Up @@ -51,28 +51,20 @@ class SlugHandler extends Extension
protected $slugs;

/**
* Which function on the Controller will get us the initial DataObject?
*
* @var array
*/
protected $dataSource;

/**
* Apply extension to {@see SilverStripe\Control\Controller}.
* Apply extension to {@see Controller}.
* Supply a parameter as a shorthand if handling multiple slugs on the same object is unneeded.
* Also takes the name of the initial getter function used to move from Controller to Model,
* the default is set to 'getFailover' as this is nicely generic - so if this property is not set
* then a failover should be ensured. The data source parameter could even be `Me` if the controller is
* 'bare' and getter methods exist directly on it for each slugged object type (with no parent relation).
*
* @param string $relationship Relationship name (optional)
* @param string $dataSource getter function name
* @param string $dataSource getter function name; Which function on the Controller will get us the initial DataObject?
*/
public function __construct($relationship = null, $dataSource = 'getFailover')
public function __construct($relationship = null, protected $dataSource = 'getFailover')
{
parent::__construct();
$this->slugs = $relationship ? ['' => $relationship] : null;
$this->dataSource = $dataSource;
}

/**
Expand All @@ -84,15 +76,15 @@ public function __construct($relationship = null, $dataSource = 'getFailover')
*/
protected function findSlug()
{
/** @var SilverStripe\Control\Controller */
/** @var Controller */
$owner = $this->getOwner();
$request = $owner->getRequest();

// You're probably wondering about these variable names...
$plot = $this->dataSource;
$garden = $owner->$plot();
if (!$garden) {
$mrMcGregors = get_class($owner);
$mrMcGregors = $owner::class;
throw new LogicException("There is no garden in $mrMcGregors::$plot() to find Slugs in!");
}
$slime = $this->slugs;
Expand Down
18 changes: 8 additions & 10 deletions tests/SlugHandlerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

namespace Nightjar\Slug\Tests;

use Override;
use Nightjar\Slug\Slug;
use InvalidArgumentException;
use SilverStripe\Control\Director;
use SilverStripe\Core\Config\Config;
use SilverStripe\Dev\FunctionalTest;
Expand Down Expand Up @@ -33,20 +33,18 @@ class SlugHandlerTest extends FunctionalTest
* 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
*/
#[Override]
public static function getExtraDataObjects()
{
$config = Config::modify();
$config->set(Injector::class, 'JournalistSlug', [
'class' => Slug::class,
'constructor' => ['Name', 'NewsPages'],
$config->set(Journalist::class, 'extensions', [
Slug::class . '("Name")'

@NightJar NightJar Mar 10, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change also neglects to carry over the second parameter to the constructor.

Am I correct in recalling that the test didn't pass previously? 😬
I think I originally wrote it at the same time as silverstripe/silverstripe-framework#8444

@wernerkrauss wernerkrauss Mar 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afair the second param didn't pass before, so it was useless.

]);
$config->merge(Journalist::class, 'extensions', ['JournalistSlug']);

// Do what we actually came here for
return parent::getExtraDataObjects();
}

protected function setUp(): void
protected function setUp(): void
{
parent::setUp();
$config = Config::modify();
Expand All @@ -73,21 +71,21 @@ public function testRequestingSlugs()
$news->Journalists()->add($jim);

$output = $this->get('news/');
$this->assertEquals('Index ok', $output->getBody());
$this->assertEquals('Index ok', trim((string) $output->getBody()));
$output = $this->get('news/nonsense/first-news');
$this->assertEquals(404, $output->getStatusCode());

$output = $this->get('news/stories/');
$this->assertEquals(404, $output->getStatusCode());
$output = $this->get('news/stories/first-news');
$this->assertEquals('First News', $output->getBody());
$this->assertEquals('First News', trim((string) $output->getBody()));
$output = $this->get('news/first-news');
$this->assertEquals(404, $output->getStatusCode());

$output = $this->get('news/contributors/');
$this->assertEquals(404, $output->getStatusCode());
$output = $this->get('news/contributors/jimbo-the-journo');
$this->assertEquals('Jimbo the Journo', $output->getBody());
$this->assertEquals('Jimbo the Journo', trim((string) $output->getBody()));

@NightJar NightJar Mar 10, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there an issue with the output? The trim call should be unnecessary; I'd rather not mutate the output under assertion to make a test pass - it should pass without interference.

Perhaps I misunderstand and the behaviour of getBody has changed?

Same for the other 2 calls above.

$output = $this->get('news/jimbo-the-journo');
$this->assertEquals(404, $output->getStatusCode());

Expand Down
23 changes: 10 additions & 13 deletions tests/SlugTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,15 @@

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
{
Expand Down Expand Up @@ -53,18 +52,16 @@ public function testCannotAssociateToInvalidRelationshipType()

$this->expectException(InvalidArgumentException::class);
// Try to go through the Blitzem to get to the tasty Lettuce!
Blitzem::create();
//we need to extend onBeforeWrite to ensure setOwner() is called
Blitzem::create()->extend('onBeforeWrite');
}

public function testSlugsWillSetAndSanitiseOnSave()
{
$config = Config::modify();
$config->set(Injector::class, 'JournalistSlug', [
'class' => Slug::class,
'constructor' => ['Name', 'NewsPages'],
$config->set(Journalist::class, 'extensions', [
Slug::class . '("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 +73,7 @@ public function testSlugsWillSetAndSanitiseOnSave()
public function testSlugKeepsParity()
{
$newArticle = Article::create();

$newArticle->update(['Title' => 'Second News'])->fakeWrite();
$this->assertEquals('second-news', $newArticle->URLSlug);

Expand Down
Loading