NEW AdminController as superclass for /admin/* routed controllers #1836

GuySartorelli · 2024-10-14T23:09:17Z

There are two commits here:

Mostly-unrelated refactor to use config instead of runtime code to remove the CMSProfileController menu item
Add the new AdminController class and wire it up

I've also added a bunch of strong typing to the new class and to AdminRouteController.
I have not added strong typing to LeftAndMain though that may happen as part of a follow-up card in this epic.
I have not added strong typing to methods which would required extensive trickle-down effects (e.g. init() and canView()).

Issue

Refactor admin/* routing logic from LeftAndMain into its own abstract class #1761

GuySartorelli · 2024-10-16T00:12:21Z

_config.php

@@ -31,5 +31,3 @@
    ]);
 // enable ability to insert anchors
 $editorConfig->insertButtonsAfter('sslink', 'anchor');
-
-CMSMenu::remove_menu_class(CMSProfileController::class);


That's handled by config in the class itself now.

GuySartorelli · 2024-10-16T00:13:03Z

code/AdminController.php

Basically everything in here was just lifted straight from LeftAndMain - though some small code quality improvements were made here-and-there.

GuySartorelli · 2024-10-16T00:13:37Z

code/AdminController.php

+        if (static::class === AdminController::class) {
+            throw new BadMethodCallException('getRequiredPermissions should be called on a subclass');
+        }


Just to avoid it being called here, since this is explicitly abstract there's no permission for this class (or reason to want one)

GuySartorelli · 2024-10-16T00:14:51Z

code/AdminController.php

+        // LeftAndMain methods have a top-level uri access
+        if (static::class === LeftAndMain::class) {
+            $segment = '';


A follow-up card will make LeftAndMain abstract after removing the methods on it that are used from singleton - but for now this has to stay.

GuySartorelli · 2024-10-16T00:15:39Z

code/AdminController.php

+    public function redirect(string $url, int $code = 302): HTTPResponse
+    {
+        if ($this->getRequest()->isAjax()) {
+            $response = $this->getResponse();
+            $response->addHeader('X-ControllerURL', $url);
+            if ($this->getRequest()->getHeader('X-Pjax') && !$response->getHeader('X-Pjax')) {
+                $response->addHeader('X-Pjax', $this->getRequest()->getHeader('X-Pjax'));
+            }
+            $newResponse = new LeftAndMain_HTTPResponse(
+                $response->getBody(),
+                $response->getStatusCode(),
+                $response->getStatusDescription()
+            );
+            foreach ($response->getHeaders() as $k => $v) {
+                $newResponse->addHeader($k, $v);
+            }
+            $newResponse->setIsFinished(true);
+            $this->setResponse($newResponse);
+            // Actual response will be re-requested by client
+            return $newResponse;
+        } else {
+            return parent::redirect($url, $code);
+        }
+    }


I'm not 100% sure exactly what all of the ramifications of this are - but since it's dealing explicitly with AJAX requests and that's mostly what this method is for, I decided to lift this out of LeftAndMain and into this class.

GuySartorelli · 2024-10-16T00:16:36Z

code/AdminController.php

+    public function getClientConfig(): array
+    {
+        // Allows the section name to be overridden in config
+        $name = static::config()->get('section_name') ?: static::class;
+        // Trim leading/trailing slash to make it easier to concatenate URL
+        // and use in routing definitions.
+        $url = trim($this->Link(), '/');
+        $clientConfig = [
+            'name' => $name,
+            'url' => $url,
+            'reactRoutePath' => preg_replace('/^' . preg_quote(AdminRootController::admin_url(), '/') . '/', '', $url),
+        ];
+        $this->extend('updateClientConfig', $clientConfig);
+        return $clientConfig;
+    }


This is lifted over here so that we can inject links for these controllers into the config that JS gets a hold of.

GuySartorelli · 2024-10-16T00:17:31Z

code/AdminController.php

+        // Don't allow access if the request hasn't finished being handled and the user can't access this controller
+        if (!$this->canView() && !$this->getResponse()->isFinished()) {
+            // Allow subclasses and extensions to redirect somewhere more appropriate
+            $this->invokeWithExtensions('onInitPermissionFailure');


This lets LeftAndMain do some redirecting it was doing before without making the method abstract (and therefore required on classes that don't need it) and without doing a method_exists() check which always feels like the wrong move.

GuySartorelli · 2024-10-16T00:18:49Z

code/AdminController.php

+            // if no alternate menu items have matched, return a permission error
+            $messageSet = [
+                'default' => _t(
+                    __CLASS__ . '.PERMDEFAULT',
+                    "You must be logged in to access the administration area."
+                ),
+                'alreadyLoggedIn' => _t(
+                    __CLASS__ . '.PERMALREADY',
+                    "I'm sorry, but you can't access that part of the CMS."
+                ),
+                'logInAgain' => _t(
+                    __CLASS__ . '.PERMAGAIN',
+                    "You have been logged out of the CMS."
+                ),
+            ];


Shortened these messages to suit the fact that most of the time these controllers will be handling AJAX requests and therefore there won't be a menu "below" like the old messages suggest.

I considered finding a way to use the longer messages in LeftAndMain specifically, but the context of "there's a form below" is unnecessary because... well, there's a form below. You can see it (or tab to it if you're on a screen reader)

GuySartorelli · 2024-10-16T00:19:41Z

code/AdminRootController.php

+     */
+    private static string $url_base = 'admin';
+
+    /**
+     * The LeftAndMain child that will be used as the initial panel to display if none is selected (i.e. if you
+     * visit /admin)
+     */
+    private static string $default_panel = SecurityAdmin::class;


