Skip to content
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 Linkfield ownership #101

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion _config.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,4 @@

// Avoid creating global variables
call_user_func(function () {

});
4 changes: 4 additions & 0 deletions _config/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,7 @@ SilverStripe\CMS\Forms\AnchorSelectorField:
SilverStripe\LinkField\Form\FormFactory:
extensions:
- SilverStripe\LinkField\Extensions\FormFactoryExtension

SilverStripe\ORM\DataObject:
extensions:
- SilverStripe\LinkField\Extensions\DataObjectExtension
Comment on lines +25 to +27
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

47 changes: 47 additions & 0 deletions src/Extensions/DataObjectExtension.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<?php

namespace SilverStripe\LinkField\Extensions;

use SilverStripe\ORM\DataExtension;
use SilverStripe\LinkField\Models\Link;
use SilverStripe\LinkField\Models\LinkArea;

class DataObjectExtension extends DataExtension
{
public function onAfterWrite(): void
{
// Using onAfterWrite instead of onBeforeWrite to ensure that $this->owner->ID is not zero when creating new objects
parent::onAfterWrite();
foreach ($this->owner->hasOne() as $relation => $relationClassName) {
$relationField = $relation . 'ID';
$relationID = $this->owner->$relationField;
if (!$relationID) {
continue;
}
if (!is_a($relationClassName, Link::class, true) && !is_a($relationClassName, LinkArea::class, true)) {
continue;
}
// skip for the has_one:LinkArea relation on Link
if (is_a($this->owner, Link::class) && $relation === 'LinkArea') {
continue;
}
Comment on lines +24 to +27
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$relationObj = $relationClassName::get()->byID($relationID);
if ($relationObj === null) {
// could throw an Exception here, though not sure how if user would be able to fix it
continue;
}
$doWrite = false;
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;
}
Comment on lines +34 to +41
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Explicitly document that people have to use belongs_to for their links and link areas, OR
  2. 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
  3. 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.

if ($doWrite) {
$relationObj->write();
}
}
}
}
76 changes: 76 additions & 0 deletions src/Extensions/LinkObjectExtension.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
<?php

namespace SilverStripe\LinkField\Extensions;

use SilverStripe\LinkField\Models\Link;
use SilverStripe\ORM\DataExtension;
use SilverStripe\ORM\DataObject;

/**
* 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@emteknetnz emteknetnz Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@GuySartorelli GuySartorelli Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
private static array $db = [
'OwnerID' => 'Int',
'OwnerClassName' => 'Varchar',
];
Comment on lines +15 to +18
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also should probably be Parent instead of Owner


public function canView($member = null)
{
return $this->canCheck('canView', $member);
}

public function canCreate($member = null)
{
return $this->canCheck('canCreate', $member);
}

public function canEdit($member = null)
{
return $this->canCheck('canEdit', $member);
}

public function canDelete($member = null)
{
return $this->canCheck('canDelete', $member);
}

private function canCheck(string $canMethod, $member)
{
if (!$member) {
$member = Security::getCurrentUser();
}
$extended = $this->extendedCan($canMethod, $member);
if ($extended !== null) {
return $extended;
}
$owner = $this->getOwningDataObject();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if ($owner) {
return $owner->$canMethod($member);
}
return parent::$canMethod($member);
}

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);
}
Comment on lines +56 to +75
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
12 changes: 12 additions & 0 deletions src/Models/Link.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
use SilverStripe\ORM\DataObject;
use SilverStripe\ORM\FieldType\DBHTMLText;
use SilverStripe\View\Requirements;
use SilverStripe\LinkField\Extensions\LinkObjectExtension;
use SilverStripe\Versioned\Versioned;
use SilverStripe\LinkField\Models\LinkArea;

/**
* A Link Data Object. This class should be a subclass, and you should never directly interact with a plain Link
Expand All @@ -33,6 +36,15 @@ class Link extends DataObject implements JsonData, Type
'OpenInNew' => 'Boolean',
];

private static array $has_one = [
'LinkArea' => LinkArea::class
];
Comment on lines +39 to +41
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static array $has_one = [
'LinkArea' => LinkArea::class
];
private static array $has_one = [
'Owner' => DataObject::class
];

As per above comments. Again, probably this should be Parent, but I'll leave everything as Owner for this pass.


private static array $extensions = [
Versioned::class,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why versioned?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

LinkObjectExtension::class,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LinkObjectExtension::class,

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

];

/**
* In-memory only property used to change link type
* This case is relevant for CMS edit form which doesn't use React driven UI
Expand Down
34 changes: 34 additions & 0 deletions src/Models/LinkArea.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php

namespace SilverStripe\LinkField\Models;

use SilverStripe\LinkField\Models\Link;
use SilverStripe\LinkField\Extensions\LinkObjectExtension;
use SilverStripe\ORM\DataObject;
use SilverStripe\Versioned\Versioned;

class LinkArea extends DataObject
{
private static $table_name = 'LinkField_LinkArea';

private static array $has_many = [
'Links' => Link::class,
];

private static array $owns = [
'Links',
];

private static array $cascade_deletes = [
'Links',
];

private static array $cascade_duplicates = [
'Links',
];

private static array $extensions = [
Versioned::class,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why versioned?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replied above on Link

LinkObjectExtension::class,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LinkObjectExtension::class,

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

];
}
Loading