Not validated form values appear in $form_state

#limit_validation_errors was a stopgap measure to allow things like "add more" buttons where more relaxed security is less of a problem, not to give people a false sense of security that the property is safe to use for submit handlers that really need validated values. The problem is that there's not necessarily a perfect correspondence between element #parents properties and validated $form_state['values']. There is when elements are well behaved, but our validation pipeline doesn't enforce good behavior. I'm concerned if people start using #limit_validation_errors for buttons that trigger #submit handlers that do potentially risky things. I'm hoping that #735800: node form triggers form level submit functions on button level submits, without validation fixes the problems of core doing this. Given that we can't control contrib, stripping $form_state['values'] to only the "sections" in #limit_validation_errors before calling the #submit handlers seems like an idea worth considering, but given the issues we're facing with entity editing forms, I wonder what unwanted side-effects that will create: especially in multi-step forms where people are persisting prior steps' validated values in $form_state['values'].

[EDIT: 'values' is within form_state_keys_no_cache(), so my concern about multistep forms might not be a problem]

I agree putting non-validated values in $form_state['values'] can end up being quite confusing.

Great. Let's see what fails.

subscribing

+++ includes/form.inc	13 Apr 2010 16:06:20 -0000
@@ -632,6 +632,32 @@ function drupal_process_form($form_id, &
+      if (isset($form_state['triggering_element']['#limit_validation_errors']) && ($form_state['triggering_element']['#limit_validation_errors'] !== FALSE)) {

Would it be possible to backup this value earlier in drupal_process_form(), so as to prevent validation handlers to break into a unbreakable system?

The rest could use a comment or two, but it's probably also good enough if it's just us who understand it. :P

Powered by Dreditor.

wow. You managed to trigger a fatal error, but only in tests that seem to be related to #limit_validation_errors. Nice work :)

#6: form-remove_unvalidated_values-763376-6.patch queued for re-testing.

This issue is about unvalidated data being in $form_state['values'], which is confusing.

This only applies to fields with #limit_validation_errors. Why can't this be solved with documentation on #limit_validation_errors?

Not sure why it was marked "needs review" in #9, when tests were failing in #6. Marking "needs work"...

In #11 Fago implies that the patch makes the API *more* confusing by having the unvalidated data in $form_state['values'] during the validation step, but not during the submit step.

This seems either not a critical bug, or a documentation fix to me...

Anyone available to push this along? Is it really critical?

I think this more or less depends on #735800: node form triggers form level submit functions on button level submits, without validation.

Correct. It will probably be relatively easy once the other issue lands, and that one is very close.

#735800: node form triggers form level submit functions on button level submits, without validation landed, so this issue can proceed again: yay!

BTW: From #14:

Imho the special casing of #type == 'form' in form_builder() should be moved to a separate after build callback functions for the #type == form element. That would clean up form_builder(), but that's out of scope.

I agree that this is a desirable thing to do in a separate issue. See http://randyfay.com/node/66#comment-102.

I haven't reviewed the latest patch yet, but:

From #22:

But furthermore cleaning up the values after validation doesn't work either, as still modules rely on button values ('$form_state['values']['op']) or similar to exist, thus this would break also existing code.

I disagree with this. Submit handlers don't have to use $form_state['values']['op']. They can use $form_state['triggering_element']. And in many cases, they won't even need that, since #limit_validation_errors is only in-effect if the button also has a button-level #submit, though you do have some cases like file.module, where the "upload" and "remove" buttons use the same #submit, and therefore, that handler does refer to $form_state['triggering_element'] to know whether to process the "upload" or "remove" action. If the submit handler wants to switch on $form_state['values']['op'], then I think it makes sense to require that the #limit_validation_errors of that button include array('op') as one of its items.

From #11:

hm, doing it at this step would mean the values are there during validation, but not during submit. That's imho confusing and bad DX.

I don't think it's confusing, because that's how FAPI works. During normal processing when there is no #limit_validation_errors, during validation, $form_state['values'] may contain invalid data. It is only known to be valid during the submit handlers. So, #6 doesn't introduce anything new in that regard. But you may be right about bad DX: do you have a specific example that illustrates what makes it bad DX?

#25 seems way more complicated than #6. But if #6 is truly bad DX, then ok, let's refine #25.

Hm. I think we need some more FAPI maintainer feedback here. I'd really like to know what sun and chx think.

I think we have two approaches we can take:

1) fago's recommendation (#25), which has, IMO, these two potential down-sides:

a) It requires the drupal_set_processed_form() function in order to have a statically cached $form_state available within form_set_error(). I actually like the idea of a statically cached $form_state available to form_set_error(), because it's annoying that that's our one place where we don't have $form_state available, and I tried to add it in the original #limit_validation_errors issue (#370537-162: Allow suppress of form errors (hack to make "more" buttons work properly)), but chx removed that in the following comment and it stayed removed in the patch that landed.

b) It only pulls things out of $form_state['values'] that fail validation. So, if a developer does the thing we're specifically trying to protect them from, which is having their submit handler try to access $form_state['values'][SOMETHING_OUTSIDE_A_LIMIT_VALIDATION_ERROR_SECTION], it will work as long as they submit valid input, and will only stop working when they submit invalid input. Seems like this isn't ideal DX, because it delays when the developer becomes aware of the bug in their code (they only become aware of it when trying to submit the form with invalid input).

2) My earlier recommendation (#6), though after reading the last paragraph of #27, I'm not sure if this is still my recommendation, which avoids the two problems of #25, but has the two down-sides mentioned in the last paragraph of #27:

a) The clicked button's name/value (e.g., 'op') gets removed from $form_state['values'] unless it's whitelisted in #limit_validation_errors. This can be easily fixed by adding a little more code at the end of #6:

if (isset($form_state['triggering_element']['#button_type'])) {
  $form_state['values'][$form_state['triggering_element']['#name']] = $form_state['triggering_element']['#value'];
}

b) Other form elements that specify a #value (e.g., 'form_id') in the build phase, and that therefore, are always valid, get removed from $form_state['values'] unless whitelisted in #limit_validation_errors. As fago points out, it may be undesirable at this stage in D7 to require these kinds of tokens to be whitelisted everywhere #limit_validation_errors is used. If we wanted to auto-whitelist all elements that have a builder-specified #value, we could add code in _form_builder_handle_input_element() to track that in some new $form_state variable (e.g., $form_state['forced_values']), but that's starting to feel icky.

