Add ability to insert FAPI wrapper element without modifying #parents of children

I'm really not sure whether I like this change for D7. Sure, I've also thought of it in the past - but in practice, this can easily get totally confusing, as you'd have to scan for #tree TRUE, perhaps FALSE, perhaps some NULL in the middle, and any other combination that is possible.

The current behavior of just TRUE and FALSE is at least followable, even in complex form structures, as it's "absolute". #tree NULL introduces relativity.

I do think ideally fieldgroup.module should use #pre_render to do it's magic - that way the $form structure stays consistent for modules. -> That way fieldgroup is no use-case any more.

Ok. So let's think of use-cases:

1) Embed multiple forms of someone else using array parents with #tree == TRUE. With proper #tree behaviour this would be rather easy (subform element bye bye) as long as the forms contain no #ajax stuff. Yes you would have to care about the #tree defaults of the embedded form as of now, but at least it would work if #tree == FALSE would be fixed. With only having NULL you'd also have to make sure any embedded form element uses NULL instead of FALSE.. ;)

2) The next use case is using $form arrays for grouping purposes only. I'm not talking about fieldsets, but just about empty "parent" arrays with #tree == NULL. They might be useful to group several form elements together, or just to avoid possible array key conflicts when building $form in a modular way. I could have needed it for the rules UI - that's why I opened the original issue. As of now I've to use #tree == TRUE so the array level used for grouping purposes also appears in the form values and the submit handler has to work around it - yuck!

Form API currently has a bad distinction between the representation of the form and the data structure of the form values as both is built up with $form - but #pre_render/#theme could help us here. So I don't think using #tree == NULL groups for adding render elements like containers is something good to do nor do I think it's good to use it to inject array parents in hook_form_alter(), but still there are use-cases 1) and 2).

Next I don't see any use case for the current behaviour of #tree == FALSE - that just makes no sense to me.

fapi_tree_null.patch queued for re-testing.

This issue is quite old and inactive, but I still run into this problem every once in a while.

The existing proposed patch is nice. But possibly BC-breaking?
'#tree' => NULL already has a meaning and a behavior before this patch. Especially for code that uses isset() and thus cannot distinguish NULL from a missing value. So the patch would change expected behavior.

To recap, here is the code that determines the #parents of child elements.

        $element[$key]['#parents'] = $element[$key]['#tree'] && $element['#tree'] ? array_merge($element['#parents'], array($key)) : array($key);

This code can be found both in D7 form_builder(), and in D8 FormBuilder::doBuildForm().

Side note: Performance

One quick observation:
I suspect that this implementation using array_merge() is not the fastest option available.
Faster would likely be something like in the *-performance.patch attached.

Idea: $element['#reparent'] = [..]

This idea gives more control over the #parents element.
Patch is made with git format-patch to distinguish the different steps.

Since '#reparent' => [..] had no meaning before and was likely not used, this is less likely to cause BC issues.

Other idea: $element['#tree'] = 'skip'

See the patch.
Since '#tree' => 'skip' had no meaning before and was likely not used, this is less likely to cause BC issues.
However, it could be that some code relies on #tree being either TRUE or FALSE.

So maybe it is better to introduce a new key like '#reparent'.

The problem was that sometimes $element['#parents'] was not set.

In existing code we do not have any bulletproof safeguard for these situations.
Instead, we just assume that if $element[$key]['#tree'] && $element['#tree'], then $element['#parents'] is set. Otherwise, $element['#parents'] is allowed to be not set.

Oh wait, we also assume further above that if !isset($element['#id']), then $element['#parents'] is set and an array.

-----

Further research shows that $element['#parents'] can be removed by a #process callback.

In Drupal\form_test\Form\FormTestFormStateValuesCleanForm:

    $form['wine'] = [
      '#type' => 'value',
      '#value' => 3000,
      '#process' => [[$class, 'cleanValue']],
    ];
[..]
  public static function cleanValue(&$element, FormStateInterface $form_state, &$complete_form) {
    $form_state->addCleanValueKey('wine');
  }

This "forgets" to return $element;. Instead, $element is treated as by-reference.
This causes the fails in StateValuesCleanTest and ElementsVerticalTabsTest.

This points to a larger problem: Do we universally trust #process callbacks, or any callbacks? Or do we need to add checks for e.g. the case where a #process callback returns NULL, or an incomplete array?

See also #1499596-145: Introduce a basic entity form controller for call_user_func() vs call_user_func_array() to call #process callbacks.

EDIT: Forget it. Dead end.

See #2769549: Log misbehaving callbacks (e.g. #process)

New patches with the fix in FormTestFormStateValuesCleanForm::cleanValue().

I also added "-CLEAN.patch" versions which do not contain the not really related "performance" code change, and also not the fix I just mentioned.

I used git format-patch all the time so different changes can be distinguished.

One solution that already works for D7 (and D8 I suppose):

function element_pop_parent(array $element) {
  array_pop($element['#parents']);
  return $element;
}

$form['container'] = [
  '#type' => 'container',
  '#process' => ['element_pop_parent'],
  'value' => [
    '#type' => 'textfield',
    [..]
  ],
];

The advanced solution with dynamic reparenting, similar to the '#reparent' in the patch, can look like this:

// Call ReparentProcessor::__invoke()
$element['#process'][] = new ReparentProcessor([FALSE, 'replacement_key_0', 'replacement_key_1']);

This already works in D7.

For anyone interested, I added this stuff to themekit:
http://cgit.drupalcode.org/themekit/commit/?h=7.x-1.x&id=a7ac625f8c03042...
http://cgit.drupalcode.org/themekit/commit/?h=7.x-1.x&id=d293663b856772f...

Building on #18 you can even write everything with a local closure. This worked for me:

"#process"=>[function(array $element) {array_pop($element['#parents']);return $element;}]

A very convenient drop-in replacement as long as there is not #tree=>"skip"

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or 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.