API Move logic from silverstripe/cms into central place #11460

GuySartorelli · 2024-11-10T22:39:33Z

This logic is used by CMSMain but needs to be callable on any hierarchical model.

In some cases this logic is generic enough it could be on any model or any DataObject.

See the corresponding CMS PR to see where this code came from.

Issue

Don't hardcode SiteTree in CMSMain silverstripe-cms#2947

GuySartorelli · 2024-11-11T02:28:48Z

src/ORM/DataObject.php

+    /**
+     * Get localised description for this class
+     */
+    public function i18n_classDescription(): ?string


I know new method names shouldn't have underscore, but I've kept this one in for two reasons:

This method is being moved from SiteTree::i18n_classDescription() - keeping the same name reduces upgrade pain.

This method name is consistent with the other i18n_* methods on this class.

GuySartorelli · 2024-11-11T02:29:33Z

src/ORM/DataObject.php

-     * @var string
-     * @config
     */
-    private static $singular_name = null;
+    private static ?string $singular_name = null;

    /**
     * Human-readable plural name
-     * @var string
-     * @config
     */
-    private static $plural_name = null;
+    private static ?string $plural_name = null;


Unrelated tidy-up and strong typing for these config

GuySartorelli · 2024-11-11T02:30:23Z

src/ORM/DataObject.php

+    /**
+     * Description of the class.
+     * Unlike most configuration, this is usually used uninherited, meaning it should be defined
+     * on each subclass.
+     *
+     * Used in some areas of the CMS, e.g. when selecting what type of record to create.
+     */
+    private static ?string $class_description = null;


This was the description config on SiteTree.
Renamed this because description is very vague (are we describing the class or the record?) and class_description is very clear in its purpose.

GuySartorelli · 2024-11-11T02:31:36Z

src/ORM/DataObject.php

+     */
+    public function classDescription(): ?string
+    {
+        return static::config()->get('class_description', Config::UNINHERITED);


Intentionally uninherited because the two classes that use this right now (SiteTree and BaseElement) both want the config to be uninherited.

Makes sense - the subclass should have a more specific description so it's clear how it's different from the superclass.

GuySartorelli · 2024-11-11T02:32:17Z

src/ORM/DataObject.php

+     * Also flush the cached results for all relations (has_one, has_many, many_many)
     *
-     * @param boolean $persistent When true will also clear persistent data stored in the Cache system.
+     * @param bool $persistent When true will also clear persistent data stored in the Cache system.
     *                            When false will just clear session-local cached data
-     * @return static $this
     */
-    public function flushCache($persistent = true)
+    public function flushCache(bool $persistent = true): static


Adding a little more description and hardcoding while I'm here. See ModelData::flushCache().

GuySartorelli · 2024-11-11T02:38:05Z

src/ORM/Hierarchy/Hierarchy.php

+        // "Can be root" validation
+        if (!$owner::config()->get('can_be_root') && !$owner->ParentID) {
+            $validationResult->addError(
+                _t(
+                    __CLASS__ . '.TypeOnRootNotAllowed',
+                    'Model type "{type}" is not allowed on the root level',
+                    ['type' => $owner->i18n_singular_name()]
+                ),
+                ValidationResult::TYPE_ERROR,
+                'CAN_BE_ROOT'
+            );
+        }
+
+        // Allowed children validation
+        $parent = $owner->getParent();
+        if ($parent && $parent->exists()) {
+            // No need to check for subclasses or instanceof, as allowedChildren() already
+            // deconstructs any inheritance trees already.
+            $allowed = $parent->allowedChildren();
+            $subject = $owner->hasMethod('getRecordForAllowedChildrenValidation')
+                ? $owner->getRecordForAllowedChildrenValidation()
+                : $owner;
+            if (!in_array($subject->ClassName, $allowed ?? [])) {
+                $validationResult->addError(
+                    _t(
+                        __CLASS__ . '.ChildTypeNotAllowed',
+                        'Model type "{type}" not allowed as child of this parent record',
+                        ['type' => $subject->i18n_singular_name()]
+                    ),
+                    ValidationResult::TYPE_ERROR,
+                    'ALLOWED_CHILDREN'
+                );
+            }
+        }


New validation comes from SiteTree - it's validating against the new can_be_root and allowed_children

GuySartorelli · 2024-11-11T02:39:01Z

src/ORM/Hierarchy/Hierarchy.php

+     * Duplicates each child of this record recursively and returns the top-level duplicate record.
+     * If there is a sort field, new sort values are set for the duplicates to retain their sort order.
+     */
+    public function duplicateWithChildren(): DataObject


CMSMain needs this, so moving it from SiteTree to Hierarchy

GuySartorelli · 2024-11-11T02:39:48Z

src/ORM/Hierarchy/Hierarchy.php

+    protected function extendCanAddChildren()
+    {
+        // Prevent canAddChildren from extending itself
+        return null;
+    }


See Versioned for other cases where we do this. This method allows us to have $extendedCan() above without getting into an infinite recursion loop.

GuySartorelli · 2024-11-11T02:40:15Z

src/ORM/Hierarchy/Hierarchy.php

+    protected function canCreate(?Member $member, array $context): ?bool
+    {
+        // Parent is added to context through CMSMain
+        // Note that not having a parent doesn't necessarily mean this record is being
+        // created at the root, so we can't check against can_be_root here.
+        $parent = isset($context['Parent']) ? $context['Parent'] : null;
+        $parentInHierarchy = ($parent && is_a($parent, $this->getHierarchyBaseClass()));
+        if ($parentInHierarchy && !in_array(get_class($this->getOwner()), $parent->allowedChildren())) {
+            return false;
+        }
+        if ($parent?->exists() && $parentInHierarchy && !$parent->canAddChildren($member)) {
+            return false;
+        }
+        return null;
+    }


This logic is moved here from SiteTree::canCreate() and checks against allowed_children config.

GuySartorelli · 2024-11-11T02:48:26Z

src/ORM/Hierarchy/Hierarchy.php

+            $subject = $owner->hasMethod('getRecordForAllowedChildrenValidation')
+                ? $owner->getRecordForAllowedChildrenValidation()
+                : $owner;


In SiteTree there was a hardcoded reference to VirtualPage and one of its relations here. Instead, we check for the presence of a new method which allowed VirtualPage to say "use this instead of me"

It's not very tidy code, though I also can't think of a better way to do it given that Hierarchy is an extension

emteknetnz · 2024-11-14T22:49:20Z

src/Model/ModelData.php

+     * Example (with optional title attribute):
+     *   "deletedonlive" => ['text' => "Deleted", 'title' => 'This page has been deleted']
+     */
+    public function getStatusFlags(bool $cached = true): array


This seems ... kind of out of scope?

I don't like the idea of adding generic functionality that we might use in the future, but we're not actually planning on implementing straight away

Also the method name for this is wrong for a generic method as 'status flags' is clearly something for versioning

It feels like this method should just live on the Versioned extension instead of on ModelData?

emteknetnz · 2024-11-14T22:53:05Z

src/ORM/Hierarchy/Hierarchy.php

+     * The name of the dedicated sort field, if there is one.
+     * Note that this does not automatically apply to default_sort,
+     * you still have to set that configuration separately.


Suggested change

* The name of the dedicated sort field, if there is one.

* Note that this does not automatically apply to default_sort,

* you still have to set that configuration separately.

* The name of the dedicated sort field, set to null if there is not one.

* Does not affect default_sort which needs to be configured separately.

"set to null" sounds like we're instructing developers to set it to null, rather than what I think you intend which is to indicate it will be null if there isn't one.

That feels like it's implied already (both through the "if there is one" language and the fact that it's literally set to null here) but I'll add an explicit note of it to this comment to avoid ping pong.

