Currently setting #tree == FALSE on a element puts it on the top level of the values, regardless of any parents with #tree == TRUE. This is not only what one would expect, it also makes it impossible to add any parent element in the form API tree that doesn't influence the values once you had a #tree == TRUE.

E.g.
$form['parent'] has '#tree' == TRUE
$form['parent']['wrapper'] has '#tree' == FALSE

Then the value of the form element
$form['parent']['wrapper']['value']
doesn't land in $form_state['values']['parent']['value'] but in $form_state['values']['value'].

Attached is patch that adds a test case for that and fixes it.

CommentFileSizeAuthor
#27 drupal-tree_default_true-759222-27.patch593 bytestunic
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#24 drupal8.form-tree-true.24.patch588 bytessun
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#3 drupal_tree_fix.patch7.38 KBfago
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_tree_fix_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
drupal_tree_fix.patch4.32 KBfago
FAILED: [[SimpleTest]]: [MySQL] 18,973 pass(es), 41 fail(s), and 1 exception(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

fago’s picture

Status: Active » Needs review
Issue tags: +D7 Form API challenge

Status: Needs review » Needs work

The last submitted patch, drupal_tree_fix.patch, failed testing.

fago’s picture

Status: Needs work » Needs review
Issue tags: +API change
FileSize
7.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal_tree_fix_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

Well, obviously modules might rely on the existing weird behaviour. Still one could get this easily be setting #parents manually. However I don't think it makes any sense to generate such parents by default.

Updated patch and fixed the image style form.

rfay’s picture

subscribing

effulgentsia’s picture

Title: Nested form elements fall out of the tree » nested form elements fall out of the tree

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).

Damien Tournoud’s picture

Issue tags: +DrupalWTF

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

Damien Tournoud’s picture

Title: nested form elements fall out of the tree » Nested form elements fall out of the tree
fago’s picture

Title: nested form elements fall out of the tree » Nested form elements fall out of the tree
Priority: Normal » Critical
Issue tags: +security

The problem when this values fall out of the tree is also that they won't play nice with #limited_validation_errors - see #763376: Not validated form values appear in $form_state. We need this fix, so this values are incorporated properly. -> Bumping to critical.

Apart from that I guess validation for our fallen out values wouldn't be properly limited as of now. Thus when the tree is supposed to be validated I think our values won't -> security.

effulgentsia’s picture

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.

effulgentsia’s picture

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.

fago’s picture

Indeed, the way the defaults are applied can also be cumbersome. Having NULL for "skip this, but keep the previous default" would be useful.

@form-alter: I think your usage is valid, however when altering forms one has to always care to not break existing stuff - thus as of now you have to care and set the children #tree values to TRUE - weird yes. However for altering the appearance, I do think using #pre_render is cleaner and thus the way to go. Also that way you don't change the structure of $form for other form alter implementations, which was sometimes an issue with fieldgroup. Thus I think a fieldgroup module in D7 should ideally use #pre_render as it's about changing the appearance only.

Still - you can use "empty arrays" just for grouping purposes in $form and set #tree to FALSE. For those cases having a #tree NULL option would be great too. So it would be a nice-to-have, but it's a must that the elements will never fall out of the tree.

fago’s picture

Issue tags: -security

@9 & security:
Indeed the issue with limit_validation_errors is a confusing consequence, but strictly seen it's perhaps no security issue. The value falls out the tree, thus it also falls out the validated values. But with the weird behaviour in place, that's valid (but probably not expected).

sun’s picture

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.

chx’s picture

Version: 7.x-dev » 8.x-dev
Priority: Critical » Normal
fago’s picture

Version: 8.x-dev » 7.x-dev

ad #13:
Yep having #tree defaulting to TRUE make sense, but that's D8 material - yes.

@patch:
The issue is just about fixing elements falling out of tree, so let's stay with discussing that. Several people have noted that's a major WTF, so why not fix it now instead of having it another release cycle? And can we please provide reasons for status changes? Thanks.

@priority:
As noted in #12 it probably doesn't qualify as security problem, so I'm fine with normal.

sun’s picture

@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.

sun’s picture

Version: 7.x-dev » 8.x-dev
effulgentsia’s picture

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.

fago’s picture

>@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.

I'd say it's unexpected behaviour developers stumbled about since Drupal 5 ;) But as you think.

effulgentsia’s picture

Issue tags: -D7 Form API challenge

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.

fago’s picture

Issue tags: -DrupalWTF, -API change

So let's fix it for D8?

#3: drupal_tree_fix.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +DrupalWTF, +API change

The last submitted patch, drupal_tree_fix.patch, failed testing.

sun’s picture

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.

sun’s picture

Title: Nested form elements fall out of the tree » #tree does not default to TRUE, fails to meet developer expectations
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +DX (Developer Experience)
FileSize
588 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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.

Status: Needs review » Needs work

The last submitted patch, 24: drupal8.form-tree-true.24.patch, failed testing.

joachim’s picture

tunic’s picture

Status: Needs work » Needs review
FileSize
593 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

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

Status: Needs review » Needs work

The last submitted patch, 27: drupal-tree_default_true-759222-27.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.