search

getgrav / grav-plugin-form

Grav Form Plugin
http://getgrav.org
MIT License
53 stars 79 forks source link

Breaking change in prepare_form_fields() v5.1.0 #530

Closed adrianw closed 3 years ago

adrianw commented 3 years ago

The way prepare_form_fields() has been developed I believe that there's a potential breaking change to form blueprints.

The blueprints now have name as a required field for every field types, even though they are not really needed as they are for cosmetic of information, for example field type 'spacer', it previously didn't require a name definition, now since v5.1.0 if it doesn't have a name field it won't be rendered.

mahagr commented 3 years ago

Do you have an example of this? I'm asking because this is the old code:

https://github.com/getgrav/grav-plugin-form/blob/5.0.3/templates/forms/default/form.html.twig#L71

So it looks like that the spacers without names only work if they are nested.

Generally, I don't use the old format of a list of fields, but with the key, which automatically acts as a field name. Nevertheless, I think line 71 is a bug, so I will look into this.

adrianw commented 3 years ago

This is just a snippet of the first spacer declaration that is skipped, the full form has quite a lot of data-options@ fields so doesn't work without lots of other bits

partial-form.md

Also, if it's a numeric field doesn't both halves need casting eg.

      $name = (string)$field['name'] ?? (string)$name;

or it'll fail on the next test as its not a string.

mahagr commented 3 years ago

Can you try the form with the above fix?

adrianw commented 3 years ago

Can you try the form with the above fix?

Hi Sorry I think I was editing at the same time as this comment, I tried with the original fix but as mentioned above if fails because the next test bails because it's not a string, casting the $field['name'] as well (the value of field[name] is 0 ) fixes this

mahagr commented 3 years ago

OK, I will make that further fix. Your fix doesn't work as it breaks nested fields (starting with a dot).

adrianw commented 3 years ago

I wasn't sure of knock on effects, just knew it resolved my particular form layout, just let me know if when you have something you want me to test.

mahagr commented 3 years ago

See the commit above ^

adrianw commented 3 years ago

Yep that's what I'd changed my local copy to already and can confirm it's fine with this particular style of building forms. It would I guess be good to have some examples of the . way of building forms on the grav learn, this was an old project I was updating so these forms had been built quite some time ago.

mahagr commented 3 years ago

A lot of it is already documented in:

https://learn.getgrav.org/17/advanced/grav-development/grav-16-upgrade-guide https://learn.getgrav.org/17/advanced/grav-development/grav-17-upgrade-guide

That said, the old way to list the fields should still work, so this one was a bug.

A lot of the documentation has also been updated to include field names as a key as it allows easier modification if you override things.