Upgrade project to work with SilverStripe 4

Adds much needed tests to ensure functionality remains as intended,
and takes care of some code cleanliness. There is now heavy documentation
to help serve the secondary purpose of a learning tool for interested parties.

One caveat is that the tests rely on a feature not yet merged into core, that
of being able to apply extensions via Injector services. The extension will
remain functional however, only some of the test suite may fail.

A new feature has also been introduced in the way of an extension for the
controller portion of the slug handling - instead of relying on commented
examples in the config file.

This new feature also allows for having multiple associations on a Controller
to various slugged objects, and is flexible in whether or not a slugged item
is available as the action itself, or under a different name denoting different
relationships to the data.

Readme.md

@@ -11,7 +11,8 @@
 
 1. `composer require nightjar/ss-slug`
 2. Apply the extensions as desired
-3. `dev/build`
+3. Optionally add configuration
+4. `dev/build`
 
 ## Usage
 
@@ -34,7 +35,7 @@ class Item extends DataObject {
         'Parent' => 'ItemsPage'
     ];
     public function Link() { // Will override extension's Link() method
-        return $this->Parent()->Link().$this->URLSlug.'/';
+        return $this->Parent()->Link($this->URLSlug);
     }
 }
 ```
@@ -62,7 +63,7 @@ class ItemsPage extends Page {
 }
 ```
 
-Then comes the much more necessary part where one adds a Controller method to access the items decorated items via a request (provided example here is for a Page_Controller). The method/action names are of course exemplary; So long as the applicable configuration elements are all consistent, the names can of course change.
+Then comes the much more necessary part where one adds a Controller method to access the items decorated items via a request (provided example here is for a PageController). The controller extension works by providing new `url_handlers`, directing to an action called `handleSlug`. One could define a custom handler to deal with this logic, should the need arise.
 
 ```php
 namespace MyVendor\MyNamespace;
@@ -71,45 +72,43 @@ use PageController;
 use Nightjar\Slug\SlugHandler;
 
 class ItemsPageController extends PageController {
-    private static $url_handlers = [
-        '$Item!' => 'viewItem'
-    ];
-    private static $allowed_actions = [
-        'viewItem'
+    private static $extensions = [
+        'Nightjar\Slug\SlugHandler'
     ];
-    public function viewItem() {
-        $item = $this->getItem();
-        if(!$item) $this->httpError(404);
-        return ['Item' => $this->Items()->filter('URLSlug', $this->request->param('Item'))->first()];
-    }
-    //One can use something like this to <% if $Top.activeItem == $Slug %> in a menu
-    public function activeItem() {
-        return $this->request->param('Item');
-    }
 }
 ```
 
-One can then define a template such as MyVendor/MyNamespace/ItemsPage_viewItem.ss
+One can then define a template such as MyVendor/MyNamespace/ItemsPage_handleSlug.ss
 
 ```html
-<% with $Item %>
+<% with $ActiveSlug %>
     $Title
     ...
 <% end_with %>
 ```
 
 ## About
-The holder/child page type pattern is often... potentially unwieldy in undefined numbers. Other modules don't particularly give the flexibility required. ModelAdmin is for managing a type of Model, not for exposing them as pages via a site's front end. Slug provides a much needed a more generic solution, and can be applied easily to multiple objects on a site with minimal fuss. It should even work when they're accessed through the same controller provided one takes care in setting up the access methods.
+The holder/child page type pattern is often... potentially unwieldy in large numbers (more than 10). Other modules don't particularly give the flexibility required. ModelAdmin is for managing a type of Model, not for exposing them as pages via a site's front end. Slug provides a much needed a more generic solution, and can be applied easily to multiple objects on a site with minimal fuss. It should even work when they're accessed directly through the a controller, provided one takes care in setting up the configuration.
 
 ### Properties
-The Extension takes three parameters, all of which are optional:
+The Slug Extension takes three constructor parameters, all of which are optional:
 
 1. The name of the field it should use to create a slug. (defaults to 'Title')
 2. The name of a relation on the 'parent' object (Page, in the example above), allowing for nested URLs. By default a slug must of course be unique, and this is usually globally to the extended class. However defining a 'parent ID' allows for a slug to be unique under that parent only. Eg. With the page setup above if `ItemsPage`s were set up to list primary colours, one can have both `primary-colours/red` AND `primary-light-colours/red`, rather than `primary-light-colours/red1`. (defaults to null)
 3. A parity bit. If set to true, whenever the field the module is bound to (Title by default, see 1.) is updated, the slug will automatically update also (keeping parity). Setting this to true also prevents the slug from being manually altered via the CMS by _not_ applying the field to the EditForm. (defaults to false)
 
