Description class not added to form-element.html.twig template

Attached patch against drupal 10.1.x

Providing Patch for D9.5.

This looks like it is a bug, however we'll need to fix it in more than just Stable9. I think we need to fix it in:

1. theme: starterkit_theme
2. theme: stable
3. theme: stable9
4. theme: demo_umami > classy
5. module: system

It's already fixed in Olivero and Claro (Claro issue: #3081563: When a form field's description is set to display before the input, it doesn't get the description classes → )

The patch in #15 is the preferred approach. #16 has a space between the div and the expression (<div {{ instead of <div{{.

Addressed the points mentioned in #18.

Modified the approcah IGNORE #19

The last submitted patch, 19: 2700439-19.patch, failed testing. View results →

The last submitted patch, 20: 2700439-20.patch, failed testing. View results →

Removed the changes from the theme: demo_umami > classy as it is causing test failures.

@Rishabh Vishwakarma Thanks for working on this.

I wonder could you move the form-element.html.twig file from umami > classy directory to umami > templates > form directory (I think you'll need to create that directory).

I'm not sure if that will also break the tests, but it's worth trying. If that does break the tests, then we have another (perhaps bigger) issue for Umami if we can add new generic templates that have the same name as templates in the umami > classy directory.

Also, I think it's worth creating this patch against 10.1.x rather than 9.5.x

Created the patch for drupal 10.1.x and also addressed the issue as stated in #24

+++ b/core/profiles/demo_umami/themes/umami/templates/form/form-element.html.twig
@@ -0,0 +1,95 @@
+    <div{{ description.attributes }}>

We need to add the description classes to this item. Once that's done, I think this is ready for RTBC.

Applied changes according to #27

LGTM

The last submitted patch, 28: 2700439-28.patch, failed testing. View results →

Setting to 'needs review' to get the bots to test again.

Think it was a random failure

   {% if description_display == 'before' and description.content %}
-    <div{{ description.attributes }}>
+<div{{ description.attributes.addClass(description_classes) }}>

Why do we change the indentation like this, in multiple files? Earlier versions of the patch don't have this change. Please maintain the same indentation level.

Updated patch #28 by addressing #33, please review it.

Thanks!

indentation seems fixed

1. +++ b/core/modules/system/templates/form-element.html.twig
   --- /dev/null
   +++ b/core/profiles/demo_umami/themes/umami/templates/form/form-element.html.twig
   

   I think we should just move the template out from the Classy directory and then fix it. I don't think we want to have duplicate templates there.

2. +++ b/core/profiles/demo_umami/themes/umami/templates/form/form-element.html.twig
   --- a/core/themes/stable9/templates/form/form-element.html.twig
   +++ b/core/themes/stable9/templates/form/form-element.html.twig
   
   +++ b/core/themes/stable9/templates/form/form-element.html.twig
   @@ -70,7 +70,7 @@
   -    <div{{ description.attributes }}>
   +    <div{{ description.attributes.addClass(description_classes) }}>
   

   Seems fine as a bug fix for Stable 👍

Here's a patch that removes (renames) template from templates/classy/form to templates/form, and fixes the issue there so there's no duplicate template.

The last submitted patch, 37: 2700439-37.patch, failed testing. View results →

This has failed because the template was removed from the classy directory but I can't find the test to remove the template name from the list of classy templates - https://git.drupalcode.org/project/drupal/-/tree/10.1.x/core/tests/Drupa... (no test in here for ConfirmClassyCopiesTest)

It exists in 9.5.x: https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/tests/Drupal.... I sent the patch to be re-tested in 10.1.x.

Oh great, thanks @lauriii

Reviewed the patch manually and the fixes are working as expected. Attached screenshot for reference.

Before patch applied:

After Patch applied:

The patch can be move for RTBC.

The last submitted patch, 37: 2700439-37.patch, failed testing. View results →

Not sure why the bots reset this, moving back to RTBC

The last submitted patch, 37: 2700439-37.patch, failed testing. View results →

I am told that they've had a random test failure explosion the past few days.

The last submitted patch, 37: 2700439-37.patch, failed testing. View results →

Random

I think it's because of the 9.5 tests failing. Hopefully we'll get this committed soon and not need to keep resetting it.

The patch is failing in 9.5 with what looks like a relevant failure from this issue.

If this should be applied to 9.5, we need a 9.5 patch too.

Thanks folks

#37 was applying on 9.5.x after the latest pull, adding a patch for 9.5.x for the same.

Umami has that template already. Under templates/classy.

@larowlan and @lauriii I'm not fully sure how to fix this for 9.5.x

If we leave the template for form-element.html.twig inside umami/templates/classy then we have a duplicate template (like we have in #52 and #23 (we're starting to go around in circles).
If we move it from there (like in the patch I am attaching to this, the test for classy templates will fail unless we remove form-element.html.twig from the ConfigClassyCopiesTest test. I have removed that in this test, but now it will mean that all other themes in 9.5 that have a classy folder will fail, since they will have a form-element.html.twig file which the test says they should not have.

My proposal: commit my patch from #37 so we have D10 fixed and either figure out how to solve it for D9, or don't worry about it as D9 is EOL in November.

There's no interdiff with this patch, as it's purely for 9.5 instead of D10 which #37 solves.

Thanks @markconroy - I'll defer to @lauriii in his FEFM capacity w.r.t what his preference would be

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

This seems like a good fix for Stable and Classy, but since we are adding additional classes for some use cases, it may have an impact on existing themes. Because of the changes to Classy and Stable, I think we may want to err on the side of caution and refrain from backporting to 10.0.x and 9.5.x.

unable to reproduce due to missing steps. tried patch applied successfully not tested yet.

liam morland → made their first commit to this issue’s fork.

Shouldn't remove a tag without saying why.

But since the entire issue summary needs updating (steps to reproduce included) I've added that tag.

Twig template change will require a CR as well.

Thanks.

Created the Draft Change record for the issue. Feel free to edit the description. The screenshot needs to be updated.

Change Record: https://www.drupal.org/node/3482370 →

I have updated the change record.

I'd recommend removing the part about this applies to the core templates and themes. Purpose of the CR is to announce a change that other themes may won't to adapt if they have overridden or included in their base theme.

Would also recommend a simple before/after snippet of the change and maybe mention the classes that will now be applied.

Change record updated.

Cleaning up issue and updated CR.