These are just moved from below per our coding standards (i.e. properties before methods)

GuySartorelli · 2024-10-16T00:20:25Z

code/AdminRootController.php

-            $classes = CMSMenu::get_cms_classes(null, true, CMSMenu::URL_PRIORITY);
+            $classes = CMSMenu::get_cms_classes(AdminController::class, true, CMSMenu::URL_PRIORITY);


null defaults to LeftAndMain - we want the new controller as the base class.

GuySartorelli · 2024-10-16T00:20:43Z

code/AdminRootController.php

-            if (($arguments = $request->match($pattern, true)) !== false) {
+            if ($request->match($pattern, true) !== false) {


Unrelated refactoring since that variable is never used.

GuySartorelli · 2024-10-16T00:20:59Z

code/CMSProfileController.php

@@ -22,6 +22,8 @@ class CMSProfileController extends LeftAndMain

    private static $tree_class = Member::class;

+    private static $ignore_menuitem = true;


This is the config that lets us remove the code from _config.php

GuySartorelli · 2024-10-16T00:21:31Z

code/LeftAndMain.php

+    private static $help_links = [
+        'CMS User help' => 'https://userhelp.silverstripe.org/en/5',
+        'Developer docs' => 'https://docs.silverstripe.org/en/5/',
+        'Community' => 'https://www.silverstripe.org/',
+        'Feedback' => 'https://www.silverstripe.org/give-feedback/',
+    ];
+
+    /**
+     * The href for the anchor on the Silverstripe logo
+     *
+     * @config
+     * @var string
+     */
+    private static $application_link = '//www.silverstripe.org/';
+
+    /**
+     * The application name
+     *
+     * @config
+     * @var string
+     */
+    private static $application_name = 'Silverstripe';


Moved from below, along with searchFilterCache as well. These were below some methods.

GuySartorelli · 2024-10-16T00:21:49Z

code/LeftAndMain.php

-        $cmsClassNames = CMSMenu::get_cms_classes(LeftAndMain::class, true, CMSMenu::URL_PRIORITY);
+        $cmsClassNames = CMSMenu::get_cms_classes(AdminController::class, true, CMSMenu::URL_PRIORITY);


The config itself can be on any AdminController now.

GuySartorelli · 2024-10-16T00:23:11Z

code/SudoModeController.php

@@ -91,7 +89,7 @@ public function activate(HTTPRequest $request): HTTPResponse
            return $this->jsonResponse([
                'result' => false,
                'message' => _t(__CLASS__ . '.INVALID', 'Incorrect password'),
-            ]);
+            ], 400);