+The SlugHandler extension takes two constructor parameters, both of which are optional:
+
+1. The name of the method to get the list of slugged items on. Usually a relationshp name. If set no other relationships will be looked at for slugs (see **About:Configuration** below), and the slugged items can all be loaded as if they were the action - e.g. `some-page/slugged-item`. (defaults to `null`)
+2. The source of data to call methods to fetch slugged item lists from. Controllers do not have relationships to objects, so we must first get one to be able to fetch related items via a slug. e.g. a PageController must first get a Page to be able to source it's data, so too must the SlugHandler find data with this method before dereferencing it. (defaults to 'getFailover')
+
+### Configuration
+The SlugHandler by default will look for routes by loading the configuration value of `slug_trails` from either the Controller it is associated with, or the DataObject (or other object - refer to the second constructor parameter explanation above) it leverages to access the list of slugged items. The format of this is `['route-name' => 'RelationshipName', ...]` in php, or `route-name: RelationshipName` in yaml.
+
+With this configuration a route will be valid in the form `some-page/route-name/slugged-item`, where `route-name` is always static. This allows one to define routes to multiple relationships on a page instead of just one. To omit this configuration will necessitate the use of the first parameter to the SlugHandler constructor in order to be able to view any items from a request.
+
 ## Notes
-- Very simple in code, however an often sought after concept implemented as a great (and flexible) time saver. Concepts also good as a tutorial for learners... uuh, learning. Covers extensions, actions, routing, & request parameters.
+- Very simple in code, however an often sought after concept implemented as a great (and flexible) time saver. Concepts also good as a tutorial for learners... uuh, learning. Covers extensions, actions, routing, & request parameters. Code is heavily commented with documentation blocks for this reason.
 - If a DataObject named Lettuce exists, it's data consistency will be compromised. Apply the silverstripe-blitzem extension to protect against this.
 - This extension will probably cease to work if the DataObject it is applied to is named [BLITZEM](http://www.yates.co.nz/brand/blitzem/). Untested.
 - _The previous two notes are jokes._ :)

src/Slug.php

@@ -3,6 +3,7 @@
 namespace Nightjar\Slug;
 
 use InvalidArgumentException;
+use UnexpectedValueException;
 use SilverStripe\ORM\DataObject;
 use SilverStripe\Forms\FieldList;
 use SilverStripe\ORM\DataExtension;
@@ -89,14 +90,18 @@ public function __construct($fieldToSlug = 'Title', $relationName = null, $enfor
 
     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) {
-            $valid = DataObject::getSchema()->hasOneComponent(get_class($owner), $this->relationName);
+            $ownerClass = get_class($owner);
+            $valid = DataObject::getSchema()->hasOneComponent($ownerClass, $this->relationName);
             if (!$valid) {
-                throw new InvalidArgumentException("$relationName is invalid has_one on " . get_class());
+                throw new InvalidArgumentException("$this->relationName is an invalid has_one on $ownerClass");
             }
         }
-        parent::setOwner($owner);
     }
 
     /**
@@ -136,11 +141,11 @@ public function getSlug($forceRegeneration = false)
     {
         $owner = $this->getOwner();
         $field = $this->fieldToSlug;
-        $slug = $owner->URLSlug;
-        if (!$slug || $forceRegeneration) {
-            $slug = URLSegmentFilter::create()->filter($owner->$field);
+        $unfilteredSlug = $owner->URLSlug;
+        if (!$unfilteredSlug || $forceRegeneration) {
+            $unfilteredSlug = $owner->$field;
         }
-        return $slug;
+        return URLSegmentFilter::create()->filter($unfilteredSlug);
     }
 
     /**
@@ -179,6 +184,17 @@ public function onBeforeWrite()
                 $owner->URLSlug = $owner->URLSlug . $count++;
                 $filter['URLSlug'] = $owner->URLSlug;
             }
+        } 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'
+            );
         }
     }
 
@@ -199,13 +215,18 @@ public function updateCMSFields(FieldList $fields)
      */
     public function Link($action = null)
     {
+        $link = null;
         $owner = $this->getOwner();
-        $slugAction = [$owner->URLSlug];
-        if ($action) {
-            array_push($slugAction, $action);
+        $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()) {
+            // Quite the assumption, but sufficient in most cases.
+            $link = Controller::curr()->Link($action);
         }
-        // Quite the assumption, but sufficient in most cases.
-        return Controller::has_curr() ? Controller::curr()->Link($slugAction) : null;
+        return $link;
     }
 
     /**

src/SlugHandler.php

@@ -2,8 +2,11 @@
 
 namespace Nightjar\Slug;
 
-use SilverStripe\Control\HTTPRequest;
+use LogicException;
 use SilverStripe\Core\Extension;
+use SilverStripe\ORM\DataObject;
+use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Control\HTTPResponse_Exception;
 
 /**
  * This is for handling a request for a slugged DataObject, and should be applied to a Controller.
@@ -52,34 +55,46 @@ class SlugHandler extends Extension
      *
      * @var array
      */
-    protected $data;
+    protected $dataSource;
 
     /**
      * Apply extension to {@see SilverStripe\Control\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 parameter could even be `Me` if the controller is 
+     * 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 $data getter function name
+     * @param string $dataSource getter function name
      */
