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.

Sign up for GitHub

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

DOC Document including attributes in formfield schema data #630

Merged
merged 1 commit into from
Nov 24, 2024
Merged

DOC Document including attributes in formfield schema data #630

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 23 additions & 0 deletions en/08_Changelogs/6.0.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ title: 6.0.0 (unreleased)
- [`DBDecimal` default value](#dbdecimal-default-value)
- [`RedirectorPage` validation](#redirectorpage-validation)
- [Update JS MIME type, remove `type` in `<script>` tags](#js-mime-type-update)
- [`getSchemaDataDefaults()` now includes attributes](#formfield-schema-data)
- [Full list of removed and changed API (by module, alphabetically)](#api-removed-and-changed)

## Features and enhancements
Expand Down Expand Up @@ -932,6 +933,28 @@ We've updated the MIME type for JavaScript from `"application/javascript"` to `"

This change is generally backward-compatible and should not affect existing functionality. However, if your project explicitly relies on the `type` attribute for `<script>` tags, you may need to adjust accordingly.

### `getSchemaDataDefaults()` now includes attributes {#formfield-schema-data}

The [`FormField::getSchemaDataDefaults()`](api:SilverStripe\Forms\FormField::getSchemaDataDefaults()) method (and by extension the [`getSchemaData()`](api:SilverStripe\Forms\FormField::getSchemaData()) method) now calls [`getAttributes()`](api:SilverStripe\Forms\FormField::getAttributes()). This was done so that attributes such as `placeholder` can be used in the react components that render form fields.

In the past it was common to call `getSchemaData()` inside the `getAttributes()` method, so that a form field rendered in an entwine admin context had the data necessary to bootstrap a react component for that field. Doing that now would result in an infinite recursion loop.
Copy link
Member

Choose a reason for hiding this comment

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

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?

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

Copy link
Member

@emteknetnz emteknetnz Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@GuySartorelli GuySartorelli Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@emteknetnz emteknetnz Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@emteknetnz emteknetnz Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


If you were calling `getSchemaData()` in your `getAttributes()` method in a `FormField` subclass include `$SchemaAttributesHtml` in your template instead. For example:

```diff
-public function getAttributes()
-{
- $attributes = parent::getAttributes();
- $attributes['data-schema'] = json_encode($this->getSchemaData());
- return $attributes;
-}
```

```diff
-<div $AttributesHTML></div>
+<div $AttributesHTML $SchemaAttributesHtml></div>
```

### Full list of removed and changed API (by module, alphabetically) {#api-removed-and-changed}

<!--- Changes below this line will be automatically regenerated -->
Expand Down