NEW SingleRecordAdmin class for editing one record at a time #1842

GuySartorelli · 2024-10-25T00:25:00Z

Note that there may be some methods still on LeftAndMain that will be migrated into this class, such as save() which isn't used by CMSMain or ModelAdmin - 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

CMS 5 PR to deprecate SiteConfigLeftAndMain::save_siteconfig() and LeftAndMain.tree_config

Issue

Refactor logic for sections that deal with only a single record out of LeftAndMain into its own class #1764

GuySartorelli · 2024-10-25T00:38:50Z

code/CMSProfileController.php

-    private static $tree_class = Member::class;
+    private static $model_class = Member::class;


See change in LeftAndMain

code/LeftAndMain.php

GuySartorelli · 2024-10-25T00:41:11Z

code/LeftAndMain.php

-        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);
-            }


All of this boils down to $form->setValidator($record->getCMSCompositeValidator()), because all DataObject classes have that method, and $record will always be a DataObject.

GuySartorelli · 2024-10-25T00:41:34Z

code/LeftAndMain.php

-        } else {
-            $form->unsetValidator();
-        }


That method just sets it null anyway so even if somehow getCMSCompositeValidator() returned null (which it can't) the result would be the same.

GuySartorelli · 2024-10-25T00:42:27Z

code/CMSProfileController.php

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.

GuySartorelli · 2024-10-25T00:43:19Z

code/CMSProfileController.php

+    {
+        parent::init();
+        if (!$this->getResponse()->isRedirect()) {
+            $this->setCurrentPageID(Security::getCurrentUser()->ID);


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.

GuySartorelli · 2024-10-25T00:44:14Z

code/SecurityAdmin.php

-    private static $tree_class = Group::class;
+    private static $model_class = Group::class;


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.

GuySartorelli · 2024-10-25T00:45:06Z

code/SingleRecordAdmin.php

+    /**
+     * 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;


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.

code/SingleRecordAdmin.php

code/LeftAndMain.php

emteknetnz · 2024-10-30T05:19:08Z

code/SingleRecordAdmin.php

+     * 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;


Suggested change

private static bool $only_one_record = true;

private static bool $single_record = true;

$only_one_record sounds weird to me, almost implying that it's a bad thing :-)

Feel free to suggest an alternative name.

$single_record

How about only_single_record? Or restrict_to_single_record? I want the name to be fairly clear that it's saying "there should only ever be one record of this type" when set to true.

Or maybe dynamic_current_record? As in "this will dynamically fetch the current record without you telling it what the ID is"?

$restrict_to_single_record sounds good

code/SingleRecordAdmin.php

emteknetnz · 2024-10-30T05:29:16Z

code/SingleRecordAdmin.php

+    /**
+     * 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.


Suggested change

* If this is not true, you need to provide a mechanism to tell the form which record it should be editing.

* If false, you need to provide a mechanism to tell the form, usually via getEditForm(), which record it should be editing.

Will make the change, but won't add "usually via getEditForm()" because the only scenario we've actually implemented doesn't do it that way.

emteknetnz · 2024-10-30T05:41:35Z

code/CMSProfileController.php

-        }
-
+        // Get the current member locale so we can see if it changes
+        $member = Member::get()->byID($data['ID']);


Suggested change

$member = Member::get()->byID($data['ID']);

$member = Member::get()->byID($id);

emteknetnz · 2024-10-30T05:44:49Z

code/CMSProfileController.php

-            $this->httpError(404);
+        // Make sure the ID is a positive number
+        $id = $data['ID'] ?? null;
+        if (!is_numeric($id) || $id < 1) {


Should validate that it's an int (or string int), is_numeric() will allow a float

emteknetnz · 2024-10-30T05:51:02Z

code/SingleRecordAdmin.php

+        return parent::getRecord($id);
+    }
+
+    protected function getSingleRecord(): ?DataObject


Suggested change

protected function getSingleRecord(): ?DataObject

private function getSingleRecord(): ?DataObject

If we're providing only_one_record and allow_new_record and implementing logic to do stuff based on how they're set, why would anyone need to override this in a subclass? Things should be private unless there's an immediate need to make it protected.

Please see the docs where overriding this method is explicitly mentioned as a way to provide more complex behaviour for getting this record.

emteknetnz · 2024-10-30T05:53:33Z

code/SingleRecordAdmin.php

+        if (!$id) {
+            return $this->getSingleRecord();
+        }
+        return parent::getRecord($id);


What's the scenario where $id is truthy? Seems like we should be throwing an exception here instead?

$id is truthy if an ID is passed in. So we either want the "single record" based on the configuration (where a null or 0 is passed in), or we want the specified record if a specific ID is passed in.

GuySartorelli · 2024-10-30T06:51:52Z

I've responded to the bits that are relevant for the CMS 5 PRs. I'll make the requested changes that are agreed upon after the CMS 5 PRs have been merged and I can rebase on top of them.

GuySartorelli marked this pull request as draft October 25, 2024 00:25

GuySartorelli commented Oct 25, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/3/singlerecordadmin branch 2 times, most recently from bdb2aeb to 1f41d29 Compare October 30, 2024 03:00

emteknetnz requested changes Oct 30, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/3/singlerecordadmin branch 3 times, most recently from 880a411 to eddc85e Compare November 1, 2024 01:33

GuySartorelli marked this pull request as ready for review November 1, 2024 01:33

NEW SingleRecordAdmin class for editing one record at a time

23d0e89

GuySartorelli force-pushed the pulls/3/singlerecordadmin branch from eddc85e to 23d0e89 Compare November 6, 2024 01:29

emteknetnz approved these changes Nov 6, 2024

View reviewed changes

emteknetnz merged commit f7ff272 into silverstripe:3 Nov 6, 2024
9 of 11 checks passed

emteknetnz deleted the pulls/3/singlerecordadmin branch November 6, 2024 04:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW SingleRecordAdmin class for editing one record at a time #1842

NEW SingleRecordAdmin class for editing one record at a time #1842

GuySartorelli commented Oct 25, 2024 •

edited

Loading

GuySartorelli Oct 25, 2024

GuySartorelli Oct 25, 2024

GuySartorelli Oct 25, 2024

GuySartorelli Oct 25, 2024

GuySartorelli Oct 25, 2024

GuySartorelli Oct 25, 2024

GuySartorelli Oct 25, 2024

emteknetnz Oct 30, 2024

GuySartorelli Oct 30, 2024 •

edited

Loading

emteknetnz Oct 30, 2024

GuySartorelli Oct 30, 2024 •

edited

Loading

emteknetnz Oct 30, 2024

GuySartorelli Nov 1, 2024

emteknetnz Oct 30, 2024

GuySartorelli Oct 30, 2024

GuySartorelli Nov 1, 2024

emteknetnz Oct 30, 2024

GuySartorelli Nov 1, 2024

emteknetnz Oct 30, 2024

GuySartorelli Nov 1, 2024

emteknetnz Oct 30, 2024

GuySartorelli Nov 1, 2024

emteknetnz Oct 30, 2024

GuySartorelli Nov 1, 2024 •

edited

Loading

GuySartorelli commented Oct 30, 2024 •

edited

Loading

		private static $tree_class = Member::class;
		private static $model_class = Member::class;

		private static $tree_class = Group::class;
		private static $model_class = Group::class;

	private static bool $only_one_record = true;
	private static bool $single_record = true;

	* If this is not true, you need to provide a mechanism to tell the form which record it should be editing.
	* If false, you need to provide a mechanism to tell the form, usually via getEditForm(), which record it should be editing.

	$member = Member::get()->byID($data['ID']);
	$member = Member::get()->byID($id);

	protected function getSingleRecord(): ?DataObject
	private function getSingleRecord(): ?DataObject

NEW SingleRecordAdmin class for editing one record at a time #1842

NEW SingleRecordAdmin class for editing one record at a time #1842

Conversation

GuySartorelli commented Oct 25, 2024 • edited Loading

TODO

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

Choose a reason for hiding this comment

GuySartorelli Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Oct 30, 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

GuySartorelli Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

GuySartorelli commented Oct 30, 2024 • edited Loading

GuySartorelli commented Oct 25, 2024 •

edited

Loading

GuySartorelli Oct 30, 2024 •

edited

Loading

GuySartorelli Oct 30, 2024 •

edited

Loading

GuySartorelli Nov 1, 2024 •

edited

Loading

GuySartorelli commented Oct 30, 2024 •

edited

Loading