NEW Linkfield ownership #101

emteknetnz · 2023-10-09T09:04:28Z

Issue #5

Also implements #12

In peer review because looking for initial feedback - have I overlooked something really obvious?

This is a draft PR that's just testing out the idea of using a LinkArea to manage the has_many relationships similar to ElementalArea in elemental. Advantage of using the extra layer is that it allows you to effectively add multiple "has_many" relations of Link::class and have them correctly assigned to each of the has_many relations.

Documentation would instruct you to add you Links and LinkArea's to $has_one + $owns + $cascade_deletes + $cascase_duplicates

A Page in a project would look something like this if it had two Link's and two LinkArea's

class Page extends SiteTree
{
    private static array $has_one = [
        'MyLinkOne' => Link::class,
        'MyLinkTwo' => Link::class,
        'MyLinkAreaOne' => LinkArea::class,
        'MyLinkAreaTwo' => LinkArea::class,
    ];

    private static array $owns = [
        'MyLinkOne',
        'MyLinkTwo',
        'MyLinkAreaOne',
        'MyLinkAreaTwo',
    ];

    private static array $cascade_deletes = [
        'MyLinkOne',
        'MyLinkTwo',
        'MyLinkAreaOne',
        'MyLinkAreaTwo',
    ];

    private static array $cascade_duplicates = [
        'MyLinkOne',
        'MyLinkTwo',
        'MyLinkAreaOne',
        'MyLinkAreaTwo',
    ];
}

Adding multiple has_many's of the same type normally isn't possible because you'll have something like this:

Page
  $has_many = [
    MyThings => MyDataObject::class,
    MoreThings => MyDataObject::class,
  ]

MyDataObject
  $has_one = [
    Page => Page::class
  ];

And you'll get $myDataObject->PageID = 123 - but there's no way to know if it should belong to the 'MyThings' or the 'MoreThings' relationship on Page

The official way to solve "multiple has_manys of the same type" this is with dot notation where the the corresponding has_one relationship is suffixed to the classname of has_many relationship. In this instance it would look something like this (excuse my use of RelOne and RelTwo, I couldn't think of proper naming here):

Page
  $has_many = [
    MyThings => MyDataObject::class . '.RelOne',
    MoreThings => MyDataObject::class . '.RelTwo',
  ]

MyDataObject
  $has_one = [
    RelOne => Page::class,
    RelTwo => Page::class,
  ];

However in the linkfield context we cannot use dot-notation, because "MyDataObject" would be "Link" and we can't set the $has_one's on Link since it has no knowledge of project code. We'd need to ask people to add the $has_ones to Link via extensions which is very clunky.

Note there's somethings to do with unit testing that needs to be extracted from #90 before closing that PR

emteknetnz · 2023-10-10T03:57:58Z

Just had an offline chat with Guy - we could get rid of DataObject extension and just put a $has_one => [Owner] on Link/LinkArea if we instead made the Page / project level a $belongs_to instead of a $has_one. However this then becomes a bit of a usability issue because website developers are very much used to using $has_one, and $belongs_to is the weird one that people often don't bother to implement. This means we'd be turning LinkField's API into something a bit non-standard

GuySartorelli

I have reviewed this assuming that the approach is fundamentally solid.

Another way we could approach this is to reconsider how has_many relations work.

We could add a new OwnerRelation DB field. This would go hand-in hand with the OwnerID and OwnerClass fields that will already exist from the polymorphic has_one.
When the link field saves the link, it saves the name of the relation that owns it. This way we have a way to identify which has_many or has_one was used to save the given record.
We could implement this somewhere in framework, and then in DataObject::getComponents() if this special new relation type is being used, add a ->filter("{$hasOneName}Relation", $hasManyName) so it only fetches the records for that relation.
If we don't implement any of this in framework, we can add an extension hook to DataObject::getComponents() (there's already one in getManyManyComponents() so it makes sense to add one here too) and add that filter via the extension hook.

There's probably a bunch of stuff about that approach that seems obvious to me that isn't obvious to anyone else so please do ask clarification questions.

GuySartorelli · 2023-10-10T04:14:10Z

_config/config.yml

+SilverStripe\ORM\DataObject:
+  extensions:
+    - SilverStripe\LinkField\Extensions\DataObjectExtension


