API Refactor template layer into its own module #11405

GuySartorelli · 2024-09-26T04:22:36Z

Includes the following large-scale changes:

Impoved barrier between model and view layers
Improved casting of scalar to relevant DBField types
Improved capabilities for rendering arbitrary data in templates

Issue

Abstract template layer into its own dedicated module #11322

src/Model/ModelData.php

GuySartorelli · 2024-10-10T01:23:28Z

src/Model/ModelData.php

-     * Return the default "casting helper" for use when no explicit casting helper is defined.
-     * This helper will be a subclass of DBField. See castingHelper()
-     */
-    protected function defaultCastingHelper(string $field): string


This method was also an early addition to CMS 6 (used to just get the default_cast config directly from castingHelper), so we don't need to deprecate this either.

GuySartorelli · 2024-10-10T02:05:24Z

src/ORM/FieldType/DBField.php

-    public function __toString(): string
-    {
-        return (string)$this->forTemplate();
-    }


Exists on the superclass (which actually does exactly this anyway) so no need to deprecate.

I left some comments about the use of ModelData::__toString() which I'm not really a fan of

Includes the following large-scale changes: - Impoved barrier between model and view layers - Improved casting of scalar to relevant DBField types - Improved capabilities for rendering arbitrary data in templates

emteknetnz

I'm really unsure how central to all of this working the implements Stringable and __toString() and the ModelData::__toString() now rendering a template is, could you please clarify what it's all for

src/Control/Controller.php

src/Control/Email/Email.php

src/Forms/GridField/GridFieldAddExistingAutocompleter.php

emteknetnz · 2024-10-15T02:51:20Z

src/Forms/GridField/GridState.php

@@ -14,7 +15,7 @@
 *
 * @see GridField
 */
-class GridState extends HiddenField
+class GridState extends HiddenField implements Stringable


Is there actually a reason why we're adding implements Stringable all over the place? Or is this unrelated refactoring?

It's unrelated refactoring and recommended best practice when the __toString() method is implemented.
When the CMS 5 PRs are merged I'll be annotating these PRs with that sort of context.

A large PR like this is really not the best PR to be doing unrelated refactoring of this nature

Just having a look at some of the things that got implements Stringable added to them, I can't help thinking we'd be better off converting the __tostring() methods to regular public methods and updating whatever's calling them so it's more obvious what's happening, rather than using a magic method. Not saying we should do that in this PR (we shouldn't).

I'm not sure if you're asking me to change something? Even if you are I think it doesn't affect the CMS 5 PRs, right? Or is this just now a continuation of the discussion in #11405 (comment)?

emteknetnz · 2024-10-15T03:00:55Z

src/Model/ModelData.php

@@ -305,12 +306,18 @@ public function exists(): bool
        return true;
    }

