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 Link ownership (alternative to #101) #102

Closed
Show file tree
Hide file tree
Changes from 1 commit
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
7 changes: 7 additions & 0 deletions _config/config.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
---
Name: linkfield
---
# Adding this here for simplicity for testing the PR, but
# we'd have to make a determination as to whether this is
# automatically applied, or is documented as necessary
# for has_many links only.
SilverStripe\ORM\DataObject:
extensions:
- SilverStripe\LinkField\Extensions\DataObjectWithLinksExtension

SilverStripe\Admin\LeftAndMain:
extensions:
Expand Down
20 changes: 20 additions & 0 deletions src/Extensions/DataObjectWithLinksExtension.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
<?php

namespace SilverStripe\LinkField\Extensions;

use SilverStripe\Core\Extension;
use SilverStripe\LinkField\Models\Link;
use SilverStripe\ORM\HasManyList;

/**
* This extension must be applied to any DataObject which has a has_many relation to the Link model.
*/
class DataObjectWithLinksExtension extends Extension
{
public function updateComponents(HasManyList &$list, string $relation)
{
if (is_a($list->dataClass(), Link::class, true)) {
$list = $list->filter('OwnerRelation', $relation);
}
}
}
4 changes: 4 additions & 0 deletions src/Form/JsonField.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ public function saveInto(DataObjectInterface $record)
$dataValue = $this->dataValue();
$value = is_string($dataValue) ? $this->parseString($this->dataValue()) : $dataValue;

$value['OwnerID'] = $record->ID;
$value['OwnerClass'] = $record->ClassName;
$value['OwnerRelation'] = $this->getName();
Comment on lines +47 to +49
Copy link
Member Author

@GuySartorelli GuySartorelli Oct 12, 2023

Choose a reason for hiding this comment

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

Obviously this should be in LinkField instead of JsonField - but for the sake of this PR it's clearer to just put it here rather than refactor LinkField to make that work.

This is required because the owner will inevitably use a has_one instead of a belongs_to - and that means the data for the relation will be stored on the owner's db table. We need to also store it in the Link's table so that we can refer to it for can* checks, and link to the owner if we add e.g. broken link reports in the future.

In theory this being here also allows developers to use a belongs_to to link (assuming that can't already be done)


if ($class = DataObject::getSchema()->hasOneComponent(get_class($record), $fieldname)) {
/** @var JsonData|DataObject $jsonDataObject */

Expand Down
13 changes: 12 additions & 1 deletion src/Form/MultiLinkField.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
use SilverStripe\ORM\SS_List;

/**
* Allows CMS users to edit a list of links.
* Allows CMS users to edit a list of links in a has_many relation.
* Explicitly doesn't support many_many.
*/
class MultiLinkField extends JsonField
{
Expand Down Expand Up @@ -88,19 +89,29 @@ public function saveInto(DataObjectInterface $record)

/** @var HasMany|Link[] $links */
if ($links = $record->$fieldname()) {
/** @var Link $linkDO */
foreach ($links as $linkDO) {
$linkData = $this->shiftLinkDataByID($value, $linkDO->ID);
if ($linkData) {
// @TODO move all the JSON stuff into the field. The model shouldn't care that
// the form field represents its data as JSON temporarily.
// We should just be calling $linkDO->update($data) here with the data already
// explicitly as an associative array.
// Also, I'm assuming this IS always an associative array, even though the Link
// model doesn't assume that.
$linkData['OwnerRelation'] = $this->getName();
$linkDO->setData($linkData);
$linkDO->write();
} else {
$linkDO->delete();
}
}

// Guy's note: I'm assuming these are explicitly new, and above is explicitly existing links
foreach ($value as $linkData) {
unset($linkData['ID']);
$linkDO = Link::create();
$linkData['OwnerRelation'] = $this->getName();
$linkDO = $linkDO->setData($linkData);
$links->add($linkDO);
$linkDO->write();
Expand Down
88 changes: 84 additions & 4 deletions src/Models/Link.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
* A Link Data Object. This class should be a subclass, and you should never directly interact with a plain Link
* instance
*
* Note that links should be added via a has_one or has_many relation, NEVER a many_many relation. This is because
* some functionality such as the can* methods rely on having a single Owner.
*
* @property string $Title
* @property bool $OpenInNew
*/
Expand All @@ -30,21 +33,23 @@ class Link extends DataObject implements JsonData, Type
private static $table_name = 'LinkField_Link';

private static array $db = [
'OwnerRelation' => 'Varchar',
'Title' => 'Varchar',
'OpenInNew' => 'Boolean',
];

private static $has_one = [
// See also the OwnerRelation field added in $db
'Owner' => DataObject::class
];

/**
* In-memory only property used to change link type
* This case is relevant for CMS edit form which doesn't use React driven UI
* This is a workaround as changing the ClassName directly is not fully supported in the GridField admin
*/
private ?string $linkType = null;

private static $has_one = [
'Owner' => DataObject::class
];

private static $icon = 'link';

public function defineLinkTypeRequirements()
Expand Down Expand Up @@ -292,4 +297,79 @@ protected function FallbackTitle(): string
{
return '';
}

/**
* This is entirely optional but is something we have the power to do now.
* We can also do checks like this in onBeforeWrite for example.
*/
public function Owner()
{
$owner = $this->getComponent('Owner');
// Since the has_one is being stored in two places, double check the owner
// actually still owns this record. If not, return null.
$ownerRelationType = $owner->getRelationType($this->OwnerRelation);
if ($ownerRelationType === 'has_one') {
$idField = "{$this->OwnerRelation}ID";
if ($owner->$idField !== $this->ID) {
return null;
}
}
Comment on lines +311 to +316
Copy link
Member Author

Choose a reason for hiding this comment

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

Doing this check here means we will never be falsely stating that we have an owner when the owner doesn't think it owns the link anymore. Resolves the problem of storing the data in two places by ignoring that secondary data source if it's incorrect

Copy link
Member

Choose a reason for hiding this comment

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

Seem like like it would be better to simply not add the data in two places for has_one's and instead just use the normal way of doing it i.e. only save it on Page.MyLinkID? Not sure why we'd want to double handle it?

Seems like your thinking was the relationships work like this:

a) Page.has_one = [ MyLink => Link::class ] -- Link.has_one = [ Owner => Page::class ] <<< seems wrong
b) Page.has_many = [ SomeLinks => Links::class ] -- Link.has_one = [ Owner => Page::class ]

The relationships should be:

a) Page.has_one = [ MyLink => Link::class ] -- Link.belongs_to = [ OptionalOwner => Page::class ]
b) Page.has_many = [ SomeLinks => Links::class ] -- Link.has_one = [ Owner => Page::class ]

In practice a) Link.belongs_to won't get filled in, though that's fine

This means for the Page.has_one's we don't need Link.OwnerID/OwnerClass/OwnerRelation filled in

Let me know if I've understood this correctly

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't have a polymorphic belongs_to - i.e. this won't work:

private static $belongs_to = [
    'OptionalOwner' => DataObject::class,
];

And we don't want to explicitly say it has to be a Page (or SiteTree) that owns the link - that severely limits the use case, especially since bespoke are saying they want this mostly for use inside elemental blocks.

Yes my proposed solution is double handling, but it is the only way we have have a one-to-one polymorphic relation like this, to my knowledge. The double handling is mitigated by what I'm doing here to validate the data is still correct before providing the record when $link->Owner() is called.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that we could also say "you have to use belongs_to instead of has_one in your model" but you already rejected that because it's not as intuitive as adding a has_one.

Copy link
Member

@emteknetnz emteknetnz Oct 12, 2023

Choose a reason for hiding this comment

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

OK so can't have a belongs_to OptionalOwner. So we'd have this:

a) Page.has_one = [ MyLink => Link::class ]
b) Page.has_many = [ SomeLinks => Links::class ] -- Link.has_one = [ Owner => Page::class ]