So, I'm torn. I see the appeal and annoyance with each approach, and haven't thought of anything better yet.

Actually, I think this is my recommendation at the moment:

#6 + the fix in #28.2a. And require use-cases that involve a #limit_validation_errors submit handler accessing $form_state['values']['form_id'] and similar tokens to add 'form_id' and the other ones that are needed by the submit handler to the #limit_validation_errors array. This requires the smallest code change to HEAD and I think only adds minimal annoyance to developers who use #limit_validation_errors. Note that none of our 3 submit handlers in core that use #limit_validation_errors require access to a form token value:
field_add_more_submit()
file_managed_file_submit()
poll_more_choices_submit()

The code looks great to me. sun should review this anyway, so I'll let him make documentation suggestions. Also, I think a test should be added to the patch. Not sure if this issue is a beta blocker or not, but if it is, I believe it can be committed as-is.

I asked chx to review and comment. @fago: while he's doing that, in case you want to do an interim improvement:

1. In IRC, chx requested that drupal_(get|set)_array_deep() not be recursive, since PHP recursion sucks performance and memory. They can be done with a simple foreach loop (in the case of set, a reference that moves during the foreach).

2.

+++ includes/form.inc
@@ -917,6 +917,25 @@ function drupal_validate_form($form_id, &$form, &$form_state) {
+      $form_state['triggering_element']['#limit_validation_errors'][] = $form_state['triggering_element']['#parents'];

This doesn't feel quite right. Since $form_state is by reference, changing #limit_validation_errors here seems unclean. Also, what if a textfield and a button have the same #parents (bad module developer, bad!)? Is there another way to achieve the same goal? Perhaps simply drupal_set_array_deep($values, $form_state['triggering_element']['#parents'], $form_state['triggering_element']['#value'])?

3.

+++ modules/simpletest/tests/form.test
@@ -324,6 +324,10 @@ class FormValidationTestCase extends DrupalWebTestCase {
+    // Ensure not validated values are not available to submit handlers.
+    $this->drupalPost($path, array('test' => 'valid'), t('Partial validate'));
+    $this->assertText('Only validated values appear in the form values.');
...
+function form_test_limit_validation_errors_form_partial_submit($form, $form_state) {
+  if (!isset($form_state['values']['title']) && isset($form_state['values']['test'])) {
+    drupal_set_message('Only validated values appear in the form values.');
+  }
+}

Can we clarify this test by submitting an explicit 'title' => ''? And a comment that the !isset() doesn't suffer the risk of a false positive, even when empty string is submitted, since empty string is different from NULL. This stumped me until I understood that subtlety.

Powered by Dreditor.

2 new very helpful helper functions, 1 fine and fancy form API change, 0 remaining review issues.

Looks very good. I am a little unsure of the names of the new functions but they will do.

Sorry crossposted.

chx asked me in IRC to post my opinion on the get/set functions.

I think the names are OK and the code is clear. Though maybe set/get_array_element_deep would be a better name?

Also, suggested minor updates to the doc headers (sorry, in a rush, don't have time to patch and probably the wrapping is incorrect):

/**
 * Sets a value in a nested array.
 *
 * Example: if $array is array('a' => 1, 'b' => array('c' => 3, 'd' => 4)), and $parents is array('b', 'c'), then
 * the 3 will be replaced by $value.
 *
 * @param $array
 *   A reference to the array to modify.
 * @param $parents
 *   An array of parent keys, starting with the outermost key.
 * @param $value
 *   The value to set.
 *
 * @see drupal_get_array_deep
*/

/**
 * Retrieves a value from a nested array.
 *
 * Example: if $array is array('a' => 1, 'b' => array('c' => 3, 'd' => 4)), and $parents is array('b', 'c'), then
 * the return value will be array(3, TRUE).

 * @param $array
 *   The array from which to get the value.
 * @param $parents
 *   An array of parent keys of the value, starting with the outermost key.
 *
 * @return
 *   An array whose first entry is the array value, and whose second entry is TRUE if all the parent keys
 *   existed, and FALSE if not (in which case the value element will be NULL).
 */

Reroll with jhodgdon's doc changes (plus an extra @see) and drupal_get/set_array_element_deep function names.

Cross-post. Guess we can run it by the bot again though.

-1 on the function name change -- "element" adds no valuable meaning to the function names. And, there is no risk of function namespace pollution in this case. Even the "deep" is senseless in reality already. Arrays are deep, by nature. If ya really wanna change tha name, considering a proper [subject]_[verb] namespacing would've more value.

To me element doesn't imply a form - it just means an array element. I think it's clearer to specify "element" in the name - otherwise it sounds like you're setting the whole array. But if element sounds too FAPI-ish maybe we could do drupal_get/set_array_value_deep.

Okay, what about drupal_get/set_deep_array_value? (I promise I will not bikeshed this too much. It's just a difficult function to understand in the first place, and it could use a good name.)

These function names really don't need to be overlengthy. Keywords "array" and "set/get" are sufficient to let each and everyone understand that they are about handling array values. What else can you, actually, remotely, expect? Is there anything else, possibly?

  $array['key'] = $value;

  $value = $array['key'];

Why on earth would you want to call a function for that?

Righto, because übercoredevs simplified the only possible array complexity for you. :)

I disagree that these names are clear enough to let everyone understand. I certainly didn't at first glance. And I am thinking of the poor person stepping through FAPI for the first time trying to understand wth is going on in there. It seems like a long function name would help in that case. These functions certainly won't be used often enough to need short names for convenience. But like I said, I won't bikeshed this.

Not limited to Form API, thus contained in common.inc.

drupal_array_set_value($page, array('page', 'content', 'system_main', '#attributes', 'class'), array('override-class'));

We've worked with form_set_value() for years. Forms are straight arrays, and this patch replaces that entire function. Thus, array_set_value() would be the direct replacement, but since Array is not a Drupal sub-system or module, and we don't want to risk function name clashes with PHP, we can simply do what we always do, prepend drupal_ and be done with it.

--

However, for both function PHPDocs, we absolutely need more elaborate descriptions, so as to clarify, why someone would want to use them. The examples that have been added do not explain this very first WTF moment. People will ask, why not simply:

$page['page']['content']['system_main']['#attributes']['class'] = array('override-class');

"How many needless functions can this bloated Drupal provide and even load on all requests?" :P

Let's try to prevent this reaction.

I'm fine with the names of drupal_(get|set)_array_deep(), but throwing some other options into the mix:

• drupal_tree_(get|set)()
• drupal_tree_(get|set)_value()
• drupal_array_(get|set)_nested_value()

Of these 3, I prefer the last, because even though "tree" is the technical term for this, it's not in widespread usage within Drupal, and people are already confused enough with #tree.

#52.2 is better than #52.1 and should be used wherever you can target each sub-index literally. The function is for when $parents is a variable, because for these cases, #52.2 is not an option. (hoping someone else can turn that into a good PHPDoc)

If we really want a short name, I experimented with a drupal_subtree() function in #370537-92: Allow suppress of form errors (hack to make "more" buttons work properly), but it got dropped in subsequent patches: I'm not suggesting we want that function, but it does provide a short name.

Hmmm.

If the parents are variables they can still be put into a direct call like $array[$var1][$var2]['etc']

The only case where you couldn't do that is if you don't even know the number/depth of the parents array.

Maybe:
"In case the array parents are known beforehand the value should be directly set/retrieved from the array. This helper function should be used only when the depth of the value may vary, i.e., the number of parents is determined by a variable."

?

I just re-read that and it's not very good IMO, as the $variable doesn't actually contain the NUMBER of parents, but the parents themselves, which in turn may vary in number. So the text is a bit misleading.

Maybe the following (just cut out 3 words):
"In case the array parents are known beforehand the value should be directly set/retrieved from the array. This helper function should be used only when the depth of the value may vary, i.e., the number of parents is variable."

Looking good.

Me merely renamed two strings and copy'n'pasted #58 into PHPDoc. Back to RTBC.

The current version is not punctuated correctly... you need a ; before an i.e.

How about

"If the array parent keys are known, the value should be directly set/retrieved from the array. This helper function should be used only when the depth of the value may vary (that is, the number of parents is variable)."

Also, "set/retrieved" should not be in there literally. It should be set in the set function, and retrieved in the get function.

Re-rolled with the suggested text from #62.

+++ includes/common.inc	9 Jul 2010 21:58:52 -0000
@@ -5532,6 +5532,71 @@ function element_get_visible_children(ar
+ * If the array parent keys are known, the value should be directly set from
+ * the array. This helper function should be used only when the depth of the

"set from the array" => "set in the array".

Powered by Dreditor.

And the change suggested by sun in 64...

Thanks! It's time to close down this critical. Further adjustments, if required, can happen in a non-critical follow-up patch.

Just a reroll to head

#68: non_validated.patch queued for re-testing.

Confirmed that #66 and #68 are identical.

+++ includes/common.inc	17 Jul 2010 02:08:02 -0000
@@ -5535,6 +5535,69 @@
+function drupal_array_set_value(&$array, $parents, $value) {

Personally not a big fan of these new helper functions. They are anything but easy and not particularly elegant from an API/DX point of view -- it is a bit of a WTF to be honest. Still need to think more about how to make it easier.

At this point in time, I think we should go with the current solution. If it's still a big WTF, then it's only the docs that need to be updated. Otherwise, those new helper functions merely resemble what existing code in HEAD (even D6) already does, just that existing code is more complicated.

Field / Field UI has to do that same sort of array navigating in a couple places. I'd love to use those helper functions instead - precisely because the code to do that is tedious to write and to read :-).

Totally agreed with Dries in #71. The examples in the docblock might make sense to the people involved in this issue, but they make absolutely no sense to a passerby. If we're hoping for these to be general utility functions for contrib authors (sounds like folks are saying that would be useful), we should spend some time on the DX here.

Probably at the very least need a fleshed-out "real world" example (and in actual @code tags) of how to use both of these. I tried looking for a good example in the patch itself and came up short. :\ Documenting this will probably help with figuring out how to name the functions/arguments a bit more sensibly.

I've heavily expanded the PHPDoc, included example code, and even included example snippets that explain that the only other way would be using evil eval(), which is why these helper functions exist.

Please don't RTBC your own very changed patches! This one needs some work...\

a) Shouldn't these be literal 'b' and 'c' in the first example in this hunk? Either that or set $b and $c so we know what you are talking about.

+ * @code
+ * // When parent keys and depth is known:
+ * $array[$b][$c] = 'replacement-value-for-c';
+ *
+ * // This function could only be resembled using evil eval():
+ * eval('$array[\'' . implode("']['", $parents) . "'] = $value;");
+ * @endcode

b) And in the same chunk, it took me a while ot figure out the second comment about "resembled using evil eval()". First of all, probably you mean "reassembled", and second, I think it was confusing to say "this function" here, when the previous line is talking about something else... probably you should put your explanation outside the @code block and make it clearer.

c) Comment (a) also applies to the get function.

Anything else?

Sorry... By 'a fleshed-out "real world" example' I meant that instead of this 'a' 'b' 'c' stuff, let's show them an actual form with a use case and why one would want to use this function. The use case must exist, because yched mentions these helper functions are valuable, but I can't understand from the current docs why these functions exist nor why I might want to use them.

Those real world use-cases exist since Form API was introduced, in various contributed modules. The code of these two helper functions is duplicated in various modules, or even worse, I've actually copied that evil eval() example from a contributed module.

I think that the example values are simple and sufficient enough and at the same time not too complex to explain the functionality. To see other examples, users can click through the list of functions that call these functions. Among them will be that function in File module, which is a nice showcase already. We'll potentially see more.

I'll make some better examples

How about this?

Thanks.

Reviewing this from the Core Developer Summit in Copenhagen.

+++ includes/common.inc
@@ -5561,6 +5561,127 @@ function element_get_visible_children(array $elements) {
+ * Sets a value in a nested array.

Could we change this to say something along the lines of:

"Sets a value in a nested array with variable depth."

The function name and the documentation at the top of the phpDoc make it sound as this is the preferred way to set a value in an array. It should only be recommended when using an array with variable depth. Would be great if that could be represented in the documentation and/or function name.

+++ includes/common.inc
@@ -5561,6 +5561,127 @@ function element_get_visible_children(array $elements) {
+ * @return
+ *   An array whose first entry is the array value, and whose second entry is
+ *   TRUE if all the parent keys existed, and FALSE if not (in which case the
+ *   value element will be NULL).

This is very ugly, and won't work for me. It is much cleaner to introduce a third function called drupal_array_exist().

If we make those changes, this is RTBC for me and webchick.

webchick and myself discussed this, and we definitely want to add drupal_array_exist() so the calls to list() can be avoided. APIs need to be clean.

Ok, Dries, Fago and I spent like 30 minutes going through this patch. At least 20 of that was me still being extremely confused. :P A big part of that was:

a) The API doc code example is not good for explaining why this function is needed. What helped me was when fago explained that the real issue is the following:

Your code wants to find the 'signature' field. The problem is, that signature field might be in an array like this:

$form['signature_settings']['signature'] = array(
  '#type' => 'text_format',
  '#title' => t('Signature'),
);

or it might be in an array like this:

$form['signature_settings']['user']['signature'] = array(
  '#type' => 'text_format',
  '#title' => t('Signature'),
);

So to deal with this situation, your code needs to do some codey thing to figure out the path to the field ($form['signature_settings']['user']['signature'] or $form['signature_settings']['signature'] or...). You end up with a dynamic $parents array.

Without this pair of functions, the only way to get at it is this line of code:

$element = eval('$form[' . implode($parents, ']['));

...which is digusting and kills kittens.

The second part of that (eval() being the only other options) is spelled out pretty well, but the initial example totally isn't. Which is why we need to add that other bit about "Don't actually use this function at all, just reference the array values directly."

So let's fix the example code to explain the actual problem, and in the part that says "don't use eval(), let's put the commented out eval line just above the actual line you would call with the new helper function. Capice?

2. Speaking of killing kittens, this makes Dries and webchick cry:

+ * @return

+ *   An array whose first entry is the array value, and whose second entry is

+ *   TRUE if all the parent keys existed, and FALSE if not (in which case the

+ *   value element will be NULL).

Dear sweet lord mercy, no. From a DX POV this is awful, and a function should simply return success or failure, and not conflate it with values.

Fago explained that this is required, because we can't do a simple "return FALSE/NULL" because then there would be no difference between trying to reference a value that didn't exist (the 'banana' test) and a value that was legitimately set to FALSE/NULL.

So what Dries and I decided is we want a third function, an "array key exists" style function, to handle the TRUE/FALSE part, and the values separately. Fago has concerns about performance here (since we have to do that loop twice), but from a DX POV it is *much* easier to follow code like this:

if (drupal_array_value_exists('banana')) {
  $values = drupal_array_get_value('banana');
}

...than code like this:

list($value, $status) = drupal_array_get_value('banana');
if ($status) {
  
}

3. Finally, the name of this function. drupal_array_get_value() and drupal_array_set_value() sound suspiciously like I should use. I should never use this function. :P This function is for people like yched or fago or people developing low-level form elements (by and large). It also doesn't remotely describe what this function is actually doing, which was at least 10 minutes of the confusion.

So. We did a mini bikeshed session and decided on the following:

drupal_array_nested_value_exists($element, $parents)
drupal_array_get_nested_value($element, $parents)
drupal_array_set_nested_value($element, $parents, $value)

...aaaaaaaand done. Fago's working on a re-roll.

TBH, and sorry for being direct, but I believe the review issues + rejections given are totally over-complicating this patch.

1) Just because there is a function somewhere, anywhere in Drupal doesn't mean I'll use it. Why should I? Do you do that? I think + hope you are not.

2) We happily lived with form_set_value() in the past. In Drupal 7, we transposed the concept of structured $form arrays to the entire page. Therefore, it's just logical and sufficient to replace that with a more generic drupal_array_set_value(). It seems like we didn't face reality yet: Our renderable arrays are not regular arrays. Thus, this is merely about s/form/drupal_array/, nothing else.

3) The additional drupal_array_get_value() is the counter-part to drupal_array_set_value(). And it's only introduced now, because it was totally missing in the past - I could have used it, but had to re-implement it all over again.

