a2lix / AutoFormBundle

Automate Symfony form building
https://a2lix.fr/bundles/auto-form
MIT License
83 stars 30 forks source link

Respect user's form config #11

Closed supersmile2009 closed 2 years ago

supersmile2009 commented 6 years ago

Changelog

### Fixed
- Fixed a bug where user-configured form fields order was not respected by AutoFormType
### Changed
- AutoFormType should not override or append automatically generated configs to manually configured fields

Subject

I'm addressing 2 issues which I experienced when upgrading to TranslationFormBundle v3, which now requires this bundle. I believe these issues are going to affect anyone who is upgrading to v3, because AutoFormBundle changed translations form behaviour a lot and does not provide at the moment enough control over generated forms when it's needed.

  1. Fields order configured by user is not respected. It renders fields in order as they are extracted from Doctrine metadata. This PR fixes it by appending automatically generated configs to $formOptions['fields'] and not vice-versa.
  2. Fields which already have user-defined config get automatically generated config appended, which is undesired and leads to bugs and exceptions in some cases, because of incorrect config. There is no way to guarantee that automatic config is going to be compatible with user-defined and no control to disable that behaviour. In my opinion the only cases when automatic field config should be allowed are
    • field not defined by user and is automatically added
    • field is defined by user but with no custom config (empty array), which means that field is defined for ordering purpose. Current behaviour is quite harmful on associations fields, where it appends some field configs. It causes issues with packages from Sonata project, in particular when using ModelListType fields. However I believe I may cause issues with a number of other use cases, when user doesn't want that automatic config appended. The changes in this PR implement suggested behaviour to respect user-defined field config.
webda2l commented 6 years ago

About the 1., I deliberately want to keep fields as small as possible to see the important.

The Serializer component as well by example, serialize object properties in the same order of they are listed in the object. From https://github.com/a2lix/Demo/blob/master/app/src/Form/CompanyType.php#L26-L29, I prefer avoid :

                'fields' => [
                    'title' => [],
                    // ...  OTHER_FIELDS => []
                    // ...  OTHER_FIELDS => []
                    'description' => [
                        'disabled' => true,
                    ],
                ],

But, force the order, like above should be nice to have, it's true.

About 2., again, I deliberately want to keep fields as small as possible but it's true that it"s quite harmful on associations fields and it will need a compromise to find.

supersmile2009 commented 6 years ago

Hi @webda2l Thanks for quick response!

  1. Serializer is not a good example here. Serialized data is normally consumed by a machine, so it doesn't need to respect the order. On the other hand - form is something user interacts with. I doubt you'd like a blog post editing form where you first have 'content' field, then a bunch of SEO tags and then 'title'. So preserving order is a really important feature. I don't really see a different practical way to explicitly define fields order. It's convenient enough, it preserves compatibility with existing apps which use your TranslationsFormBundle and upgrade to v3. Of course it can be also done like this, by adding an extra 'forceFieldsOrder' property and adding logic for it:
                'fields' => [
                    // some field configs if necessary
                ],
                'forceFieldsOrder' => [
                    'title',
                    'slug',
                    'content',
                ],

    But then if you need to add some config to ordered fields, you end up configuring them twice:

                'fields' => [
                    'slug' => [ //slug here
                        'help' => 'Some help string.'
                    ],
                ],
                'forceFieldsOrder' => [
                    'title',
                    'slug', // and again slug here
                    'content',
                ],

    So why not to keep it simple and define fields only once?

Lets look at it from another point of view - you want to configure only one field

                'fields' => [
                    'image' => [
                        // some custom configs here
                        'help' => 'Some help string.'
                    ],
                ],

but don't want it to be the first field. Well, it means that you DO care about fields order and will have to configure order anyway. I cannot imagine a situation when anyone in this example scenario would want an image to be "any but not the first field". Because without manually configuring order, its guaranteed by virtually nothing.

That's why I see suggested (old-style) config as the best option here. But If you have other good ideas on how to achieve it, I'll be glad to help with implementation.

  1. Any ideas on a compromise solution? Again, I'll be glad to help, we really need it to work properly in our app and we don't want to keep a patch in our composer.json forever :) The changes I suggest in my PR will add auto-generated field config if field is not added to fields array or if it has empty config. E. g.
                'fields' => [
                    'title' => [], // will use auto config, because user config is empty
                    'image1' => [], // will use auto config
                    'image2' => [
                        // has user-defined config, auto-config will not be added
                        'help' => 'Some help string.'
                    ],
                ],

    Again, it should preserve compatibility with apps upgrading to TranslationsFormBundle v3. Alternatively we can do one of these:

                'fields' => [
                    'image2' => [
                        // a new option in a field config
                        'autoConfig' => false, // disable appending auto config to this field
                        'help' => 'Some help string.'
                    ],
                ],
                // or a new form type option
                'disableAutoConfig' => [
                    'image1',
                    'image2',
                ],

    I'd rather go with the first of the suggested alternatives, because if you don't want auto-config to be appended, it means that you already have a custom field config defined, so you can simply stick 'autoConfig' => false, into it.

Or we can do it with a bundle config to enable/disable this behavior globally.

webda2l commented 6 years ago

ApiPlatform and EasyAdminBundle are two other examples if you want :) Fact is previous versions of TranslationsFormBundle was thought as well to only list fields which require custom configuration and not all fields..

I see two ways:

  1. If the particular case of all valids fields are listed under 'fields' then respect their order for the display. But... not very conventional... so not fan of it :/
  2. Manage a new optional display_weight / display_priority for fields which required particular display order. Conventional concept, used by Symfony for tagged services by example. And so, to keep things simple, all fields have a default display_weight of 0.

With a default order in the Doctrine entity: slug, title, content, and a required display order: title, slug, content, we'll have:

            'fields' => [
                'title' = [ display_weight => -1]
                'slug' => [
                    'help' => 'Some help string.'
                ],
            ]

It sound good to me. WDYT?


About the theme of auto configuration, I mainly see the problem for Many associations with https://github.com/a2lix/AutoFormBundle/blob/master/src/ObjectInfo/DoctrineORMInfo.php#L89-L90. Some could doesn't want _allowadd:true as default, by example. I thought in the past about add a default configuration at high level that users could update.

a2lix_auto_form:
    assoc_many_collection_options_default:        // Naming could be improved maybe..
         allow_add: true
         by_reference: false

It could be enough. Or an additional field as you suggest, like default_config:false could be added too.. to thinking..

Do you see problems with autoconfiguration, in other cases than with Many associations?