#tree does not default to TRUE, fails to meet developer expectations

subscribing

Thanks fago! I completely agree with this, so +1 from me. It's a WTF that's annoyed me too. The whole point of #tree, as I see it, is so that you can implement a hook_form_alter() that inserts a wrapper element for rendering needs (or #process insertion needs) without that element affecting $form_state['values'] (since the module implementing the alter hook doesn't want to break the existing submit handlers of the original form), but with HEAD's implementation, that doesn't work, since as the OD explains, inserting the 'wrapper' element with #tree => FALSE DOES change $form_state['values'], so instead, when you want to insert a wrapper element, your module needs to manually adjust #parents.

Unfortunately, the fix breaks BC for modules that adapted to the unintuitive behavior of the last several Drupal versions and HEAD, though as fago points out, modules can resume prior behavior by setting #parents => array($key). Would be great to see feedback here from chx (who may have a perspective on why HEAD is the way it is) and quicksketch or drewish (since image.module is the only core module affected by the change).

+1 for me too. This behavior always annoyed me.

I really like this issue, and am fine with it being "critical" on DX grounds.

I don't understand the security argument though. #limit_validation_errors is strictly about #parents/#name. All it does is within form_set_error(), suppresses an error if #name puts it outside one of the "sections" defined in #limit_validation_errors. It has nothing to do with #array_parents. The premise is that the submit handler knows what part of $form_state['values'] it needs to work with. If someone comes along and sets #tree => FALSE in a way that the element falls out of a section defined in #limit_validation_errors, then yes, errors on that element will be suppressed. But that element's input will also fall out of the expected part of $form_state['values'] and not be used by the submit handler. So, to me, this comes back to the annoying DX of not being able to add wrapper elements with #tree=>FALSE without also breaking submit handlers, but I'm not seeing where security is compromised. Leaving the tag though, for others to weigh in on.

Some more thoughts on this:

$form['a']['b']['c'] = array('#type' => 'textfield');
$form['a']['#tree'] = TRUE;

results in the textfield input going into $form_state['values']['a']['b']['c'].

Now, suppose a hook_form_alter() does:

// Move $form['a']['b'] into a wrapper.
$form['a']['wrapper']['b'] = $form['a']['b'];
unset($form['a']['b']);
// Make the wrapper transparent to input processing.
$form['a']['wrapper']['#tree'] = FALSE;

With HEAD, the textfield input goes into $form_state['values']['c'];
With #3, the textfield input goes into $form_state['values']['a']['c'];

Neither of which matches the original, so in both cases, the hook_form_alter() breaks the original form's submit handler.

Do we need to add a new option (#tree => NULL), which would be similar to FALSE, but without propogating down?

Or is this example a misuse of hook_form_alter()? Since we have #after_build and #pre_render, should it be considered bad to mess with form structure within hook_form_alter(), as it is likely to break input processing?

rfay and I are preparing a DrupalConSF session on FAPI, and this example came up as something I'd like to discuss, so looking for opinions on it.

By the way, this use-case is not contrived: it's pretty much exactly what CCK's fieldgroup.module does, using hook_form_alter(), and the only reason it works is because the fieldgroup element is at the top-level of the form: if it were put into a sub-element whose #tree was TRUE, it would be broken.

I also need to think about the meaning of #tree => FALSE twice each time I see or use it. However, I'm not sure whether there's room for this change in D7.

Just recently, I rather tinkered about whether it wouldn't be a better idea to just change the default of #tree. Most often, everyone assumes #tree => TRUE when constructing or altering forms. The fact that it's FALSE by default is what primarily makes you go into snafu.

@fago: What chx tried to say is: What you describe with "elements fall out of the tree when using #tree => FALSE" is the expected and intended behavior of #tree => FALSE since Drupal 5.

If there are form constructors that are missing #tree definitions or using them wrongly, then we of course need to fix them. But regarding any API change for #tree, I tend to agree with chx that it's too late.

While I can't currently think of a use-case where one would want an element with #tree=>FALSE inside an element with #tree=>TRUE to behave as it does in HEAD (and has since D5), if chx and sun believe this to be an out of scope BC break for D7, then I'm willing to accept that position.

Therefore: #784874: Add ability to insert FAPI wrapper element without modifying #parents of children.

Removing the D7 Form API challenge tag, since the version got bumped to D8. If someone bumps the version back to D7, please feel free to re-add the tag.

This topic just came up again in #1309394-9: Process #autocomplete_path for all form elements; remove custom/duplicated code from theme_textfield().

Specifically for the use-case of #process functions that extend a parent element with a new child. The new child should normally behave like #tree => TRUE. But if you apply that to the parent, then it can have unintended consequences on other possibly existing child elements.

And once again, this topic came up in an IRC discussion yesterday.

Pretty much every developer expects #tree to default to TRUE instead of FALSE.

Now, given that all entity forms in D8 are operating with top-level elements, or are forcing #tree to be TRUE either way already, the consequences of changing the default value would pretty much be limited to (1) settings forms and (2) completely custom forms.

So let me simply try something very naive, just to see how much will break.

Marked #1130946: elements with #tree false that have a parent with #tree TRUE fall to the base of the form values as a duplicate -- there is also a patch there!

Let's try sun test again (rerolled patch).

I tested this on 9.4.x using the example in the issue summary as a guide. In the submit function the value of wrapper was indeed at the correct level, in the parent.

Therefore, closing as outdated. If you are experiencing this problem on a supported version of Drupal reopen the issue, by setting the status to 'Active', and provide complete steps to reproduce the issue (starting from "Install Drupal core").

Thanks