Let people apply it themselves to any classes where they want to use links (the same way elemental doesn't apply its extension to DataObject or to SiteTree by default).

GuySartorelli · 2023-10-10T04:15:31Z

src/Extensions/LinkObjectExtension.php

+    private static array $db = [
+        'OwnerID' => 'Int',
+        'OwnerClassName' => 'Varchar',
+    ];


Suggested change

private static array $db = [

'OwnerID' => 'Int',

'OwnerClassName' => 'Varchar',

];

private static array $jas_one = [

'Owner' => DataObject::class,

];

This creates the same db fields (but with the correct types, dbforeignkey and dbclassname) and gives us the ability to go $this->Owner()

This should be declared directly on Link and LinkArea though. No need for it here.

Also should probably be Parent instead of Owner

GuySartorelli · 2023-10-10T04:17:23Z

src/Extensions/LinkObjectExtension.php

+ * This is only intended to be added to Link and LinkArea
+ * Implemented as an extension rather than base class so it doesn't create an extra base table that needs to be joined
+ */
+class LinkObjectExtension extends DataExtension


Don't make this an extension. If you want to share the can* methods just use a trait which is more performant. There's nothing here that needs to be in an extension.

The private static $db cannot be used as a trait, because it'll clash with the private static $db on Link. You'll get this error:

PHP Fatal error: Link and LinkObjectTrait define the same property ($db) in the composition of Link. However, the definition differs and is considered incompatible.

The Extension class is basically a heavier weight "Silverstripe trait" and will nicely merge private statics

The private static $db cannot be used as a trait, because it'll clash with the private static $db on Link.

Correct. As suggested in #101 (comment) the has_one should be declared on the distinct classes instead. Ideally, so will the can* methods but I understand not wanting to repeat them

GuySartorelli · 2023-10-10T04:22:14Z

src/Extensions/LinkObjectExtension.php

+    private function getOwningDataObject(): ?DataObject
+    {
+        // Thismethod is not called getOwner() because of existing Extension::getOwner() method
+        //
+        // If this is a Link, and LinkArea is set on it return that
+        if (is_a($this->owner, Link::class, true)) {
+            $linkArea = $this->owner->LinkArea();
+            if ($linkArea && $linkArea->exists()) {
+                return $linkArea;
+            }
+        }
+        // Otherwise look for the ownerID + ownerClassName
+        // These are set in DataObjectExtension::onAfterWrite()
+        $ownerID = $this->owner->OwnerID;
+        $ownerClassName = $this->owner->OwnerClassName;
+        if ($ownerID === 0 || $ownerClassName === '') {
+            return null;
+        }
+        return $ownerClassName::get()->byID($ownerID);
+    }


This won't be needed on LinkArea at all because of the has_one.

On Link, the Owner relation will be the LinkArea, so this method will still be useful there. But it should be like this:

public function Owner() { $owner = $this->getComponent('Owner'); // If $owner is the LinkArea, return the underlying record that owns the area. if ($owner instanceof LinkArea) { $owner = $owner->Owner(); } return $owner; }

Unfortunately we can't call this getOwner() because the magic that turns relations into methods doesn't check for getters.

With this method, we can rely on $this->Owner() in Link always being whatever ultimately owns the link - the LinkArea record is completely transparent.

This won't be needed on LinkArea at all because of the has_one.

I'm not sure? The has_one is on an artibary DataObject (usually Page), so the LinkArea needs this as a way to find it's Owner/Parent?

On Link, the Owner relation will be the LinkArea

Not if it's not using a LinkArea, and instead it's a has_one on an arbitary DataObject (usually Page).

General idea for this PR was that it allows Links to be used as has_one's on Page's, or as has_many's via LinkArea's

GuySartorelli · 2023-10-10T04:22:36Z

src/Extensions/LinkObjectExtension.php

+        if ($extended !== null) {
+            return $extended;
+        }
+        $owner = $this->getOwningDataObject();


Suggested change

$owner = $this->getOwningDataObject();

$owner = $this->Owner();

GuySartorelli · 2023-10-10T04:26:50Z

src/Models/Link.php

+    ];
+
+    private static array $extensions = [
+        Versioned::class,


Why versioned?

Linked issue says "versioned operations" - you need to make Link Versioned to do versioned operations :-)

GuySartorelli · 2023-10-10T04:27:34Z

src/Models/LinkArea.php

+
+    private static array $extensions = [
+        Versioned::class,
+        LinkObjectExtension::class,


Suggested change

LinkObjectExtension::class,

Either add all the can* methods in here or apply them as a trait.

GuySartorelli · 2023-10-10T04:27:39Z

src/Models/LinkArea.php

+    ];
+
+    private static array $extensions = [
+        Versioned::class,


Why versioned?

Replied above on Link

GuySartorelli · 2023-10-10T04:34:44Z

src/Extensions/DataObjectExtension.php

+            // skip for the has_one:LinkArea relation on Link
+            if (is_a($this->owner, Link::class) && $relation === 'LinkArea') {
+                continue;
+            }


Suggested change

// skip for the has_one:LinkArea relation on Link

if (is_a($this->owner, Link::class) && $relation === 'LinkArea') {

continue;

}

This extension shouldn't be applied directly to Link or to DataObject, so we don't need this - unless we expect a link to ever have a child link? But I'd say we don't want to support nesting links.

GuySartorelli · 2023-10-10T04:43:46Z

src/Extensions/DataObjectExtension.php

+            if ($relationObj->OwnerID !== $this->owner->ID) {
+                $relationObj->OwnerID = $this->owner->ID;
+                $doWrite = true;
+            }
+            if ($relationObj->OwnerClassName !== $this->owner->ClassName) {
+                $relationObj->OwnerClassName = $this->owner->ClassName;
+                $doWrite = true;
+            }


I don't like that we're essentially storing the has_one in two places in the DB here. We're really working around a fundamental limitation in framework.
I would prefer that we either:

Explicitly document that people have to use belongs_to for their links and link areas, OR

Implement a way in framework to say "these has_one relations are the same relation, so only store it once!" effectively treating one of them as a belongs_to, OR

Make framework store has_many in its own table like many_many, which would allow using a single belongs_to in Link to an arbitrary number of has_many or has_one relations on other classes (which means we wouldn't need LinkArea either) (would be a new major of framework, or a new relation type, or a new behaviour that only happens when framework identifies that the has_many is trying to link up to a belongs_to))

All of the above do away with needing this extension altogether.

emteknetnz · 2023-10-10T06:21:12Z

Have chatted offline with Guy re his ideas around adding a "third has-many column" that is essentially a replacement for dot-notation. If that works it would remove the need for the intermediary LinkArea table, which would be great performance wise. So he'll explore that further and if that works we'll ditch this approach

emteknetnz · 2023-10-12T21:36:42Z

Closing in favor of #102

This was referenced Oct 9, 2023

Reverse ownership #5

Closed

Permisson model #12

Closed

NEW Linkfield ownership

b8ff21d

emteknetnz force-pushed the pulls/4/ownership branch from a28643e to b8ff21d Compare October 9, 2023 10:07

GuySartorelli reviewed Oct 10, 2023

View reviewed changes

emteknetnz closed this Oct 12, 2023

GuySartorelli deleted the pulls/4/ownership branch October 12, 2023 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW Linkfield ownership #101

NEW Linkfield ownership #101

emteknetnz commented Oct 9, 2023 •

edited

Loading

emteknetnz commented Oct 10, 2023

GuySartorelli left a comment •

edited

Loading

GuySartorelli Oct 10, 2023

GuySartorelli Oct 10, 2023

GuySartorelli Oct 10, 2023

GuySartorelli Oct 10, 2023

emteknetnz Oct 10, 2023 •

edited

Loading

GuySartorelli Oct 10, 2023 •

edited

Loading

GuySartorelli Oct 10, 2023

emteknetnz Oct 10, 2023

GuySartorelli Oct 10, 2023

GuySartorelli Oct 10, 2023

emteknetnz Oct 10, 2023

GuySartorelli Oct 10, 2023

GuySartorelli Oct 10, 2023

emteknetnz Oct 10, 2023

GuySartorelli Oct 10, 2023

GuySartorelli Oct 10, 2023

emteknetnz commented Oct 10, 2023 •

edited

Loading

emteknetnz commented Oct 12, 2023

	$owner = $this->getOwningDataObject();
	$owner = $this->Owner();

NEW Linkfield ownership #101

NEW Linkfield ownership #101

Conversation

emteknetnz commented Oct 9, 2023 • edited Loading

emteknetnz commented Oct 10, 2023

GuySartorelli left a comment • 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

emteknetnz Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

GuySartorelli Oct 10, 2023 • 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

emteknetnz commented Oct 10, 2023 • edited Loading

emteknetnz commented Oct 12, 2023

emteknetnz commented Oct 9, 2023 •

edited

Loading

GuySartorelli left a comment •

edited

Loading

emteknetnz Oct 10, 2023 •

edited

Loading

GuySartorelli Oct 10, 2023 •

edited

Loading

emteknetnz commented Oct 10, 2023 •

edited

Loading