API Strong typing for the view layer #11351

GuySartorelli · 2024-08-22T02:34:26Z

I want a good base of known types for things that will be going into the template layer before I start abstracting it, and this gets me part of the way there.

Issue

Abstract template layer into its own dedicated module #11322

src/View/ViewableData.php

GuySartorelli · 2024-08-22T02:44:09Z

src/View/ViewableData.php

-    /**
-     * Return a single-item iterator so you can iterate over the fields of a single record.
-     *
-     * This is useful so you can use a single record inside a <% control %> block in a template - and then use
-     * to access individual fields on this object.
-     *
-     * @deprecated 5.2.0 Will be removed without equivalent functionality
-     *
-     * @return ArrayIterator
-     */
-    public function getIterator(): Traversable
-    {
-        Deprecation::notice('5.2.0', 'Will be removed without equivalent functionality');
-        return new ArrayIterator([$this]);
-    }


Unrelated but seems sensible to just get this out of the way now - I'll need to know that things work as expected without this method there for the refactor anyway.

src/View/ViewableData.php

GuySartorelli · 2024-08-22T02:46:25Z

src/View/ViewableData.php

-class ViewableData implements IteratorAggregate
+class ViewableData


See #11351 (comment)

GuySartorelli

All changes to use statements are unrelated refactoring, removing unused use statements.

src/Core/Exception/NotImplementedException.php

GuySartorelli · 2024-08-22T21:49:39Z

src/ORM/FieldType/DBBoolean.php

            ->setEmptyString($anyText);
    }

-    public function nullValue()
+    public function nullValue(): ?int


Note that I have kept the following methods nullable for all DBField implementations intentionally:

getValue()

nullValue()

prepValueForDB()

This is so that subclasses can intentionally override these methods to return null. In the past I've had a business requirement for a nullable boolean and a nullable int (i.e. it's null until a content author explicitly selects an option rather than defaulting to 0 or false).

For that to work, these must be nullable types, even if in this implementation it's explicitly returning a non-null value.

GuySartorelli · 2024-08-22T21:50:52Z

src/Control/RSS/RSSFeed_Entry.php

-     *
-     * @var mixed
-     */
-    protected $failover;


This property is already defined in ViewableData

GuySartorelli · 2024-08-22T21:52:54Z

src/Forms/Form.php

@@ -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


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

GuySartorelli · 2024-08-22T21:59:19Z

src/Forms/Form.php

     */
-    public function forTemplate()
+    public function forTemplate(): string


These should all return an explicit string. This is the raw HTML that will be directly input into the parent template.

GuySartorelli · 2024-08-22T22:00:45Z

src/ORM/DataObject.php

     */
-    public function setField($fieldName, $val)
+    public function setField(string $fieldName, mixed $value): static


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.

GuySartorelli · 2024-08-22T22:02:06Z

src/ORM/FieldType/DBClassName.php


-    private static $index = true;
+    private static string|bool $index = true;


See the PHPDoc for this property in DBField for the reason behind this typing.

GuySartorelli · 2024-08-22T22:03:38Z

src/ORM/FieldType/DBClassName.php

     */
-    public function setBaseClass($baseClass)
+    public function setBaseClass(?string $baseClass): static


Nullable param to allow resetting to the default value since the property itself is nullable

GuySartorelli · 2024-08-22T22:12:30Z

src/ORM/FieldType/DBComposite.php

-     *
-     * @var array|DataObject
     */
-    protected $record = [];
+    protected array|ViewableData $record = [];


Since this wasn't strongly typed before, I'm hesitant to jump straight to typing it to DataObject, since we don't really know how these are being used in the wild.
I'm cautiously typing against ViewableData instead. I've checked and everywhere that I've gone for ViewableData instead of DataObject is safe in terms of what methods are being called.

GuySartorelli · 2024-08-22T22:13:18Z

src/ORM/FieldType/DBComposite.php

-        return $this->config()->composite_db;
+        return static::config()->get('composite_db');


Unrelated refactoring to align with best practices. There are a few other places where I've made similar changes.

GuySartorelli · 2024-08-22T22:14:15Z

src/ORM/FieldType/DBComposite.php

     */
-    public function bindTo($dataObject)
+    public function bindTo(DataObject $dataObject): void


Unlike the places where I've gone for ViewableData, this one does seem to need to be DataObject explicitly. There's a conditional check elsewhere that only calls this method if the record is an instance of DataObject.

GuySartorelli · 2024-08-22T23:00:23Z

src/ORM/FieldType/DBCurrency.php

-    public function __construct($name = null, $wholeSize = 9, $decimalSize = 2, $defaultValue = 0)
-    {
-        parent::__construct($name, $wholeSize, $decimalSize, $defaultValue);
-    }


This constructor is identical to the one in the parent class.

GuySartorelli · 2024-08-22T23:01:30Z

src/ORM/FieldType/DBDatetime.php

     */
-    public function setImmutable(bool $immutable): DBDatetime
+    protected static ?DBDatetime $mock_now = null;


Moved this - it used to be hidden below some of the methods.

GuySartorelli · 2024-08-22T23:05:15Z

src/ORM/FieldType/DBDecimal.php

-        $this->defaultValue = number_format((float) $defaultValue, $decimalSize ?? 0);
+        $this->defaultValue = number_format((float) $defaultValue, $this->decimalSize);


Using 0 as a fallback was actually wrong - we should use the value we set for $this->decimalSize

GuySartorelli · 2024-08-22T23:12:02Z

src/ORM/FieldType/DBField.php

-     * Get formfield schema value
-     *
-     * @return string|array Encoded string for use in formschema response
+     * Get formfield schema value for use in formschema response
     */