4) The new drupal_array_key_exists() or whatever it is named now is plain performance overhead. It makes zero sense to recurse more than once into an array. Recursing deeply into an array is slow. drupal_array_get_value() is not the only function that returns an array. Are all of those functions considered bad? No, they are not, if there's a legit reason for doing so. That's the case here: We have to return an array to allow calling code to differ between NULL and NULL. Again, recursing more than once is 100% needless overhead for every use-case of this function. To use drupal_array_get_value(), you have to invoke drupal_array_key_exists() -- it simply makes no sense to split it.

To conclude yet again:

- Longer function names do not buy us anything. If a developer happens to use these functions where they shouldn't be used, no amount of docs and no amount of additional words in the function names will prevent that.

- drupal_array_key_exists() is plain performance overhead.

- The phpDoc of #78 and #84 was more than sufficient for the purposes of this function already. People who don't understand that, also don't understand form structures and renderable arrays, so the phpDoc of these functions is the entirely wrong place to document how Drupal's arrays work and need to be treated at run-time.

I don't disagree about the performance overhead. But I'm concerned with people being able to understand what's happening in the code.

This:

+++ includes/form.inc
@@ -1790,11 +1806,8 @@ function _form_builder_handle_input_element($form_id, &$element, &$form_state) {
-  $values = $form_state['values'];
-  foreach ($element['#parents'] as $key) {
-    $values = (isset($values[$key]) ? $values[$key] : NULL);
-  }
-  if (!isset($values)) {

...is very clear and makes complete sense.

This...

+++ includes/form.inc
@@ -1790,11 +1806,8 @@ function _form_builder_handle_input_element($form_id, &$element, &$form_state) {
+  list($values, $value_exists) = drupal_array_get_value($form_state['values'], $element['#parents']);
+  if (!$value_exists) {

...makes absolutely no sense, and I must look up the API documentation of drupal_array_get_value() to understand what's going on. A "mixed" return value that includes both "value you want" and "status" is totally inconsistent from any other function in Drupal, apart from the ungodly awful update_sql() which we've thankfully ripped out from D7. Not eager to add more functions like this, ever, unless the performance shock is shown to be so unbelievably dastardly that there's no other option.

And sorry, I was in a hurry before so wasn't quite articulate enough to summarize the 30 minute discussion. The longer function name doesn't just buy us "scary, stay away", but it also buys us a name that actually describes what the function does. drupal_array_get_value() doesn't, remotely. I would expect that it's a wrapper function around some PHP function "array_get_value()" since that's normally why we use the drupal_ prefix. I showed it to a couple other random Drupalistas at the sprint and neither one of them could parse what the function was supposed to do, but having the keyword "nested" helped tremendously.

And the PHPDoc is totally not sufficient because the code examples totally do not actually describe the actual problem. It shows a normal run-of-the-mill form definition, on which you never would have to use this function because you know the structure in advance and can just reference it with $form['foo']['bar']['baz']. The PHPDoc goes on to qualify this itself further down which is a wonderful indication that it's confusing/bad PHPDoc and should be improved.

So, basically, I stand by these requests. Dries stands by these requests. If we can get them banged out we can close another critical tomorrow.

Powered by Dreditor.

Sorry, but

And the PHPDoc is totally not sufficient because the code examples totally do not actually describe the actual problem. It shows a normal run-of-the-mill form definition, on which you never would have to use this function because you know the structure in advance and can just reference it with $form['foo']['bar']['baz']. The PHPDoc goes on to qualify this itself further down which is a wonderful indication that it's confusing/bad PHPDoc and should be improved.

solely means that we're still afraid that people could use these functions, just because they exist. That can only happen if people don't know PHP, and we totally cannot teach or explain PHP in our phpDoc. Otherwise, why would you remotely ever consider to use a function to get or set an array value? Please explain that logic, because I absolutely don't get it.

This...

  list($value, $value_exists) = drupal_array_get_value($form_state['values'], $element['#parents']);

...makes absolutely no sense, and I must look up the API documentation

um? Can there actually be more precise variable names than $value_exists and $value? I don't get this either. That's most probably the most intuitive way to name the return values of the function.

Also, having you look up API documentation is much more performant than adding this run-time overhead to each and every Drupal site in the wild. But that said, people in the need for this functionality (no one else would remotely consider that such helper functions may exist) will perfectly understand the API documentation added earlier, and will also perfectly understand how to work with those return values.

I stand by #92, we are totally over-complicating things here. phpDoc, if something is still unclear, can be improved in a follow-up issue/patch. A separate drupal_array_key_exists() can happily discussed in a separate, non-critical issue, but it's useless and a plain performance overhead here.

I have to agree to sun - I find list($values, $value_exists) = drupal_array_get_value($form_state['values'], $element['#parents']); very concise, clear and readable. No need IMO for a second function that also adds a performance overhead. Traversing arrays in PHP is not cheap.

Everybody who is in need to use this function (which is not as rare as one might think, and will get less rare in D7) will have to read the phpdoc, which includes an example on how to use the array that's returned. And it's not the only place in drupal we use an anonymous array and list() to have functions return more than one thing quickly and easily, e.g. http://api.drupal.org/api/function/entity_extract_ids/7.

Changing the names to inlcude _nested_, that's a good change. In all this discussion, don't forget that previously, we abused a private function with a totally misleading name and no helpful phpdoc at all, so having public functions in the proper namespace is a huge huge plus in any way.

What about this possible alternative pattern:

$ok = drupal_array_get_value($array, $parents, $value) with $value passed by reference? i.e. have the function return true/false for success/failure, and have the value be set in a variable that the calling function provides? Would that make it clearer?

Each approach has pros and cons. We thought and talked about them a lot, and at the end of the day, I'm in the camp of using #91.

drupal_array_nested_value_exists($element, $parents)
drupal_array_get_nested_value($element, $parents)
drupal_array_set_nested_value($element, $parents, $value)

In fact, I think #91 is RTBC.

drupal_array_nested_value_exists() / drupal_array_get_nested_value() : isn't that what exceptions are for ? bubble up abnormal / failure conditions without clobbering the natural format for return values ?

#91 is not quite RTBC:

a)

+ * So to deal with the situation, the code needs to figure out the way to
+ * the element given an array of parents being
+ * @code array('signature_settings', 'signature') @endcode in the first case or
+ * @code array('signature_settings', 'user', 'signature') @endcode in the second
+ * case.

I think this paragraph is very awkward. How about:
To deal with this situation, the code needs to figure out the route to the element, given an array of parents that is either ...

b)

+ * Without this helper functions the only way to set the signature element in
+ * one line would be using eval(), which is best to avoid:

Either needs to be "without this helper function" or "without these helper functions".

c)

+ * one line would be using eval(), which is best to avoid:
+ *
+ * @code
+ * // Do not do this! Avoid eval().
+ * eval('$form[\'' . implode("']['", $parents) . '\'] = $element;');
+ * @endcode

Remove the blank line before @code, since this belongs with the text in one logical paragraph.

d)

