Problem/Motivation

IEF displays entity form elements differently than it requires for its data structure (for storage). It does so by moving entity form elements into a fieldset wrapper in a pre_render callback. This makes it much harder to alter the form using general form hooks (instead of IEF-specific hooks) and is most likely no longer required with Drupal 8.

A similar issue was opened for field group (#2669904: Stop altering the form structure), where the solution offered is to use the #group property, and unlike field_group, we only have to deal with the widget side of things, not the formatter:

Its possible to nest form element without changing the actual form structure in D8. Sadly this is only supported in Form API and not in Render API, nevertheless there isa huge benefit because the form structure won't change with field group enabled and makes other form alter way more easier.

The code in \Drupal\inline_entity_form\Element\InlineEntityForm::addFieldsetMarkup already documents that this moving of elements is only applied for rendering the form:

Inline forms use #tree = TRUE to keep their values in a hierarchy for easier storage. Moving the form elements into fieldsets during form building would break up that hierarchy, so it's not an option for entity fields. Therefore, we wait until the pre_render stage, where any changes we make affect presentation only and aren't reflected in $form_state.

Proposed resolution

Use #group to group up entity form elements and make the form pretty, instead of forcibly moving elements around.

Remaining tasks

Decide if we can and want to do this, and if so implement.

CommentFileSizeAuthor
#2 group_elements_without-2884444-2.patch5.28 KBmarkhalliwell

Comments

ckaotik created an issue. See original summary.

markhalliwell’s picture

markhalliwell
Primary language English
commented
Title: Group elements without altering the form structure » Properly handle elements that use #group and don't alter the form structure
Category: Feature request » Bug report
Priority: Minor » Critical
Status: Active » Needs review
Related issues: +#2852792: Add vertical tabs to inline entity form
StatusFileSize
new group_elements_without-2884444-2.patch5.28 KB

There's actually no code (in this project) that is using the custom #fieldset property other than the custom #pre_render callback that implements the "logic". I think it's just safe to remove.

Now, regarding #group. This is a very touchy subject because 1) not a lot of people understand how they actually work and 2) how they actually work isn't very well documented; it's kind of just "magic".

First, the custom inline_entity_form render element does not invoke the callbacks RenderElement::preRenderGroup and RenderElement::processGroup which are necessary to properly handle #group properties which produce things like nested details/fieldset and vertical tabs (see related issue).

Second, the render array returned from an entity form build (and subsequent alter hooks introduced by this module) may not represent the correct #group identifier. The correct #group identifier has since changed when the entity form's became nested and thus any of it's child element's #parents and #array_parents values changed with it. Thus, if one of those said child elements references a #group that is local or was newly introduced to said form, it would be ignored.

The above patch aims to solve all the above. I have documented it quite well so it should be a pretty straightforward patch. Let me know if I need to explain it further.

darvanen’s picture

darvanen
he/him
Primary language English
Location Sydney, Australia
commented
Priority: Critical » Normal
Status: Needs review » Reviewed & tested by the community

Good patch and well explained, thanks. Looks good to me, setting RTBC because the only issue I see is a nit:

+++ b/src/Form/EntityInlineForm.php
@@ -182,9 +182,46 @@ class EntityInlineForm implements InlineFormInterface {
+        $entity_form[$child]['#parents'] = $entity_form[$child]['#tree'] && $entity_form['#tree'] ? array_merge($entity_form['#parents'], [$child]) : [$child];

This is too long for a ternary operation i.m.o.

I don't think this issue is critical, it's not breaking anything, is it? I would even hesitate to call it a bug.

markhalliwell’s picture

markhalliwell
Primary language English
commented
Priority: Normal » Critical

I don't think this issue is critical, it's not breaking anything, is it? I would even hesitate to call it a bug.

Yes, it is and it is definitely a bug.

It implements a custom element type that doesn't call the parent processGroup method it subclasses, so subsequent child elements do not receive the correct parent identifiers.

Thus, when this element is introduced into any sort of "complex" render array, it doesn't conform to normal behavior and breaks existing functionality.

This is especially true when this element is nested inside vertical tabs (which greatly relies on #group).

darvanen’s picture

darvanen
he/him
Primary language English
Location Sydney, Australia
commented

Fair enough, I stand corrected :)

  • bojanz committed 08a9004 on 8.x-1.x authored by markcarver 
    Issue #2884444 by markcarver: Properly handle elements that use #group...
bojanz’s picture

bojanz commented
Status: Reviewed & tested by the community » Fixed

Thanks!

Status: Fixed » Closed (fixed)

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