Both scenario's have has_one's which is the relationship that actually adds something to the database, so technically this is all we need to at least get things rendering in the correct place

I guess the downside here is now in a) you can't get the Owner() of the Link in in order to do a canCheck(). This is could be worked around if you you assume that you're strictly editing Links in the CMS on a Page/Element Form in which case the canCheck() for Link is implicitly done when rendering the Form.

However this falls apart if you have a GridField of Link's, or using XHR to call an endpoint to interact with the dataobject's directly

This is essentially the permission model for linkfield now i.e. there isn't one

If guess we're going to have a permissions model that calls Owner()->canCheck() then we'll need to double handle, or we just (technically correctly) use $belongs_to on Page so the foriegn key isn't added there, though that be an awful upgrade experience (all linkfields currently use $has_one) and is just generally unintuitive (i.e. saying that a Page belongs-to a Link rather than a Page has-one Link)

Copy link
Member Author

@GuySartorelli GuySartorelli Oct 12, 2023

Choose a reason for hiding this comment

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

Yeah the has_one to has_one connection I'm proposing here is specifically and exclusively so we can refer to the Owner record from the Link.

This is useful for:

  • Correct can* checks (regardless of where you're accessing the link from)
  • Linking back to the owner e.g. from a broken links report

I personally think it's worthwhile having, but we can live without it if we decide we want simpler can* checks (e.g. gorriecoe/silverstripe-link just assumes everyone can do everything) and we don't care about being able to access the owner in any given scenario. In that case I'd recommend we never refer to the Owner relation in code except for when declaring the has_one which would only be there as a way to store the has_many data in the database. Other than storing that data, if we're not using it for one-to-one relationships as well then we should pretend it doesn't exist.

The main purpose of this POC is for the has_many to has_one stuff anyway.

Copy link
Member

Choose a reason for hiding this comment

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

"double check the owner"

I do like this returns null since there are two has_ones pointing at each other, it's treating Page as the "source of truth".

We're really only adding the has_one to Link here out of necessity to make our canCheck() work.

This double has_one is inherently fragile as things can get out of sync. Need to assume that we're not use the LinkField UI to manage links so everything needs to happen at the model level

Perhaps there's additional things we can do to strengthen this?

  • Disallow changing OwnerID/OwnerClass/OwnerRelation on Link if OwnerID !== 0 (though maybe do allow them to be set back to 0/empty)
  • DataObjectExtension onBeforeWrite ... if a has_one that's changed is for a Link::class, then gets gets the OLD Link and reset OwnerID/OwnerClass/OwnerRelation back to 0/empty. Though at this point maybe we're better off just outright deleting the Link).

