-
Notifications
You must be signed in to change notification settings - Fork 14
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
ENH More standardisation #223
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,10 +52,10 @@ public function getURL(): string | |
return $file->exists() ? (string) $file->getURL() : ''; | ||
} | ||
|
||
public function getDefaultTitle(): string | ||
protected function getDefaultTitle(): string | ||
{ | ||
$file = $this->File(); | ||
if (!$file->exists()) { | ||
if (!$file?->exists()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Matches how it's used elsewhere in this class. |
||
return _t(__CLASS__ . '.MISSING_DEFAULT_TITLE', '(File missing)'); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,10 +2,8 @@ | |
|
||
namespace SilverStripe\LinkField\Models; | ||
|
||
use InvalidArgumentException; | ||
use ReflectionException; | ||
use SilverStripe\Core\ClassInfo; | ||
use SilverStripe\Core\Injector\Injector; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not used anymore |
||
use SilverStripe\Forms\FieldList; | ||
use SilverStripe\LinkField\Services\LinkTypeService; | ||
use SilverStripe\ORM\DataObject; | ||
|
@@ -69,17 +67,20 @@ class Link extends DataObject | |
*/ | ||
private static $icon = 'font-icon-link'; | ||
|
||
/** | ||
* Get a short description of the link. | ||
* | ||
* This is displayed in LinkField as an indication of what the link is pointing at. | ||
*/ | ||
public function getDescription(): string | ||
{ | ||
return ''; | ||
} | ||
|
||
public function scaffoldLinkFields(array $data): FieldList | ||
{ | ||
return $this->getCMSFields(); | ||
} | ||
Comment on lines
-77
to
-80
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused code |
||
|
||
public function LinkTypeHandlerName(): string | ||
/** | ||
* Get the react component used to render the modal form. | ||
*/ | ||
public function getLinkTypeHandlerName(): string | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Getters should be prefixed with |
||
{ | ||
return 'FormBuilderModal'; | ||
} | ||
|
@@ -132,91 +133,9 @@ public function onBeforeWrite(): void | |
parent::onBeforeWrite(); | ||
} | ||
|
||
function setData($data): Link | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused method |
||
{ | ||
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(); | ||
} | ||
|
||
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), | ||
), | ||
); | ||
} | ||
|
||
$typeKey = $data['typeKey'] ?? null; | ||
|
||
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), | ||
), | ||
); | ||
} | ||
|
||
$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), | ||
), | ||
); | ||
} | ||
|
||
$jsonData = $this; | ||
|
||
if ($this->ClassName !== get_class($type)) { | ||
if ($this->isInDB()) { | ||
$jsonData = $this->newClassInstance(get_class($type)); | ||
} else { | ||
$jsonData = Injector::inst()->create(get_class($type)); | ||
} | ||
} | ||
|
||
foreach ($data as $key => $value) { | ||
if ($jsonData->hasField($key)) { | ||
$jsonData->setField($key, $value); | ||
} | ||
} | ||
|
||
return $jsonData; | ||
} | ||
|
||
/** | ||
* 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 +156,6 @@ public function jsonSerialize(): mixed | |
return $data; | ||
} | ||
|
||
public function loadLinkData(array $data): Link | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused method |
||
{ | ||
$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. | ||
* | ||
|
@@ -266,13 +172,18 @@ public function forTemplate() | |
} | ||
|
||
/** | ||
* Get the URL this Link links to. | ||
* | ||
* This method should be overridden by any subclasses | ||
*/ | ||
public function getURL(): string | ||
{ | ||
return ''; | ||
} | ||
|
||
/** | ||
* Get a string representing the versioned state of the link. | ||
*/ | ||
public function getVersionedState(): string | ||
{ | ||
if (!$this->exists()) { | ||
|
@@ -394,7 +305,10 @@ public function getTitle(): string | |
return $defaultLinkTitle; | ||
} | ||
|
||
public function getDefaultTitle(): string | ||
/** | ||
* Get the title that will be displayed if there is no LinkText. | ||
*/ | ||
protected function getDefaultTitle(): string | ||
{ | ||
$default = $this->getDescription() ?: $this->getURL(); | ||
if (!$default) { | ||
|
@@ -406,6 +320,7 @@ public function getDefaultTitle(): string | |
/** | ||
* 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. | ||
|
@@ -419,6 +334,7 @@ public function getShortCode(): string | |
/** | ||
* 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. | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
Comment on lines
-21
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests should avoid using injector when they're not explicitly testing injector |
||
$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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Matches a test in |
||
{ | ||
$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)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never called outside of
Link
models except in tests.Everywhere else should use
getTitle()
to get the real title.