Unrelated refactoring - this is an error, so should have an error response code! There's no context in the original PR that created this class for why it was a 200. It looks like it was an oversight.

This should be 401, context for this is getting password wrong when trying to activate sudo mode.

https://stackoverflow.com/a/32752617

I hate HTTP response codes lol.
Updated to 401

GuySartorelli · 2024-10-16T00:23:45Z

tests/php/AdminControllerTest.php

+        }
+    }
+
+    public static function provideJsonSuccess(): array


This and everything below it was pulled from LeftAndMainTest.

GuySartorelli · 2024-10-16T00:24:24Z

tests/php/LeftAndMainTest.php

-        $admin = $this->objFromFixture(Member::class, 'admin');
-        $this->logInAs($admin);
+        $this->logInWithPermission('ADMIN');


The admin fixture and the logInWithPermission('ADMIN') user have the same permissions, so no need for the fixture.

GuySartorelli · 2024-10-16T00:25:31Z

tests/php/LeftAndMainTest.php

@@ -135,65 +128,6 @@ public function testLeftAndMainSubclasses()
        $this->assertMatchesRegularExpression('/<body[^>]*>/i', $response->getBody(), "$link should contain <body> tag");
    }

-    public function testCanView()


This was testing the wrong thing - it was trying to test canView() checks by seeing what's in the menu. But the menu actually shouldn't contain CMSProfileController so this test was passing when it should have failed.

Moved canView check to the new test class, and now actually testing canView instead of the menu.

The menu itself is already tested in this class.

emteknetnz · 2024-10-17T04:53:14Z

code/SudoModeController.php

@@ -91,7 +89,7 @@ public function activate(HTTPRequest $request): HTTPResponse
            return $this->jsonResponse([
                'result' => false,
                'message' => _t(__CLASS__ . '.INVALID', 'Incorrect password'),
-            ]);
+            ], 400);


This should be 401, context for this is getting password wrong when trying to activate sudo mode.

https://stackoverflow.com/a/32752617

emteknetnz · 2024-10-17T04:55:31Z

code/AdminRootController.php