Copy link
Member Author

@GuySartorelli GuySartorelli Oct 12, 2023

Choose a reason for hiding this comment

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

We're really only adding the has_one to Link here out of necessity to make our canCheck() work.

And as a way to allow us to link to of refer to the Owner for any other purpose, such as adding a link to the cms edit form from a broken links report, etc. It's mostly for the can* checks, but not exclusively.

Need to assume that we're not use the LinkField UI to manage links so everything needs to happen at the model level

We're already throwing that assumption out the window for the has_many relation here. I think for anything in this POC to work we have to say "If you are creating new links, you must use the link fields provided in this module".
You should be able to edit links fine in any context because the relation data has already been saved by that point.

Perhaps there's additional things we can do to strengthen this?

Quite possibly. Remember that this is just a POC and again it's mostly just to ensure we can have multiple has_many relations on the owner side. The one-to-one is just extra sugar on top.

return $owner;
}

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

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

public function canDelete($member = null, $context = [])
{
return $this->canPerformAction(__FUNCTION__, $member);
}

public function canCreate($member = null, $context = [])
{
return $this->canPerformAction(__FUNCTION__, $member);
}

public function can($perm, $member = null, $context = [])
{
$delegateToExistingMethods = [
Copy link
Member

Choose a reason for hiding this comment

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

This bit's a bit hard to understand, I'd just simplify this to something like (untested)

$check = ucfirst(strtolower($perm));
return match ($check) {
    'View', 'Create', 'Edit', 'Delete' => $this->{"can$check"}($member, $context),
    default => parent::can($perm, $member, $context)
}

Copy link
Member Author

@GuySartorelli GuySartorelli Oct 12, 2023

Choose a reason for hiding this comment

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

Makes sense. I won't make the change right now (the underlying PR that this relies on needs to be merged or swapped out for anyfield stuff or something first anyway) but we'll go with that when it's time to turn this from a POC into a real PR.

'view',
'edit',
'create',
'delete',
];

$owner = $this->Owner();
if ($owner && $owner->exists() && !in_array(strtolower($perm), $delegateToExistingMethods)) {
return $owner->can($perm, $member, $context);
}

return parent::can($perm, $member, $context);
}

private function canPerformAction(string $canMethod, $member, $context = [])
{
$results = $this->extendedCan($canMethod, $member, $context);
if (isset($results)) {
return $results;
}

$owner = $this->Owner();
if ($owner && $owner->exists()) {
// Can delete or create links if you can edit its owner.
if ($canMethod === 'canView' ||$canMethod === 'canCreate' || $canMethod === 'canDelete') {
$canMethod = 'canEdit';
}
return $owner->$canMethod($member, $context);
}

return parent::$canMethod($member, $context);
}
}
Loading