Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
ker0x wants to merge1 commit intosymfony:7.4
base:7.4
Choose a base branch
Loading
fromker0x:bugfix/form-id

Conversation

ker0x
Copy link
Contributor

@ker0xker0x commentedOct 27, 2022•
edited
Loading

QA
Branch?7.1
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#42075
LicenseMIT
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 theform_attr option onFormType.

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

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

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

This PR provides the following fixes

  • A new computed variableform_id is available in the view. It is the concatenation ofform_ and theID of the form.
  • The<form> element uses therow_attr option instead ofattr to display attributes. Therow_attr option was never used on the root form and this avoids duplicating attributes.
  • Theform_attr option now only accepts a boolean value.
    • If set on the root form, each child will have aform attribute with theform_id as value.
    • If set on a child, it will have aform attribute with theform_id as value.

@ker0xker0x 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 attributesOct 27, 2022
@fabpotfabpot modified the milestones:6.2,5.4Nov 1, 2022
@ker0x
Copy link
ContributorAuthor

ker0x commentedNov 2, 2022•
edited
Loading

I just updated the PR with the following changes:

  • A new computed variableform_id is available in the view. It is the concatenation ofform_ and theID of the form.
  • The<form> element uses therow_attr option instead ofattr to display attributes. Therow_attr option was never used on the root form and this avoids duplicating attributes.
  • Theform_attr option now only accepts a boolean value.
    • If set on the root form, each child will have aform attribute with theform_id as value.
    • If set on a child, it will have aform attribute with theform_id as value.

@nicolas-grekas
Copy link
Member

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

@ker0xker0xforce-pushed thebugfix/form-id branch 2 times, most recently from28f61d7 to8dd761eCompareApril 18, 2023 10:23
@ker0x
Copy link
ContributorAuthor

@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

looks like you need to update thecomposer.json file of the Twig bridge to make sure that the Form component is not older than 6.3 (in therequire-dev andconflict 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

@HeahDudeHeahDude 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
ContributorAuthor

@nicolas-grekas description update 👍

@HeahDude I agree to target6.4 instead of5.4

@nicolas-grekasnicolas-grekas modified the milestones:5.4,6.4Jul 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?

@ker0xker0x changed the base branch from5.4 to6.4July 26, 2023 10:04
Copy link
Member

@xabbuhxabbuh 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 in3f3d763#diff-33dca46fccec7efe765aa6cf249fddcdde0d15fa2d01adc052a5e1da01b423c5

@ker0xker0xforce-pushed thebugfix/form-id branch 2 times, most recently from8d6067d toabb301bCompareJuly 26, 2023 15:25
@@ -1,7 +1,7 @@
{% use "bootstrap_3_layout.html.twig" %}

{% block form_start -%}
{% setattr =attr|merge({class: (attr.class|default('') ~ ' form-horizontal')|trim}) %}
{% setrow_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 therow_attr variable (and that we need to fall back toattr then).

@nicolas-grekasnicolas-grekas modified the milestones:6.4,7.1Nov 15, 2023
@ker0xker0xforce-pushed thebugfix/form-id branch 3 times, most recently from42ed263 to16d3589CompareDecember 31, 2023 09:35
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@xabbuhxabbuhxabbuh requested changes

@maxbeckersmaxbeckersmaxbeckers left review comments

@HeahDudeHeahDudeHeahDude approved these changes

@ycerutoycerutoAwaiting requested review from ycerutoyceruto is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.

[Form] Form ID is not set on form element
7 participants
@ker0x@nicolas-grekas@xabbuh@HeahDude@maxbeckers@fabpot@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp