From c75615f93d5fb3c899e2b9ee40e8528614e0a644 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Thu, 15 Feb 2024 17:18:02 +1300 Subject: [PATCH] ENH More standardisation (#223) * ENH Refactoring, new tests, and other standardisation * MNT Reorder methods to match our coding conventions --- src/Form/AbstractLinkField.php | 2 +- src/Models/EmailLink.php | 14 +- src/Models/ExternalLink.php | 14 +- src/Models/FileLink.php | 28 +-- src/Models/Link.php | 289 +++++++++----------------- src/Models/PhoneLink.php | 14 +- src/Models/SiteTreeLink.php | 56 ++--- tests/php/Models/FileLinkTest.php | 25 ++- tests/php/Models/LinkTest.php | 2 +- tests/php/Models/SiteTreeLinkTest.php | 12 +- 10 files changed, 197 insertions(+), 259 deletions(-) diff --git a/src/Form/AbstractLinkField.php b/src/Form/AbstractLinkField.php index a31762f3..ff0f6f18 100644 --- a/src/Form/AbstractLinkField.php +++ b/src/Form/AbstractLinkField.php @@ -80,7 +80,7 @@ public function getTypesProp(): string $typesList[$key] = [ 'key' => $key, 'title' => $type->getMenuTitle(), - 'handlerName' => $type->LinkTypeHandlerName(), + 'handlerName' => $type->getLinkTypeHandlerName(), 'priority' => $class::config()->get('menu_priority'), 'icon' => $class::config()->get('icon'), 'allowed' => $allowed, diff --git a/src/Models/EmailLink.php b/src/Models/EmailLink.php index 55cb7829..2a84053d 100644 --- a/src/Models/EmailLink.php +++ b/src/Models/EmailLink.php @@ -36,6 +36,13 @@ public function getCMSFields(): FieldList return parent::getCMSFields(); } + public function getCMSCompositeValidator(): CompositeValidator + { + $validator = parent::getCMSCompositeValidator(); + $validator->addValidator(RequiredFields::create(['Email'])); + return $validator; + } + public function getDescription(): string { return $this->Email ?: ''; @@ -54,11 +61,4 @@ public function getMenuTitle(): string { return _t(__CLASS__ . '.LINKLABEL', 'Link to email address'); } - - public function getCMSCompositeValidator(): CompositeValidator - { - $validator = parent::getCMSCompositeValidator(); - $validator->addValidator(RequiredFields::create(['Email'])); - return $validator; - } } diff --git a/src/Models/ExternalLink.php b/src/Models/ExternalLink.php index 7d8103ed..3ff49ddc 100644 --- a/src/Models/ExternalLink.php +++ b/src/Models/ExternalLink.php @@ -37,6 +37,13 @@ public function getCMSFields(): FieldList return parent::getCMSFields(); } + public function getCMSCompositeValidator(): CompositeValidator + { + $validator = parent::getCMSCompositeValidator(); + $validator->addValidator(RequiredFields::create(['ExternalUrl'])); + return $validator; + } + public function getDescription(): string { return $this->ExternalUrl ?: ''; @@ -55,11 +62,4 @@ public function getMenuTitle(): string { return _t(__CLASS__ . '.LINKLABEL', 'Link to external URL'); } - - public function getCMSCompositeValidator(): CompositeValidator - { - $validator = parent::getCMSCompositeValidator(); - $validator->addValidator(RequiredFields::create(['ExternalUrl'])); - return $validator; - } } diff --git a/src/Models/FileLink.php b/src/Models/FileLink.php index 72af7f6e..b25703ad 100644 --- a/src/Models/FileLink.php +++ b/src/Models/FileLink.php @@ -34,6 +34,13 @@ public function getCMSFields(): FieldList return parent::getCMSFields(); } + public function getCMSCompositeValidator(): CompositeValidator + { + $validator = parent::getCMSCompositeValidator(); + $validator->addValidator(RequiredFields::create(['File'])); + return $validator; + } + public function getDescription(): string { $file = $this->File(); @@ -52,16 +59,6 @@ public function getURL(): string return $file->exists() ? (string) $file->getURL() : ''; } - public function getDefaultTitle(): string - { - $file = $this->File(); - if (!$file->exists()) { - return _t(__CLASS__ . '.MISSING_DEFAULT_TITLE', '(File missing)'); - } - - return (string) $this->getDescription(); - } - /** * The title that will be displayed in the dropdown * for selecting the link type to create. @@ -71,10 +68,13 @@ public function getMenuTitle(): string return _t(__CLASS__ . '.LINKLABEL', 'Link to a file'); } - public function getCMSCompositeValidator(): CompositeValidator + protected function getDefaultTitle(): string { - $validator = parent::getCMSCompositeValidator(); - $validator->addValidator(RequiredFields::create(['File'])); - return $validator; + $file = $this->File(); + if (!$file?->exists()) { + return _t(__CLASS__ . '.MISSING_DEFAULT_TITLE', '(File missing)'); + } + + return (string) $this->getDescription(); } } diff --git a/src/Models/Link.php b/src/Models/Link.php index cc70e22a..a9fd3bea 100644 --- a/src/Models/Link.php +++ b/src/Models/Link.php @@ -2,10 +2,7 @@ namespace SilverStripe\LinkField\Models; -use InvalidArgumentException; -use ReflectionException; use SilverStripe\Core\ClassInfo; -use SilverStripe\Core\Injector\Injector; use SilverStripe\Forms\FieldList; use SilverStripe\LinkField\Services\LinkTypeService; use SilverStripe\ORM\DataObject; @@ -69,25 +66,6 @@ class Link extends DataObject */ private static $icon = 'font-icon-link'; - public function getDescription(): string - { - return ''; - } - - public function scaffoldLinkFields(array $data): FieldList - { - return $this->getCMSFields(); - } - - public function LinkTypeHandlerName(): string - { - return 'FormBuilderModal'; - } - - /** - * @return FieldList - * @throws ReflectionException - */ public function getCMSFields(): FieldList { $this->beforeUpdateCMSFields(function (FieldList $fields) { @@ -118,105 +96,113 @@ public function getCMSFields(): FieldList return parent::getCMSFields(); } - public function onBeforeWrite(): void + /** + * Get a short description of the link. This is displayed in LinkField as an indication of what the link is pointing at. + * + * This method should be overridden by any subclasses + */ + public function getDescription(): string { - // Ensure a Sort value is set and that it's one larger than any other Sort value for - // this owner relation so that newly created Links on MultiLinkField's are properly sorted - if (!$this->Sort) { - $this->Sort = self::get()->filter([ - 'OwnerID' => $this->OwnerID, - 'OwnerRelation' => $this->OwnerRelation, - ])->max('Sort') + 1; - } + return ''; + } - parent::onBeforeWrite(); + /** + * Get the URL this Link links to. + * + * This method should be overridden by any subclasses + */ + public function getURL(): string + { + return ''; } - function setData($data): Link + /** + * Get the react component used to render the modal form. + */ + public function getLinkTypeHandlerName(): string { - if (is_string($data)) { - $data = json_decode($data, true); - - if (json_last_error() !== JSON_ERROR_NONE) { - throw new InvalidArgumentException( - _t( - __CLASS__ . '.INVALID_JSON', - '"{class}": Decoding json string failred with "{error}"', - [ - 'class' => static::class, - 'error' => json_last_error_msg(), - ], - sprintf( - '"%s": Decoding json string failred with "%s"', - static::class, - json_last_error_msg(), - ), - ), - ); - } - } elseif ($data instanceof Link) { - $data = $data->jsonSerialize(); - } + return 'FormBuilderModal'; + } - if (!is_array($data)) { - throw new InvalidArgumentException( - _t( - __CLASS__ . '.INVALID_DATA_TO_ARRAY', - '"{class}": Could not convert $data to an array.', - ['class' => static::class], - sprintf('%s: Could not convert $data to an array.', static::class), - ), - ); + /** + * The title that will be displayed in the dropdown + * for selecting the link type to create. + * + * Subclasses should override this. + * It will use the singular_name by default. + */ + public function getMenuTitle(): string + { + return $this->i18n_singular_name(); + } + + public function getTitle(): string + { + // If we have link text, we can just bail out without any changes + if ($this->LinkText) { + return $this->LinkText; } - $typeKey = $data['typeKey'] ?? null; + $defaultLinkTitle = $this->getDefaultTitle(); - if (!$typeKey) { - throw new InvalidArgumentException( - _t( - __CLASS__ . '.DATA_HAS_NO_TYPEKEY', - '"{class}": $data does not have a typeKey.', - ['class' => static::class], - sprintf('%s: $data does not have a typeKey.', static::class), - ), - ); - } + $this->extend('updateDefaultLinkTitle', $defaultLinkTitle); - $type = LinkTypeService::create()->byKey($typeKey); - - if (!$type) { - throw new InvalidArgumentException( - _t( - __CLASS__ . '.NOT_REGISTERED_LINKTYPE', - '"{class}": "{typekey}" is not a registered Link Type.', - [ - 'class' => static::class, - 'typekey' => $typeKey - ], - sprintf('"%s": "%s" is not a registered Link Type.', static::class, $typeKey), - ), - ); - } + return $defaultLinkTitle; + } - $jsonData = $this; + /** + * This method process the defined singular_name of Link class + * to get the short code of the Link class name. + * + * Or If the name is not defined (by redefining $singular_name in the subclass), + * this use the class name. The Link prefix is removed from the class name + * and the resulting name is converted to lowercase. + * Example: Link => link, EmailLink => email, FileLink => file, SiteTreeLink => sitetree + */ + public function getShortCode(): string + { + return strtolower(rtrim(ClassInfo::shortName($this), 'Link')) ?? ''; + } - if ($this->ClassName !== get_class($type)) { - if ($this->isInDB()) { - $jsonData = $this->newClassInstance(get_class($type)); - } else { - $jsonData = Injector::inst()->create(get_class($type)); + /** + * Get a string representing the versioned state of the link. + */ + public function getVersionedState(): string + { + if (!$this->exists()) { + return 'unsaved'; + } + if ($this->hasExtension(Versioned::class)) { + if ($this->isPublished()) { + if ($this->isModifiedOnDraft()) { + return 'modified'; + } + return 'published'; } + return 'draft'; } + // Unversioned - links are saved in the modal so there is no 'dirty state' and + // when undversioned saved is the same thing as published + return 'unversioned'; + } - foreach ($data as $key => $value) { - if ($jsonData->hasField($key)) { - $jsonData->setField($key, $value); - } + public function onBeforeWrite(): void + { + // Ensure a Sort value is set and that it's one larger than any other Sort value for + // this owner relation so that newly created Links on MultiLinkField's are properly sorted + if (!$this->Sort) { + $this->Sort = self::get()->filter([ + 'OwnerID' => $this->OwnerID, + 'OwnerRelation' => $this->OwnerRelation, + ])->max('Sort') + 1; } - return $jsonData; + parent::onBeforeWrite(); } + /** + * Serialise the link data as a JSON string so it can be fetched from JavaScript. + */ public function jsonSerialize(): mixed { $typeKey = LinkTypeService::create()->keyByClassName(static::class); @@ -237,19 +223,6 @@ public function jsonSerialize(): mixed return $data; } - public function loadLinkData(array $data): Link - { - $link = new static(); - - foreach ($data as $key => $value) { - if ($link->hasField($key)) { - $link->setField($key, $value); - } - } - - return $link; - } - /** * Return a rendered version of this form. * @@ -265,33 +238,6 @@ public function forTemplate() return $this->renderWith([static::class, self::class]); } - /** - * This method should be overridden by any subclasses - */ - public function getURL(): string - { - return ''; - } - - public function getVersionedState(): string - { - if (!$this->exists()) { - return 'unsaved'; - } - if ($this->hasExtension(Versioned::class)) { - if ($this->isPublished()) { - if ($this->isModifiedOnDraft()) { - return 'modified'; - } - return 'published'; - } - return 'draft'; - } - // Unversioned - links are saved in the modal so there is no 'dirty state' and - // when undversioned saved is the same thing as published - return 'unversioned'; - } - /** * Get the owner of this link, if there is one. * @@ -358,6 +304,18 @@ public function can($perm, $member = null, $context = []) }; } + /** + * Get the title that will be displayed if there is no LinkText. + */ + protected function getDefaultTitle(): string + { + $default = $this->getDescription() ?: $this->getURL(); + if (!$default) { + $default = _t(static::class . '.MISSING_DEFAULT_TITLE', '(No value provided)'); + } + return $default; + } + private function canPerformAction(string $canMethod, $member, $context = []) { // Allow extensions to override permission checks @@ -379,51 +337,4 @@ private function canPerformAction(string $canMethod, $member, $context = []) // Default to DataObject's permission checks return parent::$canMethod($member, $context); } - - public function getTitle(): string - { - // If we have link text, we can just bail out without any changes - if ($this->LinkText) { - return $this->LinkText; - } - - $defaultLinkTitle = $this->getDefaultTitle(); - - $this->extend('updateDefaultLinkTitle', $defaultLinkTitle); - - return $defaultLinkTitle; - } - - public function getDefaultTitle(): string - { - $default = $this->getDescription() ?: $this->getURL(); - if (!$default) { - $default = _t(static::class . '.MISSING_DEFAULT_TITLE', '(No value provided)'); - } - return $default; - } - - /** - * This method process the defined singular_name of Link class - * to get the short code of the Link class name. - * Or If the name is not defined (by redefining $singular_name in the subclass), - * this use the class name. The Link prefix is removed from the class name - * and the resulting name is converted to lowercase. - * Example: Link => link, EmailLink => email, FileLink => file, SiteTreeLink => sitetree - */ - public function getShortCode(): string - { - return strtolower(rtrim(ClassInfo::shortName($this), 'Link')) ?? ''; - } - - /** - * The title that will be displayed in the dropdown - * for selecting the link type to create. - * Subclasses should override this. - * It will use the singular_name by default. - */ - public function getMenuTitle(): string - { - return $this->i18n_singular_name(); - } } diff --git a/src/Models/PhoneLink.php b/src/Models/PhoneLink.php index 190553be..a89e9534 100644 --- a/src/Models/PhoneLink.php +++ b/src/Models/PhoneLink.php @@ -31,6 +31,13 @@ public function getCMSFields(): FieldList return parent::getCMSFields(); } + public function getCMSCompositeValidator(): CompositeValidator + { + $validator = parent::getCMSCompositeValidator(); + $validator->addValidator(RequiredFields::create(['Phone'])); + return $validator; + } + public function getDescription(): string { return $this->Phone ?: ''; @@ -49,11 +56,4 @@ public function getMenuTitle(): string { return _t(__CLASS__ . '.LINKLABEL', 'Phone number'); } - - public function getCMSCompositeValidator(): CompositeValidator - { - $validator = parent::getCMSCompositeValidator(); - $validator->addValidator(RequiredFields::create(['Phone'])); - return $validator; - } } diff --git a/src/Models/SiteTreeLink.php b/src/Models/SiteTreeLink.php index c704c935..0dec81b0 100644 --- a/src/Models/SiteTreeLink.php +++ b/src/Models/SiteTreeLink.php @@ -37,18 +37,6 @@ class SiteTreeLink extends Link private static $icon = 'font-icon-page'; - public function getDescription(): string - { - $page = $this->Page(); - if (!$page?->exists()) { - return _t(__CLASS__ . '.PAGE_DOES_NOT_EXIST', 'Page does not exist'); - } - if (!$page->canView()) { - return _t(__CLASS__ . '.CANNOT_VIEW_PAGE', 'Cannot view page'); - } - return $page->URLSegment ?? ''; - } - public function getCMSFields(): FieldList { $this->beforeUpdateCMSFields(function (FieldList $fields) { @@ -110,6 +98,25 @@ public function getCMSFields(): FieldList return parent::getCMSFields(); } + public function getCMSCompositeValidator(): CompositeValidator + { + $validator = parent::getCMSCompositeValidator(); + $validator->addValidator(RequiredFields::create(['PageID'])); + return $validator; + } + + public function getDescription(): string + { + $page = $this->Page(); + if (!$page?->exists()) { + return _t(__CLASS__ . '.PAGE_DOES_NOT_EXIST', 'Page does not exist'); + } + if (!$page->canView()) { + return _t(__CLASS__ . '.CANNOT_VIEW_PAGE', 'Cannot view page'); + } + return $page->URLSegment ?? ''; + } + public function getURL(): string { $page = $this->Page(); @@ -122,18 +129,6 @@ public function getURL(): string return Controller::join_links($url, $anchorSegment, $queryStringSegment); } - public function getDefaultTitle(): string - { - $page = $this->Page(); - if (!$page->exists()) { - return _t(static::class . '.MISSING_DEFAULT_TITLE', '(Page missing)'); - } - if (!$page->canView()) { - return ''; - } - return $page->Title; - } - /** * The title that will be displayed in the dropdown * for selecting the link type to create. @@ -143,10 +138,15 @@ public function getMenuTitle(): string return _t(__CLASS__ . '.LINKLABEL', 'Page on this site'); } - public function getCMSCompositeValidator(): CompositeValidator + protected function getDefaultTitle(): string { - $validator = parent::getCMSCompositeValidator(); - $validator->addValidator(RequiredFields::create(['PageID'])); - return $validator; + $page = $this->Page(); + if (!$page->exists()) { + return _t(static::class . '.MISSING_DEFAULT_TITLE', '(Page missing)'); + } + if (!$page->canView()) { + return ''; + } + return $page->Title; } } diff --git a/tests/php/Models/FileLinkTest.php b/tests/php/Models/FileLinkTest.php index 78569990..e349f796 100644 --- a/tests/php/Models/FileLinkTest.php +++ b/tests/php/Models/FileLinkTest.php @@ -2,6 +2,7 @@ namespace SilverStripe\LinkField\Tests\Models; +use ReflectionMethod; use SilverStripe\Dev\SapphireTest; use SilverStripe\LinkField\Models\FileLink; use SilverStripe\LinkField\Tests\Models\FileLinkTest\TestFileCanView; @@ -18,7 +19,7 @@ class FileLinkTest extends SapphireTest public function testGetDescription(): void { // FileLink without a page - $link = FileLink::create(); + $link = new FileLink(); $this->assertSame('File does not exist', $link->getDescription()); // FileLink with a page though cannot view the page $file = new TestFileCannotView(['Name' => 'not-allowed']); @@ -35,4 +36,26 @@ public function testGetDescription(): void $link->write(); $this->assertSame('file-b.png', $link->getDescription()); } + + public function testGetDefaultTitle(): void + { + $reflectionGetDefaultTitle = new ReflectionMethod(FileLink::class, 'getDefaultTitle'); + $reflectionGetDefaultTitle->setAccessible(true); + + // File does not exist + $link = new FileLink(); + $this->assertSame('(File missing)', $reflectionGetDefaultTitle->invoke($link)); + // File exists in DB but not in filesystem + $file = new TestFileCanView(['Name' => 'My test file']); + $file->write(); + $link->File = $file->ID; + $link->write(); + $this->assertSame('(File missing)', $reflectionGetDefaultTitle->invoke($link)); + // File actually exists + $file->setFromLocalFile(realpath(__DIR__ .'/FileLinkTest/file-b.png'), 'file-b.png'); + $file->write(); + $link->File = $file->ID; + $link->write(); + $this->assertSame('file-b.png', $reflectionGetDefaultTitle->invoke($link)); + } } diff --git a/tests/php/Models/LinkTest.php b/tests/php/Models/LinkTest.php index 5ef9fe85..fc55dc06 100644 --- a/tests/php/Models/LinkTest.php +++ b/tests/php/Models/LinkTest.php @@ -70,7 +70,7 @@ public function testLinkModel(): void { $model = $this->objFromFixture(Link::class, 'link-1'); - $this->assertEquals('FormBuilderModal', $model->LinkTypeHandlerName()); + $this->assertEquals('FormBuilderModal', $model->getLinkTypeHandlerName()); } /** diff --git a/tests/php/Models/SiteTreeLinkTest.php b/tests/php/Models/SiteTreeLinkTest.php index d13f6f7f..fb17cbd7 100644 --- a/tests/php/Models/SiteTreeLinkTest.php +++ b/tests/php/Models/SiteTreeLinkTest.php @@ -2,6 +2,7 @@ namespace SilverStripe\LinkField\Tests\Models; +use ReflectionMethod; use SilverStripe\Dev\SapphireTest; use SilverStripe\LinkField\Models\SiteTreeLink; use SilverStripe\LinkField\Tests\Models\SiteTreeLinkTest\TestSiteTreeCanView; @@ -17,7 +18,7 @@ class SiteTreeLinkTest extends SapphireTest public function testGetDescription(): void { // SiteTreeLink without a page - $link = SiteTreeLink::create(); + $link = new SiteTreeLink(); $this->assertSame('Page does not exist', $link->getDescription()); // SiteTreeLink with a page though cannot view the page $page = new TestSiteTreeCannotView(['URLSegment' => 'test-a']); @@ -35,14 +36,17 @@ public function testGetDescription(): void public function testGetDefaultTitle(): void { + $reflectionGetDefaultTitle = new ReflectionMethod(SiteTreeLink::class, 'getDefaultTitle'); + $reflectionGetDefaultTitle->setAccessible(true); + // Page does not exist - $link = SiteTreeLink::create(); - $this->assertSame('(Page missing)', $link->getDefaultTitle()); + $link = new SiteTreeLink(); + $this->assertSame('(Page missing)', $reflectionGetDefaultTitle->invoke($link)); // Page exists $page = new TestSiteTreeCanView(['Title' => 'My test page']); $page->write(); $link->Page = $page->ID; $link->write(); - $this->assertSame('My test page', $link->getDefaultTitle()); + $this->assertSame('My test page', $reflectionGetDefaultTitle->invoke($link)); } }