+ * Before calling this function be sure that the value actually exists, if
+ * unsure use drupal_array_nested_value_exists() to check it.

This is a comma splice (it is illegal to connect two complete sentences by a comma in English). Use a semicolon ; or make it into two sentences.

e)

+ * @return
+ *   TRUE if all the parent keys exist, else FALSE.

This is good PHP syntax sort of, but not so good in English. How about else -> otherwise?

Sorry, but this does not work for me. Can we please be a bit more explicit than "we discussed this today and let's do X"? No amount of discussion can eliminate the performance issue of recursing multiple times into an array.

I'm also in favor of doing this the more performant way with the returned array and list(), or the exception suggested by yched (if this wouldn't be a regular case – but probably that wouldn't be as good). Even passing a reference would be better, in my opinion.
However, if Dries is already set on the three-functions approach: Here is a re-roll taking Jennifer's concerns into account. Only comments were changed, the patch still applied cleanly.

Agreed with #101 and #102. I think this API is a bad idea -- where else in core (or in any other project's decent API) do we require you to call one function to see if another function will work? The pattern of "return success/failure and pass in a reference" is pretty common in PHP, and a much better idea than this one IMO. Or the return-a-list idea, which is a fairly common pattern in PHP (and EXTREMELY common in Perl APIs) as well.

But if the Powers That Be are set on this idea, then I agree that the patch in #102 has fixed the concerns I expressed in #100.

The arguments against an array as return value are moot and have not been justified or clarified further. While the getter may not be used much throughout core currently, it's possible that contributed modules may use it to perform heavy alterations on the renderable $page array on every request, so performance has to be a concern here, and it makes no sense to make something slower just because people fail to read docs.

I've kept the _exists() (<- Function names are too long to remember now), as there's a single use-case.

Given all of the documentation in the get function, I don't think it needs to say "see the set function for details"? I think all the details are right there.

Also:

+ *   - the requested nested value, if existent, or NULL.

List items should start with a capital letter, and I think this should say "if it exists, or NULL if it doesn't".

+ *   - a Boolean indicating whether the nested key exists (TRUE) or not (FALSE).

More concisely: "TRUE if the nested key exists, and FALSE if not". Also should start with capital letter.
Also as a note - see what the exists() function does:

+ *   TRUE if all the parent keys exist, FALSE otherwise.

These two functions should have exactly the same wording for this line.

Incorporated #106.

I've yet to see another critical issue that gets hold off by so many minor documentation issues. Why is it possible to do such improvements in follow-up patches for other issues, but not for this one?

Sorry sun. If you get jhodgdon involved in reviewing patches on an issue, you will have doc wording reviews.

My feeling is that if every issue got "held up" by the documentation issues, i.e. if every patch got reviewed for documentation, then we would (a) have better documentation and (b) have a MUCH smaller documentation issue queue. It's huge right now, and there are only a few people interested in working on it. The follow-up doc issues mostly just sit there. So I think it's much better to get the doc right in the original patch.

But that's what you'd expect the maintainer of the API documentation to say...

Anyway, that patch in #107 is fine with me.

I've yet to see another critical issue that gets hold off by so many minor documentation issues. Why is it possible to do such improvements in follow-up patches for other issues, but not for this one?

Because you're adding new API functions with confusing arguments and confusing return values. Just because a bug is critical doesn't mean we rush in a sub-optimal API docs that might (and given our history, probably) never be fixed.

API docs look good to me now. Dries felt pretty strongly on Sunday that we should have that interim "_exists" function, but I think the performance arguments against that are pretty strong. I still hate the values coming back from the functions mixing "values" and "random status return parameter", but the only real alternative is passing &$ret around like we do in update_sql() and that seems even more sub-optimal.

Since Dries isn't here at the code sprint to discuss, I've decided to commit this to HEAD. But of course if Dries feels differently, feel free to revert.

Yay that this got in. One more critical down!

Sorry I didn't have a chance to review prior to it landing. But there's one key bug in it:

+++ includes/common.inc	24 Aug 2010 13:20:50 -0000
@@ -5582,6 +5582,157 @@ function element_get_visible_children(ar
+    if (isset($array[$parent])) {
+      $array = $array[$parent];
+    }
+    else {
+      return array(NULL, FALSE);
+    }

This returns FALSE for $value_exists even if the final parent key exists, but its value is NULL. This is precisely the condition we need to return TRUE for.

But otherwise, I actually agree with webchick and Dries that it's not ideal DX to return a list from drupal_array_get_nested_value(). Whether we like it or not, PHP uses isset() vs. array_key_exists(), and even if we disagree with PHP's choice in that matter, we should try to be consistent with it, because consistency improves DX (I think). Here's a patch that changes to that, including a consideration where performance needs require the logic to be inlined.

It's an API change, but to an API that's only existed for 5 hours, so I think that's okay.

Powered by Dreditor.

I have a new #fail in the form_example multistep (#8). This worked until today's commit of this issue. I'm not figuring out what happened here.

It's in Form Example Tutorial, #8. (Examples project, #8, which is multistep).

You can see the #fail (with the actual page) in http://qa.drupal.org/pifr/test/26934 - Basically what happens is when the $form_state is reset when the next button is hit, it doesn't seem to show up properly when it gets to the next page.

Hoping this isn't a regression, but I can't figure out how to make the code work correctly.

To make things worse, I can't replicate it in my local, and that's why I put the debug() statements in and committed them (to see on the bot).

For the same reasons stated earlier, I disagree with that change. Why didn't you simply change isset() to array_key_exists() in the getter? That's the bug that needs to be fixed, nothing else.

Although the function argument consistency with array_key_exists() is nice, the resulting function argument inconsistency with the other drupal_array_nested_*() functions is ugly. I'm rather opposed to that change, but I don't find it important either.

I'm going to roll another patch on this later today, unless sun or someone else wants to beat me to it.

I like what jhodgdon said in #103 a lot [referring to #98, not what landed]:

Agreed with #101 and #102. I think this API is a bad idea -- where else in core (or in any other project's decent API) do we require you to call one function to see if another function will work? The pattern of "return success/failure and pass in a reference" is pretty common in PHP, and a much better idea than this one IMO. Or the return-a-list idea, which is a fairly common pattern in PHP (and EXTREMELY common in Perl APIs) as well.

And this is a great rebuttal against my #112 patch. But I still don't like drupal_array_get_nested_value() returning a list. The other functions that we have in Drupal core that return lists are pretty clear about it in their name. entity_extract_ids() obviously returns multiple different ids. The only drupal core function I could find that returns a list despite not making that clear in its name is ajax_get_form() which does not return $form (and is therefore inconsistent with drupal_get_form()), but rather a list of $form, $form_state, and other stuff. But I think that function name/signature combination is unfortunate, and I don't like the idea of introducing a second name/signature WTF into Drupal core.

Plus, $key_exists is not an indication of whether the function worked. Whether the key exists and the corresponding value is NULL or whether the key doesn't exist, both are "working" conditions, and it's legitimate to return NULL either way. Potentially, most callers of this function won't care why the result is NULL. FAPI cares, but it's because FAPI is complex and has to deal with a lot of subtlety.

So, following jhodgdon's alternate suggestion, what do you all think of this:

function drupal_array_get_nested_value($array, $parents, &$key_exists = NULL) {
  $key_exists = TRUE; // or FALSE
  return $value; // could be NULL whether key_exists is TRUE or FALSE
}

Thought I'd get feedback on this, before rolling another patch.

[EDIT: Also, returning lists is common in Perl APIs, but Perl supports better handling of scalar and list contexts, where if the result of the function is being assigned to a scalar, then the first item in the list is assigned to that scalar. Sadly, no such luck in PHP.]

And just to clarify #118, we could still have a wrapper function of drupal_array_nested_key_exists() for code readability where we're not as worried about every microsecond, but in performance-critical code, drupal_array_get_nested_value() could be used to return both pieces of information.

Here's the patch I promised in #118. Ok, tear it apart :)

@rfay: I haven't had a chance yet to look into the details of #113, but it is definitely possible that #107 landing caused a regression with your form example, if that example was making assumptions about $form_state['values'] containing data that is outside of what is allowed by #limit_validation_errors. This issue has an "API Change" tag for a reason, and implementing this security was critical. OTOH, if there's some *unexpected* regression with multistep forms, we certainly want to get to the bottom of that.

re rfay / effulgentsia #113 / #121 : yes, similarly this broke Field UI's formatter settings form on "Manage display" screens - #896162: Write tests for the Field UI formatter settings form. Simple fix.

Bottomline is : if a submit button uses #limit_validation_errors, and its submit handler or some associated multistep form building logic rely on the presence of specific values in $form_state['values'], those sections need to be explicitly included in #limit_validation_errors, You cannot go with a simple '#limit_validation_errors' => array() anymore, or $form_state['values'] comes up empty.

BTW : it seems the explanation for #limit_validation_errors in the PHPdoc for form_set_error() needs to be amended after the changes that got in here :

"... and the submit handlers will run despite $form_state['values']['step2'] and $form_state['values']['step2']['groupX']['choiceY'] containing invalid values"

$form_state['values']['step2'] / $form_state['values']['step2']['groupX']['choiceY'] are now not populated to begin with.

The compromise in #120 would work for me.

Here's what I think we're saying now:

#limit_validation_errors can be used to prevent validation, but the unvalidated values are now discarded. As a result there is no way of preserving the values while skipping validation (a very common use case with a "back" button, of course).

Do I get it now?

If this is correct, then a significant update to both the form_set_error() phpdoc and the Form API reference are in order. Neither explains this.

This also makes #limit_validation_errors quite a lot less useful, as my general usage was to preserve the values and allow later validation. I guess I can see what that might mean. And the title of this issue seems to say as much.

Incorporated #125 and cleaned up a bit further. Also moved the note about PHP's array_key_exists() arguments similarity into the function body, although I originally wanted to remove that entirely, since I was quite confused after reading it and asked myself why I have to read that. ;)

Didn't work on rfay's documentation update request.

+++ includes/common.inc	1 Sep 2010 11:53:33 -0000
@@ -5679,34 +5688,57 @@ function drupal_array_set_nested_value(&
+function drupal_array_get_nested_value($array, $parents, &$key_exists = NULL) {

May I additionally request to return the nested array value by reference?

Doing so allows to skip drupal_array_set_nested_value() in some cases, and I don't see how returning a reference could break anything.

Furthermore, we should add type hinting for $array and $parents to all of the helper functions added here.

Lastly, after having used these functions in a few places already, I'm very annoyed by their needlessly overly lengthy and verbose names. It's totally ugly to type them and have a function name that almost takes up 80 chars on its own. Could we revisit that decision, please? drupal_array_set|get_value() would be sufficient (and also long enough).

Powered by Dreditor.

Can we get this follow-up in first (so that the current function signatures are not too long in the wild) and care for any necessary documentation adjustments afterwards?

+++ includes/common.inc	5 Sep 2010 15:29:35 -0000
@@ -5674,14 +5674,23 @@ function drupal_array_set_nested_value(&
+ * $value_exists = NULL;
+ * $value = drupal_array_get_nested_value($form, $parents, $value_exists);
  * if ($value_exists) {
- *   // ... do something with $value ...
+ *   // ...
  * }

s/$value_exists/$key_exists/

May I additionally request to return the nested array value by reference?

+1. If you think it won't be controversial, put it in now; otherwise, let's leave it to a follow-up.

Can we get this follow-up in first (so that the current function signatures are not too long in the wild) and care for any necessary documentation adjustments afterwards?

+1. I'm fine if my above docs nit waits for a follow-up as well.

fago, jhodgdon, drunken monkey, Frando: any chance one or all of you can give this a quick review, since you had opinions on function signatures in earlier comments?

Fixed that $key_exists. #128 as well as this patch already contains the tiny by-ref tweak.

Thanks, fago! Incorporated the requested changes of #132.

sun, fago, and I like this, so RTBC. Besides, based on earlier comments from Dries and webchick, I think they'll like it too.

Great, now we're talking! Much better, thanks. That third parameter is a bit weird/non-standard, but at least it makes the return values far more sane for the 99% case.

Committed to HEAD.

All that's left on this issue now is #limit_validation_errors docs follow-up as per #123 and #126.

Varible name in example is wrong

Committed that one, too.

Ok, now hopefully this issue will finally stay fixed. :)

All that's left on this issue now is #limit_validation_errors docs follow-up as per #123 and #126.

I have not seen anyone bring this up yet, but isset does not detect the difference between strings and arrays. This means that $ref = &$ref[$parent]; will WSOD if $ref is a string.

Here is a little example from the php contrib manual (http://www.php.net/manual/en/function.isset.php#98109):

$var='string value';
if(isset($var['somekey']){          //it will be treated as true
echo 'This will be printed';
}

This issue causes both drupal_array_set_nested_value and drupal_array_get_nested_value to WSOD if an attempt is made to create a reference within a string.

There is some typehinting in drupal_array_get_nested_value, but, that only works when the initial variable is passed in. If $parents is set incorrectly, and looks too deep into the array, then everything dies.

If we are avoiding checking for arrays with in_array, there is a faster option (http://uk.php.net/manual/en/function.is-array.php#98156), but it is a bit ugly.

#111 broke #913846: Image/file field breaks after uploading two files. I don't know whether it's because of some missing changes in the file ui, or because of a problem in this issue.

This may also be the cause of #913528: Create new boolean field "Cannot create references to/from string offsets nor overloaded objects".

And maybe #935772: The function of the remove image button doesn't work correct too.

I haven't studied in-depth, but I found KarenS also raising some concerns about the implications to Date fields in #929494-15: Everything is broken again.

Shameless plug : #941620: #skip_validation_errors : complement of #limit_validation_errors (D8 feature request)

subscribe. This bug is currently stopping services from having a 7.x release.

@kylebrowning, there are no open tasks on this except documentation. This has already been committed.

I'm not sure that the ground has fully settled on this, and there may have been bugs introduced, but if so they have not been identified. There are almost certainly a number of bugs remaining in core that are due to less-than-adequate usage of #limit_validation_errors.

@rfay, after downloading the latest dev snapshot of drupal7, i now receive this error upon attempting $ret = drupal_form_submit($node->type . '_node_form', $form_state, $node);

Fatal error: Cannot create references to/from string offsets nor overloaded objects in /Users/kylebrowning/Sites/drupal7/includes/common.inc on line 6006

@kylebrowning, then I encourage you to search the issue queue to see if somebody has reported that, and if they have not, file an issue.

This change has appeared to upset a number of contributed modules and the core Image removal functionality..., I got errors when trying to remove an image that also lead to the exact position in the form include. Was this change tested against array based values? I.E. Date, Image, ... Is there something that needs to be added to the element info declaration?

It was wrong to add that "_nested" to the function names. The function names can be barely remembered now. Even though they are being used in many more locations now. Everyone has to look up their names in common.inc or api.d.o... Lots of code is going to be much harder to read and understand. It's as if drupal_render() was named drupal_array_render_element(), or element_children() was named drupal_array_get_nested_element_children(). Sucks. Heavily. Sad sun. :(

@Alan D.: Please see #926016: Several bugs when trying to remove files from a multivalued file/image field, and if you still get bugs with removing images even after that patch, please add a comment on that issue explaining the bug. Please also see #929494-21: Everything is broken again for a discussion about the Date field.

@fago: Care to comment any further on Alan D.'s question. Do you think there's any fix needed here to help make elements with array values easier?

Please see #745590-24: #managed_file element does not work when #extended not TRUE, or when ancestor element doesn't have #tree=TRUE for how this broke #managed_file, and please review the fix in the corresponding patch. Thanks!

Adding tag. It looks like this needs some updates in various API documentation places, as well as an entry in the 6/7 module update page?

I think this issue is only open for documentation, but what needs to be documented is not clear to me.

Could someone please post an issue summary here, with regards to what needs to be documented:
a) Things that need to be documented on drupal.org for developers of new modules.
b) API changes that need to be documented in the 6/7 module update guide for people updating modules from Drupal 6 to 7.
c) Documentation changes that need to be made on api.drupal.org

Thanks...

Since no one has provided any information on what needs to be documented in the update docs, I'm going to try another tactic and ask that the people involved in this issue make a change record node instead. I'm adding a note to the module/theme update pages for d7 to also look at the change list page, because there are already a few d6/7 changes there anyway.

RE #159 - those are probably best filed as one or two separate follow-up issues (drupal core / component documentation for form_set_error(), and documentation project / api docs files, tagged "FAPI reference" for the FAPI reference).

Regarding the change notification, this issue was tagged "api change", so unless that is wrong, it probably needs a change notification?

Initial, partial summary written by btmash.

As long as it is not an API change from one released actual version (not alpha/beta) to another, no we don't need to document it. Thanks for the clarification!

If someone who understands this issue could file the follow-up issues (see last few comments), that would probably allow us to mark this fixed finally.

I agree that this old stale issue doesn't need an API change notice.

However (and it's the wrong issue for this of course) but I don't agree that we only have to document changes between releases. The D7 changes were quite rough... We'd do one thing and then API-change it later, breaking all existing contrib. Once something is established in D8 it needs an API change notice when a change happens.

There is no reason we can't use API change notices for that. Just mark it up correctly and it will be clear when the change happened.

Solving this issue made hard to have Save Draft functionality implemented in D7. I understand that for security reasons approach like http://drupal.org/project/draft is safer, but it fundamentally can not work with more complex field types. Also this change caught many by surprise, recommended solutions for save draft functionality stopped working.

I found a way how to allow some selected fields to be saved unvalidated and I would like to ask if it is acceptable solution at least for D7 or if it is only hack which may easily stop working with any update:

In hook_form_alter add custom validation function like:

array_unshift($form['#validate'], 'mymodule_validate');

and in this custom validation function change #limit_validation_errors:

$form_state['triggering_element']['#limit_validation_errors'] = XXX;

where XXX contains fields which will be removed in drupal_validate_form, and is subset of original #limit_validation_errors. In extreme XXX can be set FALSE to allow all unvalidated values to be saved.

@Stepan Kabele: I'm fairly sure that usage is wrong and that the security team will contact you in case you publish a release that uses the approach. It's basically the kind of possible code that @effulgentsia hinted at in #3.

Why is ability to save node without some selected validations automatically security risk?

Common real world use case: Save Draft feature. Imagine content type with 100+ fields (some large questionnaire). It would be nice allow user to save unpublished node with some selected validations disabled (like required values) as a draft to be finished later, then saved as published with all validations.

@Stepan Kabele:

You cannot use #validate functions to affect required validation because required validation is done before your #validate functions are called.

For a solution that allows skipping required validation without touching other validation check out #1786442-4: Allow saving drafts with missing required fields

Updated issue summary.

Hmm, sorry for bumping, is this still an issue? I'm researching #2313517: Cannot create references to/from string offsets nor overloaded objects, should this issue be moved to D8?

The patches here have been committed a long time ago, and keeping the issue open is just confusing at this point, so I'm going to mark it fixed. If anyone understands what are the documentation improvements requested in #123 / #126, please open a followup :)