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

API Strong typing for the view layer #11351

Merged
merged 1 commit into from
Aug 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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: 0 additions & 7 deletions src/Control/RSS/RSSFeed_Entry.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,6 @@
*/
class RSSFeed_Entry extends ViewableData
{
/**
* The object that represents the item, it contains all the data.
*
* @var mixed
*/
protected $failover;
Copy link
Member Author

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


/**
* Name of the title field of feed entries
*
Expand Down
2 changes: 1 addition & 1 deletion src/Dev/TaskRunner.php
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ public function canInit(): bool
}
return count($this->getTaskList()) > 0;
}

public function providePermissions(): array
{
return [
Expand Down
2 changes: 1 addition & 1 deletion src/Forms/CompositeField.php
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@ public function makeFieldReadonly($field)
return false;
}

public function debug()
public function debug(): string
{
$class = static::class;
$result = "$class ($this->name) <ul>";
Expand Down
8 changes: 3 additions & 5 deletions src/Forms/Form.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

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

These are all nullable because ViewableData::castingHelper() returns null, and most if not all implementations of this method call parent::castingHelper()

{
// Override casting for field message
if (strcasecmp($field ?? '', 'Message') === 0 && ($helper = $this->getMessageCastingHelper())) {
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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();
Expand Down Expand Up @@ -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>";
Expand Down
8 changes: 3 additions & 5 deletions src/Forms/FormField.php
Original file line number Diff line number Diff line change
Expand Up @@ -790,7 +790,7 @@ public function securityTokenEnabled()
return $form->getSecurityToken()->isEnabled();
}

public function castingHelper($field, bool $useFallback = true)
public function castingHelper(string $field, bool $useFallback = true): ?string
{
// Override casting for field message
if (strcasecmp($field ?? '', 'Message') === 0 && ($helper = $this->getMessageCastingHelper())) {
Expand Down Expand Up @@ -1269,7 +1269,7 @@ public function getDescription()
/**
* @return string
*/
public function debug()
public function debug(): string
{
$strValue = is_string($this->value) ? $this->value : print_r($this->value, true);

Expand All @@ -1286,10 +1286,8 @@ public function debug()
/**
* This function is used by the template processor. If you refer to a field as a $ variable, it
* will return the $Field value.
*
* @return string
*/
public function forTemplate()
public function forTemplate(): string
{
return $this->Field();
}
Expand Down
2 changes: 1 addition & 1 deletion src/Forms/FormRequestHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,7 @@ public function validationResult()
return $result;
}

public function forTemplate()
public function forTemplate(): string
{
return $this->form->forTemplate();
}
Expand Down
2 changes: 1 addition & 1 deletion src/Forms/GridField/GridFieldDetailForm_ItemRequest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

@GuySartorelli GuySartorelli Aug 23, 2024

Choose a reason for hiding this comment

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

The DBHtmlText returned from rendering a template will be coerced to string when it's passed in here.
This change is necessary because we also pass the result of forTemplate() into this method, and that's explicitly a string.
We only need the string value in this method so it makes sense to type to that.

{
return new PjaxResponseNegotiator([
'default' => function () use ($renderedForm) {
Expand Down
2 changes: 1 addition & 1 deletion src/Forms/NullableField.php
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ public function setName($name)
/**
* @return string
*/
public function debug()
public function debug(): string
{
$result = sprintf(
'%s (%s: %s : <span style="color: red">%s</span>) = ',
Expand Down
2 changes: 1 addition & 1 deletion src/Forms/ReadonlyField.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public function Type()
return 'readonly';
}

public function castingHelper($field, bool $useFallback = true)
public function castingHelper(string $field, bool $useFallback = true): ?string
{
// Get dynamic cast for 'Value' field
if (strcasecmp($field ?? '', 'Value') === 0) {
Expand Down
2 changes: 1 addition & 1 deletion src/Forms/RequiredFields.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public function removeValidation()
* Debug helper
* @return string
*/
public function debug()
public function debug(): string
{
if (!is_array($this->required)) {
return false;
Expand Down
6 changes: 2 additions & 4 deletions src/ORM/ArrayList.php
Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,8 @@ public function count(): int

/**
* Returns true if this list has items
*
* @return bool
*/
public function exists()
public function exists(): bool
{
return !empty($this->items);
}
Expand Down Expand Up @@ -159,7 +157,7 @@ public function each($callback)
return $this;
}

public function debug()
public function debug(): string
{
$val = "<h2>" . static::class . "</h2><ul>";
foreach ($this->toNestedArray() as $item) {
Expand Down
6 changes: 2 additions & 4 deletions src/ORM/DataList.php
Original file line number Diff line number Diff line change
Expand Up @@ -868,7 +868,7 @@ public function each($callback)
return $this;
}

public function debug()
public function debug(): string
{
$val = "<h2>" . static::class . "</h2><ul>";
foreach ($this->toNestedArray() as $item) {
Expand Down Expand Up @@ -1702,10 +1702,8 @@ public function last()

/**
* Returns true if this DataList has items
*
* @return bool
*/
public function exists()
public function exists(): bool
{
return $this->dataQuery->exists();
}
Expand Down
77 changes: 30 additions & 47 deletions src/ORM/DataObject.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -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());
}
Expand All @@ -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])) {
Expand Down Expand Up @@ -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
Copy link
Member Author

@GuySartorelli GuySartorelli Aug 22, 2024

Choose a reason for hiding this comment

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

Didn't really need to do setField but I was doing getField and hasField so it made sense to do this at the same time.

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
Expand All @@ -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(
Expand All @@ -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;
Expand All @@ -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;
}

/**
Expand Down Expand Up @@ -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) {
Expand All @@ -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}")
);
}

Expand Down Expand Up @@ -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";
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/ORM/DataObjectInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public function delete();
* @param string $fieldName
* @return mixed
*/
public function __get($fieldName);
public function __get(string $property): mixed;

/**
* Save content from a form into a field on this data object.
Expand Down
2 changes: 1 addition & 1 deletion src/ORM/EagerLoadedList.php
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ public function each($callback): static
return $this;
}

public function debug()
public function debug(): string
{
// Same implementation as DataList::debug()
$val = '<h2>' . static::class . '</h2><ul>';
Expand Down
2 changes: 1 addition & 1 deletion src/ORM/FieldType/DBBigInt.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
class DBBigInt extends DBInt
{

public function requireField()
public function requireField(): void
{
$parts = [
'datatype' => 'bigint',
Expand Down
Loading
Loading