+    public function __toString(): string
+    {
+        return $this->forTemplate();


Won't this risk kind of drastically changing somethings rather negatively, possibly some parts of the debugging experience?

We exactly are we calling forTemplate() in __toString()? This just seems wrong somehow, like it's a misuse of __toString()?

Won't this risk kind of drastically changing somethings rather negatively, possibly some parts of the debugging experience?

If someone is relying on (string) $model instead of get_class($model) they're relying on very flaky behaviour which we already don't respect in DBField which does exactly what this method is doing.
Doing this here accomplishes the following:

Stops people relying on string casting to check the class name, which is flaky at best and breaks down as soon as a subclass wants to return something else from this method

Makes this method's usage more consistent across subclasses - it's taking what was already happening in DBField and making it standard for all models.

We exactly are we calling forTemplate() in __toString()? This just seems wrong somehow, like it's a misuse of __toString()?

https://www.php.net/manual/en/stringable.tostring.php "Returns the string representation of the object."

This method is specifically for this purpose. The rendered markup of a model is the string representation of that model.

This is what other template rendering engines will call to try to get the textual markup that should be injected into templates.

What do you think this method should be used for instead of this?

If someone is relying on (string) $model instead of get_class($model) they're relying on very flaky behaviour which we already don't respect in DBField which does exactly what this method is doing.

I'm not sure how that's relevant. By itself that's not a good reason to implement a magic method here.

it's taking what was already happening in DBField and making it standard for all models.

Having a look at that code, while that does call DBField::forTemplate(), ultimately all that's doing is calling DBField::getValue() which won't be rendering any HTML

The rendered markup of a model is the string representation of that model.

I'm not sure that is correct? While that makes sense in a template context, ModelData is not a class that's exclusively used in templates. Wasn't that why we renamed ViewableData to ModelData, as lots of ViewableData subclasses weren't exclusively used in a template context?

I might be making an incorrect assumption, though won't ModelData::forTemplate() be rendering HTML is some cases, which will be out of context and unhelpful some of the time?

I also just generally disklike magic methods as it's really not clear what's going on. Do we even need __toString() here? Can we not simply just update whatever "calls" thing to simply use ->forTemplate() instead?

I'm not sure how that's relevant. By itself that's not a good reason to implement a magic method here.

The method was already implemented. I'm just changing what it returns.

Having a look at that code, while that does call DBField::forTemplate(), ultimately all that's doing is calling DBField::getValue() which won't be rendering any HTML

I don't understand your point here... is your problem with the implementation of ModelData::__toString() that it returns HTML markup?

won't ModelData::forTemplate() be rendering HTML is some cases

Yes.

which will be out of context and unhelpful some of the time?

In cases where that's not helpful, people probably shouldn't be casting the object to a string. You cast something as a string when you want the string representation of it, usually to present to the end user. In what case would you do that for ModelData outside of presenting it in the browser?

In an API, for example, you'd be sending individual fields, probably as JSON, not sending a string representation of the whole model.

I also just generally disklike magic methods

The method was already there, so if your problem is with the method in general this isn't the place to discuss it.

This doesn't affect the CMS 5 PRs in any case. So feel free to continue this discussion, but it shouldn't block those PRs from being merged.

The abstraction I've done doesn't, in general, rely on this change to __toString(). I just wanted to make this method more standard across ModelData and dissuade people from relying on it for checking the class since that's a really bad way to check the class.

Can we revert it to how it was then? Whether or not it's a good idea, it seems like it's just entirely unrelated to this PR and it's kind of just derailing the actual peer review.

Could also please remove all of the added implements Stringable stuff too, they don't belong in the PR either. My preference for those would be to get rid of the __toString() methods though that's a discussion for an entirely different card

src/View/SSViewer.php

emteknetnz · 2024-10-15T03:45:38Z

src/View/ViewLayerData.php

+use Stringable;
+use Traversable;
+
+class ViewLayerData implements IteratorAggregate, Stringable


Suggested change

class ViewLayerData implements IteratorAggregate, Stringable

class TemplateData implements IteratorAggregate, Stringable

Is TemplateData an accurate name? ViewLayerData is kind of weird and makes me just want to call it ViewableData, which we shouldn't do :-) We don't call things ModelLayerData so seems like we shouldn't include the word 'Layer' here either

It can be used anywhere in the view layer, it doesn't have to be restricted to templates.

We don't call things ModelLayerData so seems like we shouldn't include the word 'Layer' here either

That's because it is the data that represents a model, it's not data for the model layer.
This, on the other hand, is data for the view layer, it doesn't represent the view or the entirety of the view layer.

All of that said, I really don't feel strongly about it, so if you want me to call it TemplateData I can do that. Just know that makes it harder to justify using it for other areas in the view layer later if we want to.

"View layer" is not a term that I've seen previously used. What specifically makes up the "view layer"?

It's often called the "presentation layer" which you've probably heard Jackson reference a few times. It's basically any part of "presenting" data to the end user. So we could use this class to wrap data inside GridField in order to help generate the GridField markup, for example.

emteknetnz · 2024-10-15T03:47:52Z

src/View/ViewLayerData.php

+            throw new InvalidArgumentException('$data must not be null');
+        }
+        if ($data instanceof ViewLayerData) {
+            $data = $data->data;


->data is a private property?

Yes, it's a private property in this class which means this class can access it.

src/View/TemplateEngine.php

GuySartorelli · 2024-10-15T23:07:40Z

I'll make the requested changes after the CMS 5 PRs have been merged and I can rebase on top of them.

GuySartorelli marked this pull request as draft September 26, 2024 04:22

GuySartorelli mentioned this pull request Sep 26, 2024

Abstract template layer into its own dedicated module #11322

Open

23 tasks

GuySartorelli force-pushed the pulls/6/refactor-template-layer branch 3 times, most recently from 649a6fc to da2ebdc Compare September 27, 2024 02:34

GuySartorelli marked this pull request as ready for review September 27, 2024 02:37

GuySartorelli marked this pull request as draft September 27, 2024 02:37

GuySartorelli force-pushed the pulls/6/refactor-template-layer branch 15 times, most recently from 2b00a05 to fff294a Compare October 2, 2024 03:20

GuySartorelli force-pushed the pulls/6/refactor-template-layer branch from fff294a to ad22082 Compare October 2, 2024 03:47

GuySartorelli force-pushed the pulls/6/refactor-template-layer branch 8 times, most recently from 3687523 to 6af9801 Compare October 10, 2024 01:04

GuySartorelli commented Oct 10, 2024

View reviewed changes

src/Model/ModelData.php Show resolved Hide resolved

GuySartorelli commented Oct 10, 2024

View reviewed changes

GuySartorelli force-pushed the pulls/6/refactor-template-layer branch 2 times, most recently from bc9fdfa to d34b708 Compare October 10, 2024 03:50

GuySartorelli mentioned this pull request Oct 10, 2024

ENH Suppress deprecation notices for API we can't avoid silverstripe/silverstripe-contentreview#264

Open

GuySartorelli force-pushed the pulls/6/refactor-template-layer branch from d34b708 to 0f38603 Compare October 10, 2024 22:31

This was referenced Oct 10, 2024

ENH Update code to reflect changes in template layer silverstripe/silverstripe-contentreview#265

Draft

ENH Update code to reflect changes in template layer symbiote/silverstripe-advancedworkflow#556

Draft

GuySartorelli force-pushed the pulls/6/refactor-template-layer branch 3 times, most recently from a9e90cd to 56db608 Compare October 14, 2024 00:29

GuySartorelli force-pushed the pulls/6/refactor-template-layer branch from 56db608 to cd8090d Compare October 14, 2024 01:52

GuySartorelli mentioned this pull request Oct 14, 2024

ENH Update code to reflect changes in template layer silverstripe/silverstripe-mfa#570

Draft

API Refactor template layer into its own module

2512626

Includes the following large-scale changes: - Impoved barrier between model and view layers - Improved casting of scalar to relevant DBField types - Improved capabilities for rendering arbitrary data in templates

GuySartorelli force-pushed the pulls/6/refactor-template-layer branch from cd8090d to 2512626 Compare October 14, 2024 02:56

GuySartorelli mentioned this pull request Oct 14, 2024

ENH Update code to reflect changes in template layer silverstripe/silverstripe-behat-extension#287

Draft

emteknetnz requested changes Oct 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Refactor template layer into its own module #11405

API Refactor template layer into its own module #11405

GuySartorelli commented Sep 26, 2024

GuySartorelli Oct 10, 2024

GuySartorelli Oct 10, 2024

emteknetnz Oct 15, 2024

emteknetnz left a comment

emteknetnz Oct 15, 2024

GuySartorelli Oct 15, 2024

emteknetnz Oct 15, 2024 •

edited

Loading

GuySartorelli Oct 15, 2024 •

edited

Loading

emteknetnz Oct 15, 2024

GuySartorelli Oct 15, 2024

emteknetnz Oct 15, 2024

GuySartorelli Oct 15, 2024

GuySartorelli Oct 15, 2024

GuySartorelli Oct 15, 2024 •

edited

Loading

emteknetnz Oct 15, 2024 •

edited

Loading

emteknetnz Oct 15, 2024

GuySartorelli Oct 15, 2024

emteknetnz Oct 15, 2024

GuySartorelli Oct 15, 2024 •

edited

Loading

emteknetnz Oct 15, 2024

GuySartorelli Oct 15, 2024

GuySartorelli commented Oct 15, 2024

	class ViewLayerData implements IteratorAggregate, Stringable
	class TemplateData implements IteratorAggregate, Stringable

API Refactor template layer into its own module #11405

Are you sure you want to change the base?

API Refactor template layer into its own module #11405

Conversation

GuySartorelli commented Sep 26, 2024

Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

GuySartorelli Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

emteknetnz Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Oct 15, 2024

emteknetnz Oct 15, 2024 •

edited

Loading

GuySartorelli Oct 15, 2024 •

edited

Loading

GuySartorelli Oct 15, 2024 •

edited

Loading

emteknetnz Oct 15, 2024 •

edited

Loading

GuySartorelli Oct 15, 2024 •

edited

Loading