-    public function __construct($relationship = null, $data = 'getFailover')
+    public function __construct($relationship = null, $dataSource = 'getFailover')
     {
         parent::__construct();
         $this->slugs = $relationship ? ['' => $relationship] : null;
-        $this->data = $data;
+        $this->dataSource = $dataSource;
     }
 
+    /**
+     * Fetches the slugged item at the end of a route (slime trail).
+     *
+     * @return DataObject
+     * @throws LogicException
+     * @throws HTTPResponse_Exception 404
+     */
     protected function findSlug()
     {
-        // You're probably wondering about these variable names...
         /** @var SilverStripe\Control\Controller */
         $owner = $this->getOwner();
         $request = $owner->getRequest();
-        $plot = $this->data;
+
+        // You're probably wondering about these variable names...
+        $plot = $this->dataSource;
         $garden = $owner->$plot();
+        if (!$garden) {
+            $mrMcGregors = get_class($owner);
+            throw new LogicException("There is no garden in $mrMcGregors::$plot() to find Slugs in!");
+        }
         $slime = $this->slugs;
         if (!$slime) {
             $slime = $garden->config()->get('slug_trails') ?: $owner->config()->get('slug_trails');
@@ -99,9 +114,9 @@ protected function findSlug()
      */
     public function handleSlug(HTTPRequest $request)
     {
-        $owner = $this->getOwner();
         $item = $this->findSlug();
         if (!$item) {
+            $owner = $this->getOwner();
             $owner->httpError(404);
         }
         $item->setSlugActive(Slug::ACTIVE_CURRENT);

tests/SlugHandlerTest.php

@@ -2,46 +2,98 @@
 
 namespace Nightjar\Slug\Tests;
 
+use Nightjar\Slug\Slug;
+use InvalidArgumentException;
 use SilverStripe\Control\Director;
+use SilverStripe\Core\Config\Config;
 use SilverStripe\Dev\FunctionalTest;
+use Nightjar\Slug\Tests\Stubs\Article;
+use Nightjar\Slug\Tests\Stubs\NewsPage;
+use SilverStripe\Core\Injector\Injector;
+use Nightjar\Slug\Tests\Stubs\Journalist;
 use Nightjar\Slug\Tests\Stubs\NewsController;
 
 class SlugHandlerTest extends FunctionalTest
 {
     protected static $fixture_file = 'SlugTest.yml';
 
-    protected function setUp()
+    protected static $extra_dataobjects = [
+        Article::class,
+        NewsPage::class,
+        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()
     {
-        parent::setUp();
-        Config::modify()->set(
-            Journalist::class,
-            'extensions',
-            [[
-                'Slug' => [
-                    'constuctor' => ['Name']
-                ]
-            ]]
-        );
-        Config::modify()->set(
-            Director::class,
-            'rules',
-            ['news' => NewsController::class]
-        );
+        $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();
     }
 
-    public function testHandlingSlugs()
+    protected function setUp()
     {
-        $output = $this->get('news/article/first-news');
-        var_dump($output);
+        parent::setUp();
+        $config = Config::modify();
+        $data = $this->objFromFixture(NewsPage::class, 'holder');
+        Injector::inst()->registerService($data, 'NewsPage');
+        $spec = [
+            'properties' => [
+                'Failover' => '%$NewsPage',
+            ],
+        ];
+        $config->merge(Injector::class, NewsController::class, $spec);
+
+        // This could be done via $extra_controllers - but this would not allow us
+        // to register the injection service above, which is critical for this test.
+        // This is due to SapphireTest::getExtraRoutes instantiating each extra
+        // controller as a singleton - which makes the above property injection irrelevant.
+        $config->set(Director::class, 'rules', ['news' => NewsController::class]);
     }
 
-    public function testHandlingSlugs()
+    public function testRequestingSlugs()
     {
-        $response = new HTTPRequest('GET', 'news/stories/first-news');
-        $this->assertArrayHasKey('Item', $reponse);
-        $this->assertInstanceOf(Article::class, $response['Item']);
-        $responseArticle = $response['Item']
-        $dbArticle = $this->objFromFixture(Article::class, 'one');
-        $this->assertSame($dbArticle->ID, $responseArticle->ID);
+        $jim = $this->objFromFixture(Journalist::class, 'jim');
+        $news = $this->objFromFixture(NewsPage::class, 'holder');
+        $news->Journalists()->add($jim);
+
+        $output = $this->get('news/');
+        $this->assertEquals('Index ok', $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());
+        $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());
+        $output = $this->get('news/jimbo-the-journo');
+        $this->assertEquals(404, $output->getStatusCode());
+
+        $output = $this->get('news/stories/jimbo-the-journo');
+        $this->assertEquals(404, $output->getStatusCode());
+        $output = $this->get('news/contributors/first-news');
+        $this->assertEquals(404, $output->getStatusCode());
     }
 }