Why this should be an RC target:

Field group module is broken without it. This is also a regression introduced from #1938900: Convert theme_field_ui_table into a template

Problem/Motivation

The Field UI tables is broken in conjunction with field group module. Because field group module creates nested elements in the table. I tracked it down to the #prefix in the table element.

Before

After

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#2 2611758-2.patch881 byteswebflo
#3 2611758-before.jpg185.96 KBwebflo
#3 2611758-after.jpg191.72 KBwebflo
#15 2611758-15.patch4.56 KBswentel
#15 2611758-15-fail.patch3.7 KBswentel

Comments

webflo created an issue. See original summary.

webflo’s picture

webflo commented
Status: Active » Needs review
StatusFileSize
new 2611758-2.patch881 bytes
webflo’s picture

webflo commented
Issue summary: View changes
StatusFileSize
new 2611758-before.jpg185.96 KB
new 2611758-after.jpg191.72 KB
webflo’s picture

webflo commented

The change was introduced in #1938900: Convert theme_field_ui_table into a template.

swentel’s picture

swentel commented
Status: Needs review » Reviewed & tested by the community

Looks sane to me - we should probably also credit zuuperman because of #2605518: Table drag indentation is not rendered - both patches were created independent.

axe312’s picture

axe312 commented

Tested it and it works. RTBC + 1

it-cru’s picture

it-cru
Primary language English
Location Munich
commented

I think this should help others to better find this patch. Perhaps this could be included in issue title ;)

field_group elements prefix with Array

nils.destoop’s picture

nils.destoop commented

Webflo added a reference to the fieldgroup issue.

jcnventura’s picture

jcnventura
Primary language English
commented

Tested also and it fixes the problem. RTBC++

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -rc eligible +Needs tests

This is a runtime code fix and therefore not covered but the rc eligible rules. Also it looks at though we're missing test coverage.

swentel’s picture

swentel commented
Issue tags: +Contributed project blocker

Really, this blocks the one of the top 30 (well, technically top 31) used contrib modules for Drupal - one that is basically ready for release, together with Display Suite. Only those two modules interact on a fundamental level with Field UI that this is really rc eligble. Not having this in, would basically mean people have to patch drupal core on day one it's released, which is just silly.

Will look for tests this evening, unless someone beats me to it the next 8 hours, but after that this is RTBC.

aspilicious’s picture

aspilicious commented

I fully agree with Swentel on this one...

webflo’s picture

webflo commented
Issue summary: View changes
Issue tags: +rc target triage
swentel’s picture

swentel commented
Issue summary: View changes
Related issues: +#1938900: Convert theme_field_ui_table into a template
swentel’s picture

swentel commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2611758-15-fail.patch3.7 KB
new 2611758-15.patch4.56 KB

Test only + fix - took me a while to find a good entry point for this, I almost started porting field group into core, something for 8.1 maybe :p

The last submitted patch, 15: 2611758-15-fail.patch, failed testing.

The last submitted patch, 15: 2611758-15.patch, failed testing.

aspilicious’s picture

aspilicious commented
Status: Needs review » Reviewed & tested by the community

That first testbot fail was very weird and unrelated to this issue...

The last submitted patch, 15: 2611758-15-fail.patch, failed testing.

jcnventura’s picture

jcnventura
Primary language English
commented

RTBC++

nils.destoop’s picture

nils.destoop commented

RTBC++

swentel’s picture

swentel commented
Issue tags: -Contributed project blocker, -rc target triage

No need for those tags anymore I guess. Would be great to see this for the next minor release though.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 2611758-2.patch, failed testing.

webflo’s picture

webflo commented
Status: Needs work » Needs review
aspilicious’s picture

aspilicious commented
Status: Needs review » Reviewed & tested by the community
cameron tod’s picture

cameron tod commented

I have applied this on a D8 project already to use field group, adding to the noise in hoping it gets into 8.0.1.

  • catch committed 5ad5797 on 8.1.x 
    Issue #2611758 by swentel, webflo, zuuperman: Field UI table is broken...

  • catch committed cfe6c4e on 
    Issue #2611758 by swentel, webflo, zuuperman: Field UI table is broken...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x and cherry-picked to 8.0.x, thanks!

Should we have a follow-up to allow #prefix to take render arrays?

andypost’s picture

andypost
he/him
Primary language Russian
commented

Filed #2624324: Allow special case render array elements to take render arrays

Status: Fixed » Closed (fixed)

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

turion’s picture

turion commented

Should this have fixed issue #2749061: Warning: strlen() expects parameter 1 to be string, array given in Drupal\Component\Utility\Unicode::validateUtf8() (line 686 of core/lib/Drupal/Component/Utility/Unicode.php)?