[FormField] Add new propertyNameSuffix to + Consistency between open & close

[WedgeSama]

Allow to set a specific suffix to all FormField (tab, fieldset, row and column) in place of a random Ulid.

Still use Ulid if nothing given.

The suffix allow to easily override template by knowing the form name (instead of random one).

The suffix given is kept between open and closed type.

The suffix must be valid like a SF form name.

Usage example:

    public function configureFields(string $pageName): iterable
    {
        $fieldset = FormField::addFieldset('A super title', propertySuffix: 'foo');
        $fieldset = FormField::addFieldset('A super title')
            ->setPropertySuffix('foo')
        ;
    }

{% block _MyEntity_ea_form_fieldset_foo_row %}
    {# do stuff #}
    {{ block('ea_form_fieldset_open_row') }}
    {# do other stuff #}
{% endblock _MyEntity_ea_form_fieldset_foo_row %}

{% block _MyEntity_ea_form_fieldset_close_foo_row %}
    {# do stuff #}
    {{ block('ea_form_fieldset_close_row') }}
    {# do other stuff #}
{% endblock _MyEntity_ea_form_fieldset_close_foo_row %}

[bytes-commerce]

I'd assume you have fixed this for the same reason that I have created the PR below - multiple fields with same names/ids cause issues.

See my PR that adds Uniqid by default to tabs to prevent said issues. #6517

[WedgeSama]

I'd assume you have fixed this for the same reason that I have created the PR below - multiple fields with same names/ids cause issues.

See my PR that adds Uniqid by default to tabs to prevent said issues. #6517

Nop, the case I wanted to address is to allow to remove any ids generation and replace it by a known value. With in mind, overriding template capability.

I started to do some PRs to use more native SF form capabilities ( #6536 )

IMHO, we should not rely on any id generation to create SF Form, it prevent native form functionalities like form template overriding to be used.

I did not encountered your case yet. (Am I lucky? ;) ) And after a quick look at your PR, I think there are different.

[bytes-commerce]

Well, lets reverse it; I'd assume that this feature has the potential to fix my experienced issue as well when delivered in a broader scale (for every field). 👍

[javiereguiluz]

@WedgeSama thanks for this contribution. I like it but I have a couple of suggestions.

This feature looks the same as Symfony Form's block_prefix option: https://symfony.com/doc/current/form/form_themes.html#custom-fragment-naming-for-individual-fields

So, I propose the following changes:

• Rename propertyNameSuffix as blockPrefix or formBlockPrefix
• Don't change the FieldDto::getProperty() method but add a FieldDto::getBlockName() or FieldDto::getFormBlockName() that does the same

If you agree, I can make this changes for you while merging. Cheers!

[WedgeSama]

The PR does more then that:

1. It change the form name to remove Ulid if set
2. Allow template overriding
3. More template overriding: keep the block_prefix settable too by FormField::setFormTypeOption('block_prefix', 'foobar'), having both name and block_prefix to do template overriding can be useful.
4. It "forward" the suffix (Ulid or the set value) to closing forms (fieldset, row, etc...)

IMO, the important features of the PR are the 2. and the 4.

I thought of somethink to change the PR with your suggestions while keeping the keys features:

• adding the blockPrefix
• use it for the closing form with a suffix _close to keep the consistency between open and close form (but only block_prefix, the names will not have the consistency)
• dont change getProperty

This will solve the 2. & 4. but not 1. nor 3. And 3. will be hard (maybe impossible without 1.?) but can live without it.

@javiereguiluz if your OK with those behaviors, I'll do the changes 😉

PS: Still can address the 1. in another PR but maybe we need to discuse more of how we can do it

[javiereguiluz]

The main problem here is that I don't know the Symfony Form component well 😐 so I'm missing things.

In any case, there's only one thing in your PR that scares me. This change:

The getProperty() method is used everywhere and it always must be the real property name in the related Doctrine entity (except for the special "properties" used in the forms and that use the random ULIDs suffixes). It scares me a lot that changing this method will break things 😓

Just asking:

Instead of changing getProperty() and adding getRealPropertyName() to keep the code of the original getProperty() method ...

... could we keep getProperty() intact and add a getPropertyNameForForms() or getPropertyNameWithSuffix() or something like that with the new behavior?

Thanks

[WedgeSama]

True, I did not see that the getProperty() was used everywhere, I understand

I created getPropertyNameWithSuffix() like you suggested, and call it only if a FormField is used, when build the form.

With this, I can keep all features listed above + preserve getProperty() as it is.

I think we got something now @javiereguiluz 😉

PS : Plus I added more tests ^^

[WedgeSama]

@javiereguiluz let me know if you need anything about the PR 😉

[javiereguiluz]

Benjamin, sorry it took so long to merge this.

Thanks again for contributing it ... and thanks too for adding tests 🙇

While merging I added some docs for the feature: 4718260

[WedgeSama]

Yes you are right, completely forget to add doc with it🤦

Thanks ^^

[Ollie222]

These changes look quite useful however have they introduced a problem with the default templates when you have multiple fieldsets?

In EasyAdminBundle/crud/detail.html.twig the macro render_fieldset_open(field) uses field.property for the fieldset id and the button uses this to target it.

The id is now the same ea_form_fieldset for all fieldsets with no mention of the custom property siffix or the random ulid one thus if you have multiple fieldsets clicking the button on one of them opens or closes all fieldsets.

Changing the 3 references of {{ field.property }} to {{ field.propertyNameWithSuffix }} looks to restore normal functionality.

[WedgeSama]

Thanks @Ollie222 you are right.

I open a PR to fix this: #6648

If you can test it too, I will appreciate 😉

[Ollie222]

@WedgeSama all done and tested, looks good to me.