emteknetnz · 2024-11-14T23:13:47Z

src/ORM/Hierarchy/Hierarchy.php

+            $subject = $owner->hasMethod('getRecordForAllowedChildrenValidation')
+                ? $owner->getRecordForAllowedChildrenValidation()
+                : $owner;


It's not very tidy code, though I also can't think of a better way to do it given that Hierarchy is an extension

This logic is used by CMSMain but needs to be callable on any hierarchical model. In some cases this logic is generic enough it could be on any model or any DataObject

GuySartorelli marked this pull request as draft November 10, 2024 22:39

This was referenced Nov 10, 2024

Don't hardcode SiteTree in CMSMain silverstripe/silverstripe-cms#2947

Open

API Move some code to framework to be reusable silverstripe/silverstripe-cms#3022

Open

GuySartorelli force-pushed the pulls/6/move-from-cms branch from 2f4001e to b57b8e2 Compare November 10, 2024 22:44

GuySartorelli force-pushed the pulls/6/move-from-cms branch 2 times, most recently from 2a11c1e to ade38d7 Compare November 11, 2024 03:20

GuySartorelli mentioned this pull request Nov 11, 2024

API Deprecate and update code in preparation for CMS 6 silverstripe/silverstripe-cms#3023

Merged

GuySartorelli commented Nov 11, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/6/move-from-cms branch 3 times, most recently from 990e4bc to 43bdde5 Compare November 14, 2024 04:31

GuySartorelli marked this pull request as ready for review November 14, 2024 04:40

GuySartorelli force-pushed the pulls/6/move-from-cms branch from 43bdde5 to 75ac6cc Compare November 14, 2024 20:10

emteknetnz requested changes Nov 14, 2024

View reviewed changes

API Move logic from silverstripe/cms into central place

113a2e4

This logic is used by CMSMain but needs to be callable on any hierarchical model. In some cases this logic is generic enough it could be on any model or any DataObject

GuySartorelli force-pushed the pulls/6/move-from-cms branch from 75ac6cc to 113a2e4 Compare November 14, 2024 23:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Move logic from silverstripe/cms into central place #11460

API Move logic from silverstripe/cms into central place #11460

GuySartorelli commented Nov 10, 2024 •

edited

Loading

GuySartorelli Nov 11, 2024

GuySartorelli Nov 11, 2024

GuySartorelli Nov 11, 2024

GuySartorelli Nov 11, 2024

GuySartorelli Nov 11, 2024

GuySartorelli Nov 11, 2024

GuySartorelli Nov 11, 2024

GuySartorelli Nov 11, 2024

GuySartorelli Nov 11, 2024

GuySartorelli Nov 11, 2024

emteknetnz Nov 14, 2024

GuySartorelli Nov 14, 2024

emteknetnz Nov 14, 2024 •

edited

Loading

emteknetnz Nov 14, 2024

GuySartorelli Nov 14, 2024 •

edited

Loading

emteknetnz Nov 14, 2024

API Move logic from silverstripe/cms into central place #11460

Are you sure you want to change the base?

API Move logic from silverstripe/cms into central place #11460

Conversation

GuySartorelli commented Nov 10, 2024 • edited Loading

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

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

emteknetnz Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Nov 10, 2024 •

edited

Loading

emteknetnz Nov 14, 2024 •

edited

Loading

GuySartorelli Nov 14, 2024 •

edited

Loading