SPIKE Analyse how LeftAndMain and CMSMain are put together and how they could work better

[maxime-rainville]

CMSMain, LeftAndMain, ModelAdmin and other related Admin controllers have been around and have accumulated quite a bit of technical debt.

Timebox

3 days

Objectives of this SPIKE

• Review the architecture of LeftAndMain, CMSMain, ModelAdmin and any other related class.
• Identify key methods and their purpose and structure them into inter-related groups.
• Propose a more sensible architecture and break down. This may include:
  • moving method between classes
  • creating brand new classes
  • splitting concerns into composable services or traits
• Identify other concerns or discrepancies that could be address in a future major release
• Report findings at a CMS Architecture catch up

Related cards

• #2896
• silverstripe/silverstripe-admin#348

POC PRs

For demonstrative purposes only

• #2939

[GuySartorelli]

LeftAndMain

tl;dr

LeftAndMain could be broken into the following:

• abstract AdminRouteController
  • Possibly a new class, or possibly dump stuff into the existing AdminRootController
  • Handles permissions and routing against /admin/*
  • Any controller which should have a /admin/* route (i.e. it's intended to be used in the CMS, not on the front-end) should be a subclass of this.
• abstract FormSchemaController
  • Either a subclass of AdminRouteController or a trait
  • Has all the stuff we need to handle formschema modal stuff.
  • The LinkFieldController class would either use this trait or be a subclass of this class.
• abstract LeftAndMain
  • Implements whatever is needed for classes to be in the "left" menu, and have a "main" edit form

Recommended changes in depth

Move all properties to the top of the class. This can be done in CMS 5. For some reason there's properties that are below some of the methods, which makes them harder to find.

LeftAndMain should be made abstract - the few cases that currently require a singleton of LeftAndMain should be refactored to not require that. From what I could see, those are mostly related to formschema currently.

Break into multiple classes

abstract AdminRouteController

• Possibly a new class, or possibly dump stuff into the existing AdminRootController
• Handles permissions and routing against /admin/*
• No coupling with templates
• Any controller which should have a /admin/* route (i.e. it's intended to be used in the CMS, not on the front-end) should be a subclass of this.

abstract FormSchemaController

• Either a subclass of AdminRouteController or a trait
• The LinkFieldController class would either use this trait or be a subclass of this class.

abstract LeftAndMain

• Implements whatever is needed for classes to be in the "left" menu, and have a "main" edit form
• Basically, if it doesn't go into either of the other classes (or get deleted), it stays here.

tree_class config

There's a tree_class config (the same that CMSMain uses) - which is silly.

• e.g. SecurityAdmin has it set to Group and AssetAdmin has it set to Folder - but both of those manage multiple classes.
• This config is used in the following methods on LeftAndMain:

  method name	description	recommendation
  getRecord()	Get a record by ID, based on the tree_class. LeftAndMain is the only class that seems to override this, and that's to work with the history viewer (getting specific versions of records). Doesn't seem to be used much, but probably not worth removing it.	Leave as is.
  save()	Action for clicking "save" button on a form. Almost nothing uses it directly, and anything that does will get its own superclass as we'll see further down.	make abstract
  getNewItem()	Only called in the save() method - CMSMain calls it but it also overrides it with its own implementation. Probably not useful, uses an antipattern of creating an item with a specific ID.	Delete it. Use injector or ::create() directly for the few places that call this method.
  delete()	Action for clicking "delete" button on a form. ModelAdmin doesn't use it 'cause that's handled by GridField. CMSMain overrides it. SiteConfig and Reports don't have a delete button, and AssetAdmin` does stuff in its own funky graphql way instead. Maybe this is useful for some custom functionality?	Make it abstract and let it be implemented anywhere that needs it (same deal as save() above)
  getEditForm()	Makes the form for that admin section. Only seems to be used by CMSMain (and its subclasses) and CMSProfileController - which both make sense, they only manage a single class right now. Interestingly not used by ReportAdmin or SiteConfigLeftAndMain	make abstract - it's not used widely enough to be useful at this level, but could be implemented elsewhere. That separate implementation will be discussed further down (same deal as save() above)
  batchactions()	Seems to only be used in sitetree. Perhaps a future enhancement would be to make these somehow usable in a flexible way so we can have colymba/GridFieldBulkEditingTools and asset admin batch actions using this logic, but we can look at it again if/when we do that. AssetAdmin does register a batch action with the CMSBatchActionHandler but that seems to be dead code.	Move to CMSMain directly (not `HierarchicalModelAdmin - making batch actions generic and moving it to a higher superclass can be a follow-on work later on)

Remove the tree_class config in favour of an abstract getModelClass() method, which returns whatever the current class is. Some LeftAndMain subclasses will always return the same class name (e.g. ReportAdmin would always return Report::class) but some would return an appropriate class based on the request (like ModelAdmin::getModelClass).

Additional recommendations

There's a client_debugging config which is only used to enable something in redux - we should either:

1. Use this more, and have more debug output from js, or
2. Remove the config and either remove whatever it's doing with redux (if it's not worth keeping) or just always do whatever it's doing in redux when in dev mode (if it is worth keeping).

Specific methods

• Delete printable()
• Delete getNewItem() and just use injector or the ::create() method directly. It's an anti-pattern to make a new item with a specific ID.
• Delete LinkPreview() and its usage in templates and any JS/CSS related to it - seems to be a legacy way to get the preview URL into the preview panel
• Delete SwitchView() - isn't used, looks legacy
• Delete SiteConfig() method - silverstripe/siteconfig shouldn't be a dependency of admin, and you can get the current siteconfig both via PHP and in templates with methods provided by that module directly.
• Delete ApplicationLink() - not used
• Remove AddForm from allowed_actions - there's no corresponding method
• Move everything related to batch actions into CMSMain since that's the only place that uses it
• Regarding currentPageID() and its associated code, rename to currentRecordID() (or probably even currentRecord() and keep a reference to the actual DataObject object rather than just the ID) and either:
  1. Find a way to make it work with all admins (e.g. modeladmin gridfield edit forms as well) - and probably turn it into a stack rather than a single value
     • Means there's an easy way to get the current record being edited, and any records that needed to be accessed to drill down to it (aka its "parents"), but is meaningless if we don't use it wherever we can ourselves
  2. Move this to a class that can be a superclass of SiteConfigLeftAndMain for dealing with only a single record (more details about that lower down)
• Delete LeftAndMain_SearchFilter - it's only implemented by CMSSiteTreeFilter and even then only for a single form field. It was probably intended to be a way to customise SearchContext but doesn't seem to be used in the end.
• Move getSearchFilter to CMSMain or even into SiteTree since it's related to SiteTree's searchable fields which should work the same in a gridfield as they do in this controller

New abstract AdminRouteController class

Or, maybe it's not abstract and this is all added to the existing AdminRootController. Either way is fine.

This class would handle permissions and anything you're likely to want in a generic controller that has a /admin/* route.

It becomes the superclass for LeftAndMain. Any time someone wants to check if the current controller is a CMS controller they can simple check if it's an instance of AdminRouteController.

AdminRootController::rules() and AdminRootController::add_rule_for_controller() need to be updated to include routing for all non-abstract subclasses of AdminRouteController.

Methods that go in here

• jsonSuccess()
• jsonError()
• canView()
• getRequiredPermissions()
• Some parts of init()
  • set locale and everything before that
  • permissions stuff
  • set stage (if Versioned module exists)
  • may need setting theme in case the custom controller wants to render out a template
  • probably don't want the requirements js stuff
  • no session ping
  • no combined config
  • no WYSIWYG stuff
• Check if handleRequest() validation exception catching makes sense here - if so, keep that part of this method here, if not leave that for a subclass.
  • Find out what the X-* headers are used for and include them in this class if it'd be useful for generic purposes
• redirect()
• Link()
• providePermissions()

New abstract FormSchemaAdmin class, extends AdminRouteController

This becomes a superclass for classes like LinkFieldController that are intended for use with form schema modal forms.

Note that there's additional formschema changes below - see ModalController.

Methods that go in here

• getFormSchema()
• setFormSchema()
• schema()
• methodSchema()
• getSchemaRequested() (if it's even needed)
• getSchemaResponse()
• Anything from handleRequest() that makes sense here (e.g. the ValidationException handling probably fits here)
• save() if it's needed for formschema modal forms to work
• delete() if it's needed or usable for formschema modal forms
• EditForm() and/or getEditForm() if they're used by formschema forms
• EmptyForm() if it's still needed by delete(), though ideally delete should close the modal instead of returning an empty form

[GuySartorelli]

CMSMain

This section assumes recommendations for LeftAndMain above are done.

tl;dr

CMSMain could be made more generic with the following changes:

• new abstract HierarchicalModelAdmin class
  • Becomes the superclass of CMSMain
  • Replace all hardcoded references to SiteTree with calls to getModelClass() (see POC which calls it getTreeClass())
  • Allows for hierarchy classes like TaxonomyTerm to be managed in a tree structure
• Current subclasses of CMSMain turn into subclasses of AdminRouteController
  • Either become subclasses of AdminRouteController or are folded back into HierarchyModelAdmin
  • Developers can make a subclass of HierarchicalModelAdmin and set whatever model class they want

Recommended changes in depth

The main this is to replace hardcoded SiteTree references with calls to getModelClass(), which can be implemented in each subclass to the specific class handled by that admin. CMSMain would obviously return SiteTree from that method.

Most of the code in CMSMain should be moved to a new abstract HierarchicalModelAdmin class, which CMSMain should be a subclass of.

Subclasses of CMSMain

Currently CMSMain has the following subclasses:

• CMSPagesController
• CMSPageEditController
• CMSPageSettingsController
• CMSPageHistoryViewerController
• CMSPageAddController

There seems to be no reason for these to subclass CMSMain other than convenience at the time it was originally implemented. These should all either:

1. become direct subclasses of AdminRouteController and keep their own built-in routing
   • Makes it harder to override their functionality in subclasses of HierarchicalModelAdmin
   • Need an instance of HierarchicalModelAdmin passed in so they can call methods on it
2. become actions directly on HierarchicalModelAdmin itself
   • This is probably the simplest solution - possibly also the cleanest
   • Means there's extra stuff on HierarchicalModelAdmin that doesn't necessarily have to be there
   • Makes it easier to override their functionality in subclasses of HierarchicalModelAdmin

Out of those options I recommend 2. It's simpler and it's easier to customise in projects.

Additional recommendations/changes

1. Add checks to ensure that the model class is hierarchical (has the Hierarchy extension applied) and has the CMSEditLink() method.
2. In a few places it'd pay to add early returns when Versioned isn't applied to the record
3. There's some methods on SiteTree that the POC had to reimplement to get things working. This is things like tree title vs menu title vs title, icon, status flags, tree node classes, breadcrumbs for the list view, etc. These methods should be kept implemented on HierarchicalModelAdmin and call invokeWithExtensions() to allow the model to update these either directly or via extensions.
4. Breadcrumbs for the list view should
5. There's a lot of wording to adjust that assumes CMSMain is for pages. These should use methods like i18n_plural_name() to get the appropriate name for the given model class where possible.
6. The way search works for CMSMain is just completely incompatible with non-SiteTree classes. It has a hardcoded form, and presumably a different code path for the actual search functionality. We should marry up the way GridFieldFilterHeader and the filter/search in here work.
   • Maybe have a new controller or trait for CMS search. This gets the form using similar logic to the current GridFieldFilterHeader logic, and GridFieldFilterHeader would be modified to use the controller/trait.
   • Current fields in CMSMain::getSearchForm() get moved into SiteTree::searchableFields()
   • SiteTree gets a custom SearchContext subclass which leverages CMSSiteTreeFilter for the field that uses it (if that's necessary - I haven't looked too deep into the search code path for sitetree)
7. I haven't really looked at all at the history tab - it might need some work. MVP would be to just not show that for anything other than SiteTree (i.e. keep its stuff specific to CMSMain rather than allow it to be used by HierarchicalModelAdmin and have refactoring it to work generically as a separate follow-up effort.
8. A whole bunch of new unit and behat tests will need to be written
9. The root of the site tree is the site name, which makes sense for pages but doesn't really make sense for much of anything else. Might be worth just removing that root, or making it configurable, or something.

Stretch goal

Make this handle multiple classes in separate tabs, like ModelAdmin does via its managed_models config. Then you could still have a single "Taxonomies" CMS section, but it could handle both TaxonomyTerm and TaxonomyType with this hierarchical style.

Note that TaxonomyType isn't hierarchical - so either we'd need to change that, or better yet AAALLL of this logic could just get bundled together with ModelAdmin, so you can have for example one tab for managing TaxonomyTerm which uses the tree structure, and one tab for managing TaxonomyType which uses the normal ModelAdmin grid structure.

This is very much a follow-on massive chunk of work which would have to happen after the main refactoring is done.

[GuySartorelli]

ModelAdmin

• Remove SearchForm from allowed_actions - this is handled by gridfield, not modeladmin
• Make clearer rules about when to use modelTab and when to use modelClass - currently it's not super clear when to use which, which can cause problems
• ImportForm() should be moved out of here and handled by the gridfield component instead.
  • The importer should be accessible either via the DataObject class, or passed into the component with a setter method
  • Probably need a way to update/replace the form (see SecurityAdmin)
  • import() should be moved along with it
• Should provide a way to give i18n replacements for tab titles without having to override getManagedModels() (could theoretically do this in CMS 5, but would be easier in 6)

SilverStripe\Admin\ModalController

This class is just a bad way to handle registering routes for formschema forms. We should refactor this to have FormFactory classes registered to it dynamically via yaml config, and make this a subclass of the new FormSchemaController class.

This class can then be used everywhere that currently requires a singleton of LeftAndMain to get the routes for these special form schemas.

This would be trivial.

RemoteFileModalExtension in silverstripe/asset-admin and InternalLinkModalExtension in silverstripe/cms would be replaced with yaml config.

Benefits

• Should enable easy setup for formschema forms using FormFactory

SilverStripe\CMS\Controllers\RootURLController

This class could be made more generic and moved to framework

This would be trivial.

Benefits

• Enables any model to be automatically routed as "home" even without the CMS module

Dependencies

• Requires ModelAsController to also be made generic and moved to framework

SilverStripe\CMS\Controllers\ContentController

This class could be made more generic and moved to framework.

This would be fairly easy:

• Remove methods that assume a hierarchy (e.g. ChildrenOf() which is probably barely ever used anyway)
• Remove Page() method, which is probably barely ever used and would be easy to either implement into SiteTree or for projects to implement if they need it
• Needs a new config property or method or something to tell it what class it's supposed to handle
• Need to move URLSegment somewhere more generic (maybe a PageDataObject which has anything related to URLSegment added to it and is either an extension applied to or a superclass for SiteTree)
• Need to reconsider how Menu() and getMenu() methods are done
• Remove SiteConfig() method while we're at it - that's provided in a global template provider elsewhere anyway.
• Ideally change getViewer() to always (or conditionally based on config) use the base Page template, falling back on appropriately namespaced templates even for non-SiteTree

Benefits

• Enables more easily treating non-SiteTree models as pages
• There are already some places in this class that don't assume the record is a SiteTree so it cleans up that ambiguity

Dependencies

• Requires ModelAsController to also be made generic and moved to framework
• Requires refactoring OldPageRedirector to either be more generic, or more feasibly to just not be used when that extension's owner isn't looking at site trees.

SilverStripe\CMS\Controllers\ModelAsController

This class could be made more generic and moved to framework

This would be trivial:

• requires setting a default model class to fall back to (CMS could set it to SiteTree)
• requires a type (e.g. interface) for getControllerName() or else a hasMethod() check before calling that method.

Benefits

• Enables RootURLController to be more generic (see above)

Dependencies

• Requires refactoring OldPageRedirector to either be more generic, or more feasibly to just not be used when that extension's owner isn't looking at site trees.

[GuySartorelli]

New superclass for single-record LeftAndMain admins

CMSProfileController and SiteConfigLeftAndMain are basically identical. It's also not terribly uncommon to want to make your own LeftAndMain that handles a single record.

We should implement an abstract SingleRecordModelAdmin class which allows easily setting up an admin section which only edits a single record at a time.

• Implement all appropriate shared methods, such as getEditForm()
• CMSProfileController and SiteConfigLeftAndMain need to only define their appropriate config (url_segment etc), implement getModelClass(), and set the current record in init().
• SiteConfigLeftAndMain::save_siteconfig() and CMSProfileController::save() do nothing special - a save() method in the superclass can handle that in a nice generic way. Override as needed but defer to parent::save() for most of the logic.
• Breadcrumbs() methods in both don't actually do anything - just let them rely on the parent class Breadcrumbs() method instead.

Possible use case with GridField

This new controller could be a superclass for GridFieldDetailForm_ItemRequest, so that as much as possible of the logic can be shared between gridfield forms and LeftAndMain.

We would need to have an exception in CMSMenu that doesn't add records to the left menu if they're an instance of GridFieldDetailForm_ItemRequest.

I'm not sure how this would work with routing, exactly. It's possible it wouldn't work with routing, and instead this logic should be shared via a trait.

[GuySartorelli]

Aside from all that, the following are currently handled differently between GridField (i.e. in ModelAdmin) and CMSMain:

• search/filter
• a large amount of javascript/php logic, e.g. form submission, toasts
• Template duplication

In particular, removing CMSTabSet.ss and making gridfield work with TabSet.ss could help to reduce a lot of that duplication.

[GuySartorelli]

High level tl;dr

Restructuring

The actual names of classes don't matter here - I just needed a way to refer to everything. We can change those during implementation.

Each of these numbered points essentially becomes its own card, though the CMSMain card could be broken up into 2 (one for doing basically what the POC does, and another for abstracting stuff out into its own generic class).
The work for the controllers that could move to framework could probably be done as three cards, too.

1. LeftAndMain becomes:
   • abstract AdminRouteController (routing/permissions for /admin/* controllers)
   • abstract FormSchemaAdmin (everything you need in a superclass for form schema modal controllers)
   • abstract LeftAndMain (everything that doesn't go into the other two)
2. CMSMain becomes:
   • abstract HierarchicalModelAdmin (generic tree-structure admin section for hierarchical data models)
   • CMSMain (subclass of HierarchicalModelAdmin specifically for SiteTree)
   • The existing subclasses of CMSMain get folded back into HierarchicalModelAdmin
3. CMSProfileController and SiteConfigLeftAndMain get a new superclass, which lives between them and LeftAndMain
4. ModalController gets a makeover and allows for easy registration of FormFactory modal forms
5. RootURLController, ContentController, and ModelAsController are made more generic and maybe moved to framework.
6. Minor changes to ModelAdmin

Benefits

1. Reduced friction creating general controllers that route in /admin/*
2. Reduced friction creating form schema controllers
3. Ability to manage any hierarchical data in a hierarchical way (e.g. you wouldn't have to drill through all of the TaxonomyTerm record edit forms to find the deeply nested one you want to touch)
4. Reduced friction creating admin sections that edit a single record at a time
5. Reduced friction creating modals for forms
6. Reduced friction using framework without admin or cms
7. Unlocks potential for a future super-modeladmin (see below)

Future AC breaking changes that could be done afterward

1. Make HierarchicalModelAdmin handle multiple classes, similar to ModelAdmin, or
2. Blend HierarchicalModelAdmin and ModelAdmin together so we can have a single admin section with one model in a tree structure and another in a gridfield/list view

[GuySartorelli]

Created new epic with issues linked to it for all of the above refactorings.