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.

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

[TwigBridge] Set Form ID on form element, prevent duplicate form element attributes #48013

Open
wants to merge 1 commit into
base: 7.1
Choose a base branch
from

Conversation

ker0x
Copy link
Contributor

@ker0x ker0x commented Oct 27, 2022

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #42075
License MIT
Doc PR -

As explained in #42075, when a form is rendered, its ID is displayed in the child div of the <form> element.

This prevents to use the form_attr option on FormType.

Moreover, attributes defined on <form> element are duplicated in the child div

For example, the following form

return $this
    ->createFormBuilder(null, [
        'attr' => [
            'class' => 'foo-bar',
            'data-foo' => 'bar'
        ],
    ])
    ->add('input', TextType::class)
    // ...
;

has the following HTML rendering

<form name="form" method="post" class="foo-bar" data-foo="bar">
    <div id="form" class="foo-bar" data-model="form">
         // ... fields
    </div>
</form>

This PR provides the following fixes

  • A new computed variable form_id is available in the view. It is the concatenation of form_ and the ID of the form.
  • The <form> element uses the row_attr option instead of attr to display attributes. The row_attr option was never used on the root form and this avoids duplicating attributes.
  • The form_attr option now only accepts a boolean value.
    • If set on the root form, each child will have a form attribute with the form_id as value.
    • If set on a child, it will have a form attribute with the form_id as value.

@ker0x ker0x changed the title [TwigBridge] Set Form ID on form element, prevent duplicate form elem… [TwigBridge] Set Form ID on form element, prevent duplicate form element attributes Oct 27, 2022
@fabpot fabpot modified the milestones: 6.2, 5.4 Nov 1, 2022
@ker0x
Copy link
Contributor Author

ker0x commented Nov 2, 2022

I just updated the PR with the following changes:

  • A new computed variable form_id is available in the view. It is the concatenation of form_ and the ID of the form.
  • The <form> element uses the row_attr option instead of attr to display attributes. The row_attr option was never used on the root form and this avoids duplicating attributes.
  • The form_attr option now only accepts a boolean value.
    • If set on the root form, each child will have a form attribute with the form_id as value.
    • If set on a child, it will have a form attribute with the form_id as value.

@nicolas-grekas
Copy link
Member

Looks like a rebase is needed. Still up to do it?

@ker0x ker0x force-pushed the bugfix/form-id branch 2 times, most recently from 28f61d7 to 8dd761e Compare April 18, 2023 10:23
@ker0x
Copy link
Contributor Author

ker0x commented Apr 18, 2023

@nicolas-grekas PR has been rebase, however unit tests are failing with high and low deps and I don't know why!

@xabbuh
Copy link
Member

xabbuh commented Apr 21, 2023

looks like you need to update the composer.json file of the Twig bridge to make sure that the Form component is not older than 6.3 (in the require-dev and conflict sections)

@nicolas-grekas
Copy link
Member

I pushed the fix to the composer.json.
Can you please update the description of the PR? I feel like it's a bit out of sync.

Copy link
Contributor

@HeahDude HeahDude left a comment

Choose a reason for hiding this comment

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

Since this issue has been there for a long time already, maybe this should target 6.4 to prevent side effects in the LTS.

@ker0x
Copy link
Contributor Author

ker0x commented Jul 26, 2023

@nicolas-grekas description update 👍

@HeahDude I agree to target 6.4 instead of 5.4

@nicolas-grekas nicolas-grekas modified the milestones: 5.4, 6.4 Jul 26, 2023
@nicolas-grekas
Copy link
Member

target 6.4 instead of 5.4

Would be better I agree. Can you update the target of this PR and rebase on 6.4 please?

@ker0x ker0x changed the base branch from 5.4 to 6.4 July 26, 2023 10:04
Copy link
Member

@xabbuh xabbuh left a comment

Choose a reason for hiding this comment

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

you will need to update the version constraints similarly to what Nicolas did in 3f3d763#diff-33dca46fccec7efe765aa6cf249fddcdde0d15fa2d01adc052a5e1da01b423c5

@@ -1,7 +1,7 @@
{% use "bootstrap_3_layout.html.twig" %}

{% block form_start -%}
{% set attr = attr|merge({class: (attr.class|default('') ~ ' form-horizontal')|trim}) %}
{% set row_attr = row_attr|merge({class: (row_attr.class|default('') ~ ' form-horizontal')|trim}) %}
Copy link
Member

Choose a reason for hiding this comment

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

If we have to make this change here, it seems to me that we need to adapt for custom form types that do not provide the row_attr variable (and that we need to fall back to attr then).

@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@ker0x ker0x force-pushed the bugfix/form-id branch 3 times, most recently from 42ed263 to 16d3589 Compare December 31, 2023 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Form] Form ID is not set on form element
7 participants