Skip to content
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.

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

POC DO NOT MERGE - Make CMSMain work with other models #2939

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented May 9, 2024

This is a proof of concept as part of #2933 to see what's needed to make CMSMain work with classes that aren't SiteTree.

To play around with this, install silverstripe/taxonomy and add the following YAML config to your project:

SilverStripe\Taxonomy\TaxonomyTerm:
  cms_edit_owner: SilverStripe\CMS\Controllers\CMSMain
  extensions:
    - SilverStripe\Admin\CMSEditLinkExtension

SilverStripe\CMS\Controllers\CMSMain:
  tree_class: SilverStripe\Taxonomy\TaxonomyTerm

Your /admin/pages will now be working with taxonomy terms, rather than pages.

Theoretically this could work with any DataObject class which has CMSEditLinkExtension and Hierarchy applied.

What would still need to be done besides this

  1. Refactor CMSPagesController, CMSPageEditController, CMSPageSettingsController, CMSPageHistoryViewerController, and CMSPageAddController so you could have multiple instances of this going at once, e.g. one admin for pages, one for taxonomies, etc - without having to subclass all of these controllers as well.
    • Ideally these wouldn't be subclasses of LeftAndMain at all - either this could all be handled in the main HierarchyModelAdmin class (see below) or these could all be more generic controllers.
    • Either way they need to not be subclasses of CMSMain, but instead get any information they need by calling methods on CMSMain (i.e. have the CMSMain instance passed to their constructor) or having that information passed through to them via setters or via their constructor.
  2. Have this logic all in a superclass called something more generic, like HierarchyModelAdmin (which would belong in silverstripe/admin along with all appropriate JS), and have CMSMain be a subclass of HierarchyModelAdmin with its tree_class set to SiteTree
    • While moving the JS to silverstripe/admin, it'd be worth finding all JS logic that's duplicated and making it so that all works in the same way for ModelAdmin and this new class.
  3. Obviously we'd need to add in appropriate type checking, make sure the tree class is hierarchical and has a cms edit link.
  4. In a few places it'd pay to add early returns when versioned isn't applied to the record
  5. This POC does a lot of lazy work to avoid refactoring - e.g. there's a lot of methods now duplicated between SiteTree and CMSMain. These should go into a new class (probably an extension) that can be applied to any classes that want to live in this admin type. Things like tree title vs menu title vs title, icon, status flags, tree node classes, etc
    • This will end up meaning if you want to add a model to this admin, it must have Hierarchy, CMSPageEditController, and this new extension. But that also means you can just apply 3 extensions to any model and boom, you're ready to go.
    • Some things like the breadcrumbs for list view shouldn't be on the model - just leave that in the controller or fully template-ify it
    • Some things like status flags should probably have default functionality on the controller but use invokeWithExtensions (or just a hasMethod() check) to allow the model to update it
  6. There's a lot of wording to adjust that assumes this is for pages.
  7. Batch actions assume Versioned is applied and probably also have other assumptions that make them not work
  8. The way search works for CMSMain is just completely incompatible with these changes. It has a hardcoded form, and a different code path for the actual search functionality. We should marry up the way GridFieldFilterHeader and the filter/search in here work.
    • Probably have a new controller for CMS search. This gets the form using similar logic to the current GridFieldFilterHeader logic
    • 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)
  9. 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 and handle making it work as a separate effort.
  10. A whole bunch of new unit and behat tests would need to be written
  11. 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.

@GuySartorelli GuySartorelli marked this pull request as draft May 9, 2024 03:23
@GuySartorelli GuySartorelli changed the title POC DO NOT MERGE - Make CMS Main work with other models POC DO NOT MERGE - Make CMSMain work with other models May 9, 2024
@GuySartorelli GuySartorelli force-pushed the pulls/5/poc-configurable-cmsmain branch from 188d95a to 769136f Compare May 9, 2024 03:40
@GuySartorelli
Copy link
Member Author

Closing - no need to keep it open, the code will still be there even when it's closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant