Claro's preprocessing of field multiple value form's table header cell removes potential changes by others

Patch #42 works great for me, also (tested 9.4.x). Thank you very much.

Moving to RTBC due to the positive feedback.

1. +++ b/core/themes/claro/claro.theme
   @@ -908,27 +908,13 @@ function claro_preprocess_field_multiple_value_form(&$variables) {
   +    if (isset($variables['table']['#header'][0]['data']['#attributes']) && $variables['table']['#header'][0]['data']['#attributes'] instanceof Attribute) {
   

   We could technically add support for array type of attributes too if that increases the chances of this working

2. +++ b/core/themes/claro/css/components/form.pcss.css
   @@ -37,9 +37,12 @@ tr .form-item,
   +.field-multiple-table .field-label h4.label {
   

   Can we add comment explaining that this is to try to keep the styles working even when there are modules overriding the markup

3. A small CR that warns about the render array changes would probably be useful since we know that there are some themes that would be impacted by this.

Patch updated per #47. Creating CR shortly

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

CR created at https://www.drupal.org/node/3344342 → . Please double check this, I'm not entirely positive that what I wrote is 100% correct.

Could the Issue summary be updated some? The proposed solution mentions there needs to be discussion. If a decision has already been made can it be documented there please.

Thanks!

Agree. Adding the "Needs issue summary update" tag.

Note that since the patch has been created and still needs review, I'm setting this back to "Needs Review". Please see the documentation at https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... →

Needs work is only for:

There is a patch attached to the issue, but the patch needs additional work before it should be reviewed. The author of the patch may indicate that it is incomplete, or the patch has gone through the reviewing process and has received feedback about areas where it needs improvement.

Will leave in review but if the proposed solution isn't clear any reviewer can't review the patch to see if the approach being proposed is being followed.

Since it's been about a week moving back to NW as the issue summary does need to be updated.

Don't want it to rot away in the review queue.

Actually turned into a great improvement for paragraphs

Tested by installing paragraphs and making a dummy bundle with just a simple boolean field

Without the patch. I can't drag and drop the header

With the patch I can!

+++ b/core/themes/claro/claro.theme
@@ -993,27 +993,13 @@ function claro_preprocess_field_multiple_value_form(&$variables) {
+    if (isset($variables['table']['#header'][0]['data']['#attributes']) && $variables['table']['#header'][0]['data']['#attributes']) {

Isn't the isset check here sufficient?

What I was thinking of doing here originally was something like:

// Convert #attributes to \Drupal\Core\Template\Attribute object to allow
// using its utilities for modifying the attributes.
if (!($variables['table']['#header'][0]['data']['#attributes'] instanceof Attribute)) {
  $variables['table']['#header'][0]['data']['#attributes'] = new Attribute($variables['table']['#header'][0]['data']['#attributes']);
}

However, I'm not certain we need to because it looks like we have other instances where we are already assuming that #attributes are instances of \Drupal\Core\Template\Attribute. Because of this, it's probably fine to just remove the second second condition from here.

Removed the second condition per #58. Patch interdiff attached. Thanks!

Attached is the same patch as above but for 9.5.x.

IMO this bug is a major usability issue when paired with the popular Paragraphs module (which is why I'm working on this), and should be ported to 9.5.x.

Get the same results still using #59 so marking it again.

And agree with the comment made in #60 that this makes paragraphs better as it shows a feature that I actually didn't know existed.

We are allowed to change the render arrays in minor releases based on https://www.drupal.org/about/core/policies/core-change-policies/frontend... → so we don't have to implement a BC layer for this. However, we cannot backport this to 9.5.x because of the potential BC concerns.

Committed 90897af and pushed to 10.1.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.