From 7f65d4fdee873fc7746acdc82e3bc2b591138a02 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Tue, 12 Dec 2023 14:14:55 +1300 Subject: [PATCH] API Add new Owner relation for handling permissions (#127) --- README.md | 22 +++--- src/Form/LinkField.php | 10 +++ src/Models/FileLink.php | 2 +- src/Models/Link.php | 44 +++++++++++- tests/php/Form/LinkFieldTest.php | 43 ++++++++++++ tests/php/Models/LinkTest.php | 92 +++++++++++++++++++++++++ tests/php/Models/LinkTest.yml | 7 ++ tests/php/Models/LinkTest/LinkOwner.php | 18 +++++ 8 files changed, 227 insertions(+), 11 deletions(-) create mode 100644 tests/php/Form/LinkFieldTest.php create mode 100644 tests/php/Models/LinkTest/LinkOwner.php diff --git a/README.md b/README.md index 4f0162e7..1f45e9b2 100644 --- a/README.md +++ b/README.md @@ -40,22 +40,27 @@ class Page extends SiteTree ]; private static $has_many = [ - 'HasManyLinks' => Link::class, + // Multiple has_many relations on the same class should point at the same has_one on Link. + 'HasManyLinksOne' => Link::class . '.Owner', + 'HasManyLinksTwo' => Link::class . '.Owner', ]; private static array $owns = [ 'HasOneLink', - 'HasManyLinks', + 'HasManyLinksOne', + 'HasManyLinksTwo', ]; private static array $cascade_deletes = [ 'HasOneLink', - 'HasManyLinks', + 'HasManyLinksOne', + 'HasManyLinksTwo', ]; private static array $cascade_duplicates = [ 'HasOneLink', - 'HasManyLinks', + 'HasManyLinksOne', + 'HasManyLinksTwo', ]; public function getCMSFields() @@ -63,13 +68,14 @@ class Page extends SiteTree $fields = parent::getCMSFields(); // Don't forget to remove the auto-scaffolded fields! - $fields->removeByName(['HasOneLinkID', 'Links']); + $fields->removeByName(['HasOneLinkID', 'HasManyLinksOne', 'HasManyLinksTwo']); $fields->addFieldsToTab( 'Root.Main', [ LinkField::create('HasOneLink'), - MultiLinkField::create('HasManyLinks'), + MultiLinkField::create('HasManyLinksOne'), + MultiLinkField::create('HasManyLinksTwo'), ], ); @@ -78,13 +84,11 @@ class Page extends SiteTree } ``` -Note that you also need to add a `has_one` relation on the `Link` model to match your `has_many` here. See [official docs about `has_many`](https://docs.silverstripe.org/en/developer_guides/model/relations/#has-many) - Adding the relationship(s) to the `$owns`, `$cascade_deletes`, and `$cascade_duplicates` config properties is required for versioning (publishing) to work correctly. ## Default title for each link type -By default, if the title for the link has not been set, then the default title will be used instead according to the type of link that is used. Default link is not stored in the database as link title. This value is used only when rendering page content. +By default, if the title for the link has not been set, then the default title will be used instead according to the type of link that is used. Default link is not stored in the database as link title. This value is used only when rendering page content. The default title value can be updated using an `Extension` with an `updateDefaultLinkTitle()` method and applying that extension to a subclass of `Link`. diff --git a/src/Form/LinkField.php b/src/Form/LinkField.php index e31eab16..e02a3d98 100644 --- a/src/Form/LinkField.php +++ b/src/Form/LinkField.php @@ -44,6 +44,16 @@ public function saveInto(DataObjectInterface $record) $dbColumn = $fieldname . 'ID'; $record->$dbColumn = $linkID; + // Store the record as the owner of the link. + // Required for permission checks, etc. + $link = Link::get()->byID($linkID); + if ($link) { + $link->OwnerID = $record->ID; + $link->OwnerClass = $record->ClassName; + $link->OwnerRelation = $fieldname; + $link->write(); + } + return $this; } diff --git a/src/Models/FileLink.php b/src/Models/FileLink.php index 21b8d832..a0b5051e 100644 --- a/src/Models/FileLink.php +++ b/src/Models/FileLink.php @@ -48,7 +48,7 @@ public function getDefaultTitle(): string 'File missing', ); } - + return (string) $this->getDescription(); } } diff --git a/src/Models/Link.php b/src/Models/Link.php index 1cdb9638..dbebf719 100644 --- a/src/Models/Link.php +++ b/src/Models/Link.php @@ -12,6 +12,7 @@ use SilverStripe\Forms\RequiredFields; use SilverStripe\LinkField\Type\Registry; use SilverStripe\ORM\DataObject; +use SilverStripe\ORM\DataObjectSchema; use SilverStripe\ORM\FieldType\DBHTMLText; use SilverStripe\Versioned\Versioned; @@ -19,6 +20,9 @@ * A Link Data Object. This class should be treated as abstract. 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 */ @@ -31,6 +35,17 @@ class Link extends DataObject 'OpenInNew' => 'Boolean', ]; + private static array $has_one = [ + // Note that this handles one-to-many relations AND one-to-one relations. + // Any has_one pointing at Link will be intentionally double handled - this allows us to use the owner + // for permission checks and to link back to the owner from reports, etc. + // See also the Owner method. + 'Owner' => [ + 'class' => DataObject::class, + DataObjectSchema::HAS_ONE_MULTI_RELATIONAL => true, + ], + ]; + private static array $extensions = [ Versioned::class, ]; @@ -300,6 +315,33 @@ public function getVersionedState(): string return 'published'; } + /** + * Get the owner of this link, if there is one. + * + * Returns null if the reciprocal relation is a has_one which no longer contains this link + * or if there simply is no actual owner record in the db. + */ + public function Owner(): ?DataObject + { + $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. + if ($this->OwnerRelation && $owner->getRelationType($this->OwnerRelation) === 'has_one') { + $idField = "{$this->OwnerRelation}ID"; + if ($owner->$idField !== $this->ID) { + return null; + } + } + + // Return null if there simply is no owner + if (!$owner || !$owner->isInDB()) { + return null; + } + + return $owner; + } + /** * Get all link types except the generic one * @@ -327,7 +369,7 @@ public function getDisplayTitle(): string if ($this->Title) { return $this->Title; } - + $defaultLinkTitle = $this->getDefaultTitle(); $this->extend('updateDefaultLinkTitle', $defaultLinkTitle); diff --git a/tests/php/Form/LinkFieldTest.php b/tests/php/Form/LinkFieldTest.php new file mode 100644 index 00000000..4fb94aec --- /dev/null +++ b/tests/php/Form/LinkFieldTest.php @@ -0,0 +1,43 @@ +write(); + $owner = new LinkOwner(); + $owner->write(); + + // Save link into owner + $field->setValue($link->ID); + $field->saveInto($owner); + // Get the link again - the new values are in the DB. + $link = Link::get()->byID($link->ID); + + // Validate + $this->assertSame($link->ID, $owner->LinkID); + $this->assertSame($owner->ID, $link->OwnerID); + $this->assertSame($owner->ClassName, $link->OwnerClass); + $this->assertSame('Link', $link->OwnerRelation); + } +} diff --git a/tests/php/Models/LinkTest.php b/tests/php/Models/LinkTest.php index 85b1a028..7ad54151 100644 --- a/tests/php/Models/LinkTest.php +++ b/tests/php/Models/LinkTest.php @@ -21,6 +21,7 @@ use SilverStripe\ORM\ValidationException; use SilverStripe\Versioned\Versioned; use SilverStripe\LinkField\Tests\Extensions\ExternalLinkExtension; +use SilverStripe\LinkField\Tests\Models\LinkTest\LinkOwner; class LinkTest extends SapphireTest { @@ -35,6 +36,10 @@ class LinkTest extends SapphireTest ], ]; + protected static $extra_dataobjects = [ + LinkOwner::class, + ]; + protected function setUp(): void { parent::setUp(); @@ -423,4 +428,91 @@ public function testDefaultLinkTitle(string $identifier, string $class, string $ $this->assertEquals($expected, $link->getDisplayTitle()); } + + public function provideOwner() + { + return [ + 'null because there is no owner' => [ + 'class' => EmailLink::class, + 'fixture' => 'email-link-with-email', + 'expected' => null, + ], + 'null because the has_one is only stored on the owner' => [ + 'class' => SiteTreeLink::class, + 'fixture' => 'page-link-1', + // The owner has_one link, but the relationship wasn't saved in the link's Owner has_one. + // See the LinkOwner.owns-has-one fixture. + 'expected' => null, + ], + 'has_many owner always works' => [ + 'class' => SiteTreeLink::class, + 'fixture' => 'page-link-page-only', + 'expected' => [ + 'class' => LinkOwner::class, + 'fixture' => 'owns-has-many', + ], + ], + ]; + } + + /** + * Test the functionality of the overridden Owner method. + * Note this is NOT explicitly testing multi-relational has_many relations pointing at the has_one since that's + * a framework functionality, not a linkfield one. + * + * @dataProvider provideOwner + */ + public function testOwner(string $class, string $fixture, ?array $expected) + { + $link = $this->objFromFixture($class, $fixture); + if (is_array($expected)) { + $expected = $this->idFromFixture($expected['class'], $expected['fixture']); + } + + $this->assertSame($expected, $link->Owner()?->ID); + } + + /** + * Testing a scenario where a has_one to has_one is stored on the link. + * Note we can't easily use providers here because of all the necessary logic to set this all up. + */ + public function testOwnerHasOne() + { + $link = new Link(); + $link->write(); + $owner = new LinkOwner(); + $owner->write(); + + // Add the owner relation on the link - without the relation + $link->update([ + 'OwnerID' => $owner->ID, + 'OwnerClass' => $owner->ClassName, + ]); + $link->write(); + + // Clear out any previous-fetches of the owner component. We'll do this each time we check the owner. + $link->flushCache(false); + // The link tells us who the owner is - it doesn't have any way to tell that + // the owner doesn't have a reciprocal relationship yet. + $this->assertSame($owner->ID, $link->Owner()?->ID); + + // LinkField adds the relation name to the link, so this is what we'll normally see + $link->OwnerRelation = 'Link'; + $link->write(); + + // The actual has_one component is the LinkOwner record + $link->flushCache(false); + $this->assertSame($owner->ID, $link->getComponent('Owner')?->ID); + // Owner returns null, because there is no reciprocal relationship from the LinkOwner record + $link->flushCache(false); + $this->assertSame(null, $link->Owner()); + + // Add the link relation on the owner + $owner->LinkID = $link->ID; + $owner->write(); + + // The link is now happy to declare its owner to us + $link->flushCache(false); + $this->assertSame($owner->ID, $link->Owner()?->ID); + } } diff --git a/tests/php/Models/LinkTest.yml b/tests/php/Models/LinkTest.yml index 89229344..7f2bdb0b 100644 --- a/tests/php/Models/LinkTest.yml +++ b/tests/php/Models/LinkTest.yml @@ -79,3 +79,10 @@ SilverStripe\LinkField\Models\FileLink: OpenInNew: true file-link-with-default-title: File: =>SilverStripe\Assets\Image.image-1 + +SilverStripe\LinkField\Tests\Models\LinkTest\LinkOwner: + owns-has-one: + Link: =>SilverStripe\LinkField\Models\SiteTreeLink.page-link-1 + owns-has-many: + LinkList: + - =>SilverStripe\LinkField\Models\SiteTreeLink.page-link-page-only diff --git a/tests/php/Models/LinkTest/LinkOwner.php b/tests/php/Models/LinkTest/LinkOwner.php new file mode 100644 index 00000000..a9b9e02e --- /dev/null +++ b/tests/php/Models/LinkTest/LinkOwner.php @@ -0,0 +1,18 @@ + Link::class, + ]; + + private static array $has_many = [ + 'LinkList' => Link::class . '.Owner', + ]; +}