-
Notifications
You must be signed in to change notification settings - Fork 92
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
NEW SingleRecordAdmin class for editing one record at a time #1842
base: 3
Are you sure you want to change the base?
NEW SingleRecordAdmin class for editing one record at a time #1842
Conversation
private static $tree_class = Member::class; | ||
private static $model_class = Member::class; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See change in LeftAndMain
private static $tree_class = null; | ||
private static $model_class = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most subclasses of LeftAndMain
which use this config are not trees - and even with CMSMain
which is the only tree structure, this is getting refactored to be more generic anyway.
Makes sense to update this to be named something that all the classes using it can respect.
if ($record->hasMethod('getCMSCompositeValidator')) { | ||
// As of framework v4.7, a CompositeValidator is always available form a DataObject, but it may be | ||
// empty (which is fine) | ||
$form->setValidator($record->getCMSCompositeValidator()); | ||
} elseif ($record->hasMethod('getCMSValidator')) { | ||
// BC support for framework < v4.7 | ||
$validator = $record->getCMSValidator(); | ||
|
||
// The clientside (mainly LeftAndMain*.js) rely on ajax responses | ||
// which can be evaluated as javascript, hence we need | ||
// to override any global changes to the validation handler. | ||
if ($validator) { | ||
$form->setValidator($validator); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this boils down to $form->setValidator($record->getCMSCompositeValidator())
, because all DataObject
classes have that method, and $record
will always be a DataObject
.
} else { | ||
$form->unsetValidator(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That method just sets it null anyway so even if somehow getCMSCompositeValidator()
returned null (which it can't) the result would be the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anything removed from this class was either not necessary (likely related to legacy CMS 3 or older code that no longer exists) or is already being done in one of the superclasses.
{ | ||
parent::init(); | ||
if (!$this->getResponse()->isRedirect()) { | ||
$this->setCurrentPageID(Security::getCurrentUser()->ID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving this into init
instead of getEditForm
so it takes affect for all actions on this controller (including any that might be added through extensions).
Skipping for redirects because at that point we're not doing anything in this controller anyway.
private static $tree_class = Group::class; | ||
private static $model_class = Group::class; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I strongly suspect this config doesn't actually do anything for this class, but checking that is out of scope for this PR. I'm just updating it to the config that LeftAndMain
expects.
/** | ||
* Determines if there should be a single record in the database that this admin edits. | ||
* | ||
* If this is not true, you need to provide a mechanism to tell the form which record it should be editing. | ||
* This could be an action (e.g. edit/$ID), or could be based on something about the current member, etc. | ||
*/ | ||
private static bool $only_one_record = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for SiteConfig
style functionality - but since CMSProfileController
is explicitly for the current member and not the one member in the database, it needs to be configurable.
/** | ||
* If no record exists, allow a new one to be created | ||
*/ | ||
private static bool $allow_new_record = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For almost all controllers, we'll want to be able to create a record if there isn't one. CMSProfileController
is the exception. Even with SiteConfig
, if the record in the DB gets somehow deleted, you'll want to be able to create a new one.
Note that there may be some methods still on
LeftAndMain
that will be migrated into this class, such assave()
which isn't used byCMSMain
orModelAdmin
- but moving those will be handled by #1763 once the bulk of the other refactoring has been completed. This is to reduce the chance of regressions in areas that still need to be refactored.TODO
SiteConfigLeftAndMain::save_siteconfig()
andLeftAndMain.tree_config
Issue
LeftAndMain
into its own class #1764