+
+    /**
+     * Holds an array of url_pattern => controller k/v pairs, the same as Director::rules. However this is built
+     * dynamically from introspecting on all the classes that derive from LeftAndMain.


Suggested change

* dynamically from introspecting on all the classes that derive from LeftAndMain.

* dynamically from introspecting on all the classes that derive from AdminController.

emteknetnz · 2024-10-17T05:16:38Z

code/AdminController.php

+            return;
+        }
+
+        $this->extend('onInit');


Weird place for an 'onInit' hook - seems like it should be at the bottom? Any reason why it's here? Does the implementation in LeftAndMain not work right if this hook is at the bottom?

I'm just keeping it in the same logic place it was with LeftAndMain. I don't know if it would break anything if we move it, and I don't particularly want to find out. Chances are if it breaks anything to move it, we won't find out for a year or so.
I'm guessing it's intentionally placed and that it's specifically not called onAfterInit() for that reason, though I can't confirm that.

Safer to just leave it where it is. But I can move it if you feel strongly.

emteknetnz · 2024-10-17T05:21:13Z

code/LeftAndMain.php

+        'CMS User help' => 'https://userhelp.silverstripe.org/en/5',
+        'Developer docs' => 'https://docs.silverstripe.org/en/5/',


Suggested change

'CMS User help' => 'https://userhelp.silverstripe.org/en/5',

'Developer docs' => 'https://docs.silverstripe.org/en/5/',

'CMS User help' => 'https://userhelp.silverstripe.org/en',

'Developer docs' => 'https://docs.silverstripe.org/en/',

We'll need to update a bunch of references like this, we should probably just do them all together. But I'll make this change to avoid ping pong.
Going with an explicit 6 so people don't get docs for 7 in their 6 sites when we get to that point.

ENH Use config instead of runtime code to remove menu item

d2dd58c

GuySartorelli marked this pull request as draft October 14, 2024 23:09

GuySartorelli force-pushed the pulls/3/new-admincontroller branch from 993a9c2 to 05aad97 Compare October 15, 2024 00:41

This was referenced Oct 15, 2024

DOC Document new AdminController silverstripe/developer-docs#598

Open

API Make use of the new AdminController silverstripe/silverstripe-externallinks#151

Open

GuySartorelli force-pushed the pulls/3/new-admincontroller branch 2 times, most recently from ae6c47b to 7d9c9d0 Compare October 15, 2024 23:13

GuySartorelli commented Oct 16, 2024

View reviewed changes

GuySartorelli marked this pull request as ready for review October 16, 2024 00:25

emteknetnz requested changes Oct 17, 2024

View reviewed changes

NEW AdminController as superclass for /admin/* routed controllers

93f9ce2

GuySartorelli force-pushed the pulls/3/new-admincontroller branch from 7d9c9d0 to 93f9ce2 Compare October 17, 2024 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW AdminController as superclass for /admin/* routed controllers #1836

NEW AdminController as superclass for /admin/* routed controllers #1836

GuySartorelli commented Oct 14, 2024

GuySartorelli Oct 16, 2024

GuySartorelli Oct 16, 2024

GuySartorelli Oct 16, 2024

GuySartorelli Oct 16, 2024

GuySartorelli Oct 16, 2024

GuySartorelli Oct 16, 2024

GuySartorelli Oct 16, 2024

GuySartorelli Oct 16, 2024 •

edited

Loading

GuySartorelli Oct 16, 2024

GuySartorelli Oct 16, 2024

GuySartorelli Oct 16, 2024

GuySartorelli Oct 16, 2024

GuySartorelli Oct 16, 2024

GuySartorelli Oct 16, 2024

GuySartorelli Oct 16, 2024

emteknetnz Oct 17, 2024

GuySartorelli Oct 17, 2024

GuySartorelli Oct 16, 2024

GuySartorelli Oct 16, 2024

GuySartorelli Oct 16, 2024

emteknetnz Oct 17, 2024

emteknetnz Oct 17, 2024

GuySartorelli Oct 17, 2024

emteknetnz Oct 17, 2024

GuySartorelli Oct 17, 2024

emteknetnz Oct 17, 2024

emteknetnz Oct 17, 2024

GuySartorelli Oct 17, 2024

		$classes = CMSMenu::get_cms_classes(null, true, CMSMenu::URL_PRIORITY);
		$classes = CMSMenu::get_cms_classes(AdminController::class, true, CMSMenu::URL_PRIORITY);

		if (($arguments = $request->match($pattern, true)) !== false) {
		if ($request->match($pattern, true) !== false) {

		@@ -22,6 +22,8 @@ class CMSProfileController extends LeftAndMain

		private static $tree_class = Member::class;

		private static $ignore_menuitem = true;

		$cmsClassNames = CMSMenu::get_cms_classes(LeftAndMain::class, true, CMSMenu::URL_PRIORITY);
		$cmsClassNames = CMSMenu::get_cms_classes(AdminController::class, true, CMSMenu::URL_PRIORITY);

	* dynamically from introspecting on all the classes that derive from LeftAndMain.
	* dynamically from introspecting on all the classes that derive from AdminController.

		'CMS User help' => 'https://userhelp.silverstripe.org/en/5',
		'Developer docs' => 'https://docs.silverstripe.org/en/5/',

NEW AdminController as superclass for /admin/* routed controllers #1836

Are you sure you want to change the base?

NEW AdminController as superclass for /admin/* routed controllers #1836

Conversation

GuySartorelli commented Oct 14, 2024

Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Oct 16, 2024 •

edited

Loading