-    public function getSchemaValue()
+    public function getSchemaValue(): mixed
    {
        return $this->RAW();


The raw value could be anything, so we can't constrain this type to string|array despite what the PHPDoc says.

GuySartorelli · 2024-08-22T23:13:02Z

src/ORM/FieldType/DBFloat.php

    {
        $field = NumericField::create($this->name, $title);
        $field->setScale(null); // remove no-decimal restriction
        return $field;
    }

-    public function nullValue()
+    public function nullValue(): ?int


Arguably should be float|int|null but realistically 0.0 and 0 are identical, and 0.0 is the only float that would make sense here.

GuySartorelli · 2024-08-22T23:13:47Z

src/ORM/FieldType/DBForeignKey.php

-     * @var DataObject
-     */
-    protected $object;
+    protected ?DataObject $object;


Explicitly DataObject rather than ViewableData here unlike other DBField implementations, because the foreignkey represents a relation to a DataObject.

GuySartorelli · 2024-08-22T23:16:23Z

src/ORM/FieldType/DBForeignKey.php

@@ -70,11 +58,11 @@ public function scaffoldFormField($title = null, $params = null)
        return $field;
    }

-    public function setValue($value, $record = null, $markChanged = true)
+    public function setValue(mixed $value, null|array|ViewableData $record = null, bool $markChanged = true): static


Keeping this as ViewableData mostly for consistency, but also because it's conceivable to save a reference to a DataObject into some arbitrary non-database-backed model (e.g. linking some db data up with some 3rd party integration).

GuySartorelli · 2024-08-22T23:17:49Z

src/ORM/FieldType/DBHTMLText.php

+    /**
+     * List of html properties to whitelist
+     */
+    protected array $whitelist = [];


Moved from below, because it was buried below some methods.

GuySartorelli · 2024-08-22T23:33:18Z

src/ORM/FieldType/DBPrimaryKey.php

-    protected $object;
+    protected ?DataObject $object;


Just like with DBForeignKey, this is explicitly intended to represent the ID for a DataObject record so we're using DataObject instead of ViewableData here.

GuySartorelli · 2024-08-22T23:43:53Z

src/View/Parsers/ShortcodeParser.php

-        return $content;
+        return $content ?? '';


This was needed to respect the typehinting of methods that call this and return its value.
It's also just sensible.

GuySartorelli · 2024-08-22T23:44:18Z

src/View/SSTemplateParser.peg

-            $res['php'] .= "->$method('$property', null, true)";
+            $res['php'] .= "->$method('$property', [], true)";


This was needed to respect the typehint for obj() and XML_val() in ViewableData.

GuySartorelli · 2024-08-22T23:44:37Z

src/View/SSTemplateParser.php

Todo comments here were automatically generated from the peg

GuySartorelli · 2024-08-22T23:52:52Z

tests/php/View/ArrayDataTest.php

-    public function testViewabledataItemsInsideArraydataArePreserved()
+    public function testViewableDataItemsInsideArraydataArePreserved()


unrelated refactor that happened as part of a broader search/replace that happened to fix a small typo.

GuySartorelli · 2024-08-23T01:05:29Z

src/Forms/GridField/GridFieldDetailForm_ItemRequest.php

-    private function getResponseNegotiator(DBHTMLText $renderedForm): PjaxResponseNegotiator
+    private function getResponseNegotiator(string $renderedForm): PjaxResponseNegotiator


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.

GuySartorelli · 2024-08-23T01:15:58Z

src/ORM/FieldType/DBBoolean.php

    {
        return 0;
    }

-    public function prepValueForDB($value)
+    public function prepValueForDB(mixed $value): array|int|null


This method must always allow returning an array so that subclasses can do funky custom stuff. See MockDynamicAssignmentDBField in tests for an example of this.

GuySartorelli · 2024-08-25T23:51:22Z

tests/php/View/SSViewerTest.php

@@ -2369,7 +2372,7 @@ public function testPrimitivesConvertedToDBFields()
    public function testMe(): void
    {
        $mockArrayData = $this->getMockBuilder(ArrayData::class)->addMethods(['forTemplate'])->getMock();
-        $mockArrayData->expects($this->once())->method('forTemplate');
+        $mockArrayData->expects($this->once())->method('forTemplate')->willReturn('');


Required to avoid returning null which breaks the type safety of ViewableData::XML_val()

GuySartorelli · 2024-08-25T23:51:58Z

tests/php/View/SSViewerTest.php

Changes to this test are mostly because we're removing the getIterator from ViewableData - so we need to loop over an arraylist instead.

GuySartorelli · 2024-08-26T00:41:58Z

src/View/ViewableData.php

+     * @return object|DBField|null The specific object representing the field, or null if there is no
+     * property, method, or dynamic data available for that field.
+     * Note that if there is a property or method that returns null, a relevant DBField instance will
+     * be returned.
+     */
+    public function obj(
+        string $fieldName,
+        array $arguments = [],
+        bool $cache = false,
+        ?string $cacheName = null
+    ): ?object {


obj() can now return null.

This makes it incredibly clear the distinction between

we have a field or method, but it's returning null (will return an object), and

we have no field or method (will return null)

There were a few problems related to removing the iterator where you'd have <% loop $MissingProperty %> - and since there's no property it returned a DBText by default, and tried to loop over that.
Since we removed the iterator for ViewableData that started throwing an error. But it should neither try to iterate over it (there's nothing to iterate over) nor should it throw an error.

Now, because this method explicitly returns null, the template engine won't try to iterate over the missing value.

GuySartorelli mentioned this pull request Aug 22, 2024

Abstract template layer into its own dedicated module #11322

Open

23 tasks