DOC Document including attributes in formfield schema data

[GuySartorelli]

Issue

• Form field attributes aren’t included in form schema silverstripe-framework#9124

[emteknetnz]

It seems like this solution may be worse than the status quo?

This would mean that (to be fair, probably a small number) of sites will get a possibly very hard to diagnose infinite loop when they upgrade to CMS 6, and, they have to put some HTML attributes separately in the template, which seems slightly worse IMO than having them done separately in PHP. In PHP you can at least do things like just do the "manual workaround" in a super class and have it inherited?

[GuySartorelli]

This would mean that (to be fair, probably a small number) of sites will get a possibly very hard to diagnose infinite loop when they upgrade to CMS 6

There's probably all kinds of errors that can occur when doing a major upgrade if you don't take care to read through the changelog and check for instances of the things it calls out.
I think that's to be expected.

they have to put some HTML attributes separately in the template, which seems slightly worse IMO than having them done separately in PHP

They're data attributes that can't be added in another way without stopping attributes from being usable in react form fields. A very minor inconvenience for unlocking what should be a no brainer - setting attributes on the form field and seeing them used in those form fields.

In PHP you can at least do things like just do the "manual workaround" in a super class and have it inherited?

I'm not sure what you mean by this, but people shouldn't have to make subclasses of built in fields just to get setAttribute() to work.

It seems like this solution may be worse than the status quo?

Before, attributes would not be pulled through to the react fields. Now they are. Seems like it's better, not worse?

[emteknetnz]

Would something like this work? It would prevent the need to update templates

// FormField

public function getSchemaDataDefaults(): array
{
    $data = [
        // ...
        'attributes' => array_merge($this->getSchemaAttributes(), $this->getAttributes()),
        // ...
}

private function getSchemaAttributes(): array
{
    return [
        'data-schema' => json_encode($this->getSchemaData()),
        'data-state' => json_encode($this->getSchemaState()),
    ];
}

And then remove 'data-schema' + 'data-state' from all the FormField subclasses getAttributes() ?

May result in adding too many 'data-schema' + 'data-state' attributes to fields that don't actually use them, though could add protected bool $includeSchemaAttributes = false to FormField and set to true on subclasses that need them

[GuySartorelli]

I don't understand how the data-schema and data-state would get to the template, in that case?

You'd also introduce a new infinite loop: getSchemaDataDefaults -> getSchemaAttributes -> getSchemaData -> getSchemaDataDefaults

[emteknetnz]

Oh true :-)

Perhaps just have getSchemaAttributesHTML() as a public method on FormField and in the template just go $SchemaAttributesHTML on the templates that need it, it's a little confusing right now that some of them don't include data-state while some do, easier to just always include it even if it's unused by the react component

[GuySartorelli]

it's a little confusing right now that some of them don't include data-state while some do

That seems fine to me that fields which need it will have it, and those which don't need it don't have it.

Just to clarify... are you asking me to add:

1. a new public method
2. call that method in all form field templates, even if they don't need it

If so, that just seems to me like a worse version of what this PR already does, but I'll do it if that's what you need to approve this PR.

[emteknetnz]

Yeah, add a new public method, and use it on those templates where you've made changes e.g. SearchableDropdownField.ss, though don't add it to FormField templates that simply don't need the schema attributes though.

I think that's an improvement because that way we won't look at this at the templates in the future and go "Why isn't data-schema just in getAttributes()?". Would also be worth adding a comment to the new method saying this exists to prevent an infinite loop

[GuySartorelli]

Done

[emteknetnz]

Merge conflict