-
Notifications
You must be signed in to change notification settings - Fork 821
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
API Strong typing for the view layer #11351
Changes from all commits
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 |
---|---|---|
|
@@ -521,7 +521,7 @@ public function setFieldMessage( | |
return $this; | ||
} | ||
|
||
public function castingHelper($field, bool $useFallback = true) | ||
public function castingHelper(string $field, bool $useFallback = true): ?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. These are all nullable because |
||
{ | ||
// Override casting for field message | ||
if (strcasecmp($field ?? '', 'Message') === 0 && ($helper = $this->getMessageCastingHelper())) { | ||
|
@@ -1547,10 +1547,8 @@ public function getData() | |
* | ||
* This is returned when you access a form as $FormObject rather | ||
* than <% with FormObject %> | ||
* | ||
* @return DBHTMLText | ||
*/ | ||
public function forTemplate() | ||
public function forTemplate(): 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. These should all return an explicit string. This is the raw HTML that will be directly input into the parent template. |
||
{ | ||
if (!$this->canBeCached()) { | ||
HTTPCacheControlMiddleware::singleton()->disableCache(); | ||
|
@@ -1750,7 +1748,7 @@ public function removeExtraClass($class) | |
return $this; | ||
} | ||
|
||
public function debug() | ||
public function debug(): string | ||
{ | ||
$class = static::class; | ||
$result = "<h3>$class</h3><ul>"; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -968,7 +968,7 @@ private function getModelName(): string | |
* Get Pjax response negotiator so form submission mirrors other form submission in the CMS. | ||
* See LeftAndMain::getResponseNegotiator() | ||
*/ | ||
private function getResponseNegotiator(DBHTMLText $renderedForm): PjaxResponseNegotiator | ||
private function getResponseNegotiator(string $renderedForm): PjaxResponseNegotiator | ||
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. The |
||
{ | ||
return new PjaxResponseNegotiator([ | ||
'default' => function () use ($renderedForm) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -816,10 +816,8 @@ public function defineMethods() | |
* Returns true if this object "exists", i.e., has a sensible value. | ||
* The default behaviour for a DataObject is to return true if | ||
* the object exists in the database, you can override this in subclasses. | ||
* | ||
* @return boolean true if this object exists | ||
*/ | ||
public function exists() | ||
public function exists(): bool | ||
{ | ||
return $this->isInDB(); | ||
} | ||
|
@@ -2687,7 +2685,7 @@ public function getFrontEndFields($params = null) | |
return $untabbedFields; | ||
} | ||
|
||
public function getViewerTemplates($suffix = '') | ||
public function getViewerTemplates(string $suffix = ''): array | ||
{ | ||
return SSViewer::get_templates_by_class(static::class, $suffix, $this->baseClass()); | ||
} | ||
|
@@ -2714,11 +2712,8 @@ public function CMSEditLink() | |
/** | ||
* Gets the value of a field. | ||
* Called by {@link __get()} and any getFieldName() methods you might create. | ||
* | ||
* @param string $field The name of the field | ||
* @return mixed The field value | ||
*/ | ||
public function getField($field) | ||
public function getField(string $field): mixed | ||
{ | ||
// If we already have a value in $this->record, then we should just return that | ||
if (isset($this->record[$field])) { | ||
|
@@ -2929,12 +2924,8 @@ public function isChanged($fieldName = null, $changeLevel = DataObject::CHANGE_S | |
/** | ||
* Set the value of the field | ||
* Called by {@link __set()} and any setFieldName() methods you might create. | ||
* | ||
* @param string $fieldName Name of the field | ||
* @param mixed $val New field value | ||
* @return $this | ||
*/ | ||
public function setField($fieldName, $val) | ||
public function setField(string $fieldName, mixed $value): static | ||
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. Didn't really need to do Changed parameter name is to be consistent with the parent class naming so that named arguments can be used more reliably. That will be a theme with some of these. |
||
{ | ||
$this->objCacheClear(); | ||
//if it's a has_one component, destroy the cache | ||
|
@@ -2953,42 +2944,42 @@ public function setField($fieldName, $val) | |
if ($schema->unaryComponent(static::class, $fieldName)) { | ||
unset($this->components[$fieldName]); | ||
// Assign component directly | ||
if (is_null($val) || $val instanceof DataObject) { | ||
return $this->setComponent($fieldName, $val); | ||
if (is_null($value) || $value instanceof DataObject) { | ||
return $this->setComponent($fieldName, $value); | ||
} | ||
// Assign by ID instead of object | ||
if (is_numeric($val)) { | ||
if (is_numeric($value)) { | ||
$fieldName .= 'ID'; | ||
} | ||
} | ||
|
||
// Situation 1: Passing an DBField | ||
if ($val instanceof DBField) { | ||
$val->setName($fieldName); | ||
$val->saveInto($this); | ||
if ($value instanceof DBField) { | ||
$value->setName($fieldName); | ||
$value->saveInto($this); | ||
|
||
// Situation 1a: Composite fields should remain bound in case they are | ||
// later referenced to update the parent dataobject | ||
if ($val instanceof DBComposite) { | ||
$val->bindTo($this); | ||
$this->setFieldValue($fieldName, $val); | ||
if ($value instanceof DBComposite) { | ||
$value->bindTo($this); | ||
$this->setFieldValue($fieldName, $value); | ||
} | ||
// Situation 2: Passing a literal or non-DBField object | ||
} else { | ||
$this->setFieldValue($fieldName, $val); | ||
$this->setFieldValue($fieldName, $value); | ||
} | ||
return $this; | ||
} | ||
|
||
private function setFieldValue(string $fieldName, mixed $val): void | ||
private function setFieldValue(string $fieldName, mixed $value): void | ||
{ | ||
$schema = static::getSchema(); | ||
// If this is a proper database field, we shouldn't be getting non-DBField objects | ||
if (is_object($val) && !($val instanceof DBField) && $schema->fieldSpec(static::class, $fieldName)) { | ||
if (is_object($value) && !($value instanceof DBField) && $schema->fieldSpec(static::class, $fieldName)) { | ||
throw new InvalidArgumentException('DataObject::setFieldValue: passed an object that is not a DBField'); | ||
} | ||
|
||
if (!empty($val) && !is_scalar($val)) { | ||
if (!empty($value) && !is_scalar($value)) { | ||
$dbField = $this->dbObject($fieldName); | ||
if ($dbField && $dbField->scalarValueOnly()) { | ||
throw new InvalidArgumentException( | ||
|
@@ -3001,12 +2992,12 @@ private function setFieldValue(string $fieldName, mixed $val): void | |
} | ||
|
||
// if a field is not existing or has strictly changed | ||
if (!array_key_exists($fieldName, $this->original ?? []) || $this->original[$fieldName] !== $val) { | ||
if (!array_key_exists($fieldName, $this->original ?? []) || $this->original[$fieldName] !== $value) { | ||
// At the very least, the type has changed | ||
$this->changed[$fieldName] = DataObject::CHANGE_STRICT; | ||
|
||
if ((!array_key_exists($fieldName, $this->original ?? []) && $val) | ||
|| (array_key_exists($fieldName, $this->original ?? []) && $this->original[$fieldName] != $val) | ||
if ((!array_key_exists($fieldName, $this->original ?? []) && $value) | ||
|| (array_key_exists($fieldName, $this->original ?? []) && $this->original[$fieldName] != $value) | ||
) { | ||
// Value has changed as well, not just the type | ||
$this->changed[$fieldName] = DataObject::CHANGE_VALUE; | ||
|
@@ -3017,7 +3008,7 @@ private function setFieldValue(string $fieldName, mixed $val): void | |
} | ||
|
||
// Value is saved regardless, since the change detection relates to the last write | ||
$this->record[$fieldName] = $val; | ||
$this->record[$fieldName] = $value; | ||
} | ||
|
||
/** | ||
|
@@ -3048,7 +3039,7 @@ public function setCastedField($fieldName, $value) | |
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function castingHelper($field, bool $useFallback = true) | ||
public function castingHelper(string $field, bool $useFallback = true): ?string | ||
{ | ||
$fieldSpec = static::getSchema()->fieldSpec(static::class, $field); | ||
if ($fieldSpec) { | ||
|
@@ -3073,19 +3064,16 @@ public function castingHelper($field, bool $useFallback = true) | |
* Returns true if the given field exists in a database column on any of | ||
* the objects tables and optionally look up a dynamic getter with | ||
* get<fieldName>(). | ||
* | ||
* @param string $field Name of the field | ||
* @return boolean True if the given field exists | ||
*/ | ||
public function hasField($field) | ||
public function hasField(string $fieldName): bool | ||
{ | ||
$schema = static::getSchema(); | ||
return ( | ||
array_key_exists($field, $this->record ?? []) | ||
|| array_key_exists($field, $this->components ?? []) | ||
|| $schema->fieldSpec(static::class, $field) | ||
|| $schema->unaryComponent(static::class, $field) | ||
|| $this->hasMethod("get{$field}") | ||
array_key_exists($fieldName, $this->record ?? []) | ||
|| array_key_exists($fieldName, $this->components ?? []) | ||
|| $schema->fieldSpec(static::class, $fieldName) | ||
|| $schema->unaryComponent(static::class, $fieldName) | ||
|| $this->hasMethod("get{$fieldName}") | ||
); | ||
} | ||
|
||
|
@@ -3233,7 +3221,7 @@ public function canCreate($member = null, $context = []) | |
* | ||
* @return string HTML data representing this object | ||
*/ | ||
public function debug() | ||
public function debug(): string | ||
{ | ||
$class = static::class; | ||
$val = "<h3>Database record: {$class}</h3>\n<ul>\n"; | ||
|
@@ -4406,13 +4394,8 @@ public function provideI18nEntities() | |
/** | ||
* Returns true if the given method/parameter has a value | ||
* (Uses the DBField::hasValue if the parameter is a database field) | ||
* | ||
* @param string $field The field name | ||
* @param array $arguments | ||
* @param bool $cache | ||
* @return boolean | ||
*/ | ||
public function hasValue($field, $arguments = null, $cache = true) | ||
public function hasValue(string $field, array $arguments = [], bool $cache = true): bool | ||
{ | ||
// has_one fields should not use dbObject to check if a value is given | ||
$hasOne = static::getSchema()->hasOneComponent(static::class, $field); | ||
|
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.
This property is already defined in
ViewableData