-
Notifications
You must be signed in to change notification settings - Fork 333
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
Don't hardcode SiteTree
in CMSMain
#2947
Comments
This was referenced Oct 16, 2024
Draft
Draft
Draft
Draft
Draft
Draft
This was referenced Nov 7, 2024
Draft
This was referenced Nov 11, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
CMSMain
has atree_class
configuration property which is sometimes referenced - but sometimesSiteTree
is explicitly hardcoded.The hardcoded references and the assumptions about using
SiteTree
should be removed.Benefits
SiteTree
so it becomes just another subclass ofDataObject
rather than having its own code paths in adminRelated
Notes
SiteTree
methods/properties more generic, such as:CMSMain
that calls that method if it exists, but falls back on other things - e.g. getMenuTitle
if there is one, but fall back onTitle
SiteTree
intoCMSMain
, and invoking an extension hook on the record (probably withinvokeWithExtensions()
) to allow updating the valueAcceptance criteria
getModelClass()
method is implemented, which returns the value stored in thetree_class
configuration propertySiteTree
orPage
use thegetModelClass()
method instead (including subclasses ofCMSMain
)Versioned
extensionCMSMain
, one of its subclasses, or a template tries to call a method or fetch a property that is explicitly implemented onSiteTree
, this is handled in a generic wayDataObject
class or theHierarchy
extension, throw clear exceptions if they're missing. One example (and probably the only case) isCMSEditLink
.init()
), a clear exception is thrown if the model class doesn't meet requirements to be used inCMSMain
(i.e. doesn't have theHierarchy
extension applied, or is missing a method/property that is explicitly required forCMSMain
to work)SiteTree
is being used are updated to pull from whatever model is used.CMSMain
works withTaxonomyTerm
(when that class has theCMSEditLinkExtension
extension applied - see config in the description of the POC PR)TO DECIDE
Decision for the above: Leave that as is for now. Part of #2951 will include keeping the site name being returned from CMSMain while the generic abstract class returns a generic value.
PRs
Quick cleanup PR:
This PR removes the
provideI18nEntities()
entry for a localisation key that was just always in the wrong place. More details are in the PR itself.It doesn't block anything so even if you have questions about this PR, please also review any other PRs below that are ready for review.
Moving Code Around PRs:
There's a bunch of stuff that
CMSMain
relies on which is currently only onSiteTree
. This set of PRs moves most of that stuff around so that it can be reliably called on anyDataObject
class which has theHierarchy
extension, which is the criteria for models to be used inCMSMain
.You can see these changes in context in the "Main PRs" section
CMS 5 PRs
Reassign to Guy after merging these so they can rebase the CMS 6 PRs on top
CMS 6 PRs
Kitchen Sink CI
Reassign to Guy once these PRs are merged so they can rebase the main PRs and get those ready for the review cycle.
Main PRs:
CMS 5 PRs
TBD
Reassign to Guy after merging these so they can rebase the CMS 6 PRs on top
CMS 6 PRs
The text was updated successfully, but these errors were encountered: