Allow suppress of form errors (hack to make "more" buttons work properly)

The idea behind this patch was cooked up after a few hours of brainstorming with quicksketch about one of our fundamental problems: in D6, we can do different validations depending on what we're doing (delete won't enforce business rules about data entry, for example, while submit will). The #required property in particular, though, can't be bypassed during certain kinds of validation, even if we're really only trying to process a small portion of the form and the un-populated #required fields outside that section wouldn't be executed anyways.

This keeps the current behavior as the fallback, and lets buttons/forms supply a small function that determins what part of the form needs to have #required enforced. It also degrades perfectly, since it isn't tied directly to the AHAH handling function. Poll.module's more button, for example, works now even without the Title filled in, with or without JS. Good times.

I'm very curious about chx's thoughts on this approach.

subscribing

looking over the patch the only thing that jumped out at me was the wording of this comment:

+  // Check if a section function is specified to validate only a section of
+  // the form using the built-in validation.

it just seemed really awkward.

I could only agree to this if you would hand only the validated portion to the submit handler. Alternatively, I also suggested moving AHAH processing into #after_build.

webchick tells it's not evident what's my problem. Simple. Validation is the wall against tampered so if we have data that's not validated it must not reach the submit where some careless function might save it expecting the usual security features (option checker for eg)

Things we discussed and agreed on

1. #validate_section can only be used on a button
2. If the pressed button has a #validate_section then it's used but then only the button #submit callbacks fire, the form level do not.
3. #validate_section becomes an array of keys. Should you want to do two parts of the form, you put 'em in a wrapper and theme them apart as necessary.
4. The patch does not get in without the explicit consent of Heine and me.

I'm on the fence regarding #3, but I think those conditions are good ones. The approach of only validating a subsection of the form is a good one, but chx is correct in pushing for the subsection being the only part that is processed in submit callbacks, as well.

Eaton, I have given up on the "partial section to the form handlers". It'd be great if we can make it work but even if we cant the tight coupling of validate_section with its submit handler will, presumedly, avoid any grave mistakes. We shall see what Nate can up with.

Subscribing

There is no point IMO on settings #validate => array() on a button, only #element_validate fires there anyways. I also doubt that you need to change how element_validate is called. Having a possibility to peek at the complete form is good thing still. Noone said we want to validate that. This might make it necessary to change the recursive logic a little bit. The recurse logic needs more comments anyways.

Subscribing! Will review ASAP.

It should be so that if #validate_section is used then only the #element_validate and the #submit handlers defined on the button are fired. No point in enforcing an empty, meaningless empty #validate. I do not know about "Similar to settings #submit on the button prevents form-level submit handlers from firing" I thought that button and form level handlers add up. The changes to _form_validate() are for efficiency -- sorry, but google php reference counting. It's actually slower to pass around by reference.

-          $function($elements, $form_state, $complete_form);
+          $function($elements, $form_state, $form);

my problem is there, we previously had the complete_form and I would keep that. Or you are saying this is still so just your reorganization made it so that now $form is the complete form?

Anyways, the recursing logic needs comments, (empty($validate_section) || $validate_section[0] == $key) this part.

Very nice job. I am absolutely happy now, let's see what Heine has to say on this. I doubt we opened a security hole in here but let's see whether he can find some.

Minutes (summary) after a discussion in IRC a while ago.

The patch in #27 contains this gem:

 function poll_more_choices_submit($form, &$form_state) {
   // Set the form to rebuild and run submit handlers.
-  node_form_submit_build_node($form, $form_state);
+  if (drupal_function_exists('node_form_submit_build_node')) {
+    node_form_submit_build_node($form, $form_state);
+  }

This demonstrates how easy it is to accidentally get unvalidated values at the formbuilders or hook_form_alter; #node will contain non-validated data after node_form_submit_build_node has run.

To prevent such mistakes, $form_state['values'] should only contain validated values (as suggested earlier by chx). Chx suggested that form_set_value was to be used in validation to have an automatic safe $form_state['values'].

Right, I suggested moving the form_set_value call to validation so that only items validate gets set. Only #after_build is affected which is a weirdo edges case and can work from form['#value'] . @Heine, about how easy to get invalidated stuff somewhere, why do you think I said this patch does not get in w/o your approval?

Several existing issues cover other aspects of partial and skippable validation. I suspect many could be marked as dupes, but I'm not clear if any existing patch covers these other needs.
#280465: #required (and #element_validate too?) not skippable
#332614: allow buttons like 'reset' to bypass validation

Heine, even without this patch, if something fails validation it stays in form_state['values'] because form_set_value is run on build and validate does not change it.

subscribing

the summary list of followups:
#370823: Allow Nodes to Be Previewed Without Required Fields
#216064: Entity form "Delete" button triggers server-side + HTML5 form validation; change "Delete" button to a link
#280465: #required (and #element_validate too?) not skippable
#370819: Upgrade Upload Module to FAPI3

subscribe.

subscribing, will review in, ahem, 2 weeks

subscribing.

I've pulled together a module which allows the skipping of the validation procedures by clicked button for D6.

http://drupal.org/project/skip_validation

Manual re-roll against latest HEAD, accounting for new ajax.inc and plenty of other Form API changes. Just to see what the bot thinks.

Hmm, the node_form_submit_build_node() pattern is something we generalized to all entities as they were made fieldable, so that they could support 'Add more' : taxo terms, comments - not users for now, because user form code was a little scary back then, dunno if the recent cleanups made that area cleaner.
#367006: [meta] Field attach API integration for entity forms is ungrokable was open to get feedback on the approach, but I guess the issue title was not really enticing...

/me wonders if he should run and hide :-/

It will do until we clean up node - form relationship in D8 if we do.

tagging

I can't stand Eclipse patches because they don't say what function they're from. Here's a straight re-roll of #56 for legibility.

This actually is what I'd consider a critical D7 API clean-up, since the UX for AHAH forms is pretty bad without this. For example, uploading a file when you haven't filled out the node title yet gives you an error at the file upload field telling you that node title is required.

Oh. Excuse the size of the patch. I review a lot of patches. :P

+++ includes/form.inc	13 Oct 2009 06:53:27 -0000
@@ -833,7 +833,7 @@ function _form_validate($elements, &$for
       // A simple call to empty() will not cut it here as some fields, like
       // checkboxes, can return a valid value of '0'. Instead, check the
       // length if it's a string, and the item count if it's an array.
-      if ($elements['#required'] && (!count($elements['#value']) || (is_string($elements['#value']) && strlen(trim($elements['#value'])) == 0))) {
+      if ($elements['#required'] && !_form_validate_skip($elements, $form_state) && (!count($elements['#value']) || (is_string($elements['#value']) && strlen(trim($elements['#value'])) == 0))) {

I don't understand the added count() check, and the inline documentation wasn't updated to explain it.

+++ includes/form.inc	13 Oct 2009 06:53:27 -0000
@@ -883,6 +883,36 @@ function _form_validate($elements, &$for
+function _form_validate_skip($element, $form_state) {
+  $skip = FALSE;
+  if (isset($form_state['clicked_button']['#validate_section'])) {
+    $parents = $form_state['clicked_button']['#validate_section'];
+    foreach ($parents as $key => $parent) {
+      if ($element['#parents'][$key] != $parent) {
+        $skip = TRUE;
+        break;
+      }
+    }
+  }
+  return $skip;

We could use some inline documentation here that explains how exactly this recursive #parents skipping check is supposed to work.

This review is powered by Dreditor.

That sounds like a "needs work."

Patch needs at least one test? Might be as easy as adding another file upload and making sure there is no validation error message.

File handling is too complex for this test.

#622922: User input keeping during multistep forms contains a simple form constructor with multiple "Add more" buttons. After that patch landed, we can simply re-use that by adding a #required element to the form.

.

Re-rolled against HEAD and added comments.

This looks ready to fly to me.

Revamped the comments once again, 'cause I didn't like them.

Discussed this some more with chx in IRC.

Writing this test felt PLAIN AWESOME.

Also note that I renamed #validate_section to #validate_parents. There's absolutely no need to invent a new term here. The value of #validate_parents is an array of #parents of elements to validate. And #parents on its own is an essential Form API concept that every Drupal developer should know.

Added one more assertion to make this test even more bullet-proof.

+++ includes/form.inc	2 Dec 2009 12:09:46 -0000
@@ -920,6 +920,65 @@ function _form_validate(&$elements, &$fo
+ * For example, this is used to skip validation of #required and
+ * #element_validate properties for other elements in the form (such as a
+ * required title) and process submitted form values to add another item
+ * belonging to the field of the pressed submit button.

This paragraph is confusing to me. How about:
----------
For example, when an "add another item" button belonging to a field is clicked, we don't want a missing required title causing a validation error and preventing a new item from being added to the form. Setting that button's #validate_parents property to the #parents of the field will result in only elements within that field being validated when that button is clicked.
----------

+++ includes/form.inc	2 Dec 2009 12:09:46 -0000
@@ -920,6 +920,65 @@ function _form_validate(&$elements, &$fo
+  // Check whether the pressed button defined form element parents to validate
+  // and a button-level submit handler to execute. If it didn't, then form
+  // submit handlers would potentially get unvalidated values, so we cannot
+  // allow this.

Check whether the pressed button defines #validate_parents to limit validation to elements within those parents. We prevent insecure execution of form-level submit handlers, by not supporting limited validation unless a button-level submit handler is defined.

+++ includes/form.inc	2 Dec 2009 12:09:46 -0000
@@ -920,6 +920,65 @@ function _form_validate(&$elements, &$fo
+  // defined in the pressed button. If this element's parents are not contained
+  // in the defined parents, then validation for this element can be skipped.

This is confusing to me, because "contained in" can be interpreted as "this element is a descendant of" as well as "there is an intersection between #parents and #validate_parents". Would it help to call the property "#validation_target", and then explain it as "if the validation target is not an ancestor of the element"?

+++ includes/form.inc	2 Dec 2009 12:09:46 -0000
@@ -920,6 +920,65 @@ function _form_validate(&$elements, &$fo
+    if ($element['#parents'][$key] != $parent) {

Why #parents instead of #array_parents? Do we want #tree=FALSE to influence this logic in the way that using #parents suggests? If so, can there be a comment explaining why we want that?

This review is powered by Dreditor.

Sorry, wrong post.

Thanks! Incorporated those suggestions.

Why #parents instead of #array_parents?

This is all about user input validation, i.e. the now well-known $form_state['input'] and $form_state['values'] ;) (as also the @todo for D8 explains), and #parents define the parents for submitted form values.

err. Fixed a stale, err, missing comment in there ;)

Looks ready to fly for me now.

+++ includes/form.inc	3 Dec 2009 11:08:48 -0000
@@ -920,6 +920,68 @@ function _form_validate(&$elements, &$fo
+ * validation error, preventing a new item from being added to the form. By
+ * setting that button's #validate_parents property to the #parents of the field
+ * will result in only elements within the field being validated when the button
+ * is clicked.

Grammar: either remove "By" or rephrase second part of sentence.

+++ includes/form.inc	3 Dec 2009 11:08:48 -0000
@@ -920,6 +920,68 @@ function _form_validate(&$elements, &$fo
+ * @todo D8: This is should live in _form_builder_handle_input_element(),
+ *   because when taking a step back, we want to skip input processing for
+ *   unrelated elements in reality. However, we currently cannot skip input

Grammar, but also "in reality" seems superfluous. How about: This should live in _form_builder_handle_input_element(), because ideally, we should skip not just validation, but all input processing for unrelated elements.

+++ includes/form.inc	3 Dec 2009 11:08:48 -0000
@@ -920,6 +920,68 @@ function _form_validate(&$elements, &$fo
+  // defined in the pressed button. If the parents defined in #validate_parents
+  // are not ancestors of this element's parents, then validation for this
+  // element may be skipped.

What's still tripping me up is the use of plurals, because even though #parents and #validate_parents are arrays, they refer to something that is conceptually a singular entity (each array defines a singular point in the $form_state['values'] tree, and what we're trying to determine is if the one point in the tree defined by #validate_parents is an ancestor to the one point defined by $element['#parents']). How about: By setting #validate_parents, the button has indicated that it only needs that portion of $form_state['values'] to be validated. If the element's #parents property is such that the element's value resides in a different part of $form_state['values'], then validation for this element may be skipped.

This review is powered by Dreditor.

yay, nitpicks! :)

Thanks!

Any chance to get this in?

Though it does not appear in later comments this approach has my approval.

made #validate => array() to be filled in automatically.

Had a long conversation with chx in IRC about this patch.

My main concern is that I think module developers are not going to get this right, and accidentally bypass validation when they think they are in fact validating.

#validate is a "developer-facing" property. It's something most developers will end up using quite often through the course of their module building. #parents, on the other hand, is largely an internal, "form.inc-facing" property. I don't think I've ever once encountered a situation where I needed to futz with #parents, and I've written my fair share of forms. :P

#validate_parents has two issues:

1. It's not at all descriptive of what it does, which is "send only this small chunk of the form for validation, bypassing the normal form validation process"
2. It requires specifying the entire parents array in order to get the validation to work as you'd expect. For example, if you had the following form:

$form['foo']['bar']['bazzle'] = array(
  '#type' => 'textfield',
);
$form['foo']['bar']['quxxie'] = array(
  '#type' => 'textfield',
);

You would need to add #validate_parents = array('foo', 'bar') in order for this chunk to be validated. Not the fields themselves -- array('bazzle', 'quxxie') -- and not just the immediate parents of the fields -- array('bar') -- but the entire hierarchy array.

Not having to blank out #validate yourself certainly helps, so #80 is an improvement over #78. But I still think people are going to get this wrong. :( However, I also have no bright ideas on how to address this, and certainly want to see this fixed. I'll leave it until Boxing Day and hope we have a Christmas miracle. ;)

One another issue would be to use a reference to the actual form parent but then you can get a recursive form array easily and I am VERY uncomfortable letting that happen before we raise PHP to 5.3.0 with it's circular reference collector.

@webchick: I'm sad to hear that the PHPDoc for _form_validate_skip() and the code comments within it fail to explain the concept well enough, but I agree with you that the concept isn't explained so clearly that developers will just "get it" and use it correctly.

Here's a stab at explaining it: maybe something here can be useful towards whatever further documentation is needed.

You're building a form, and have a button on it that will fire a button-level submit handler instead of the form-level submit handler. That submit handler does not need the entire form to be valid, and for best UI, specifically does not want the entire form to be validated. The submit handler will be working with some part of $form_state['values'] (in your example, $form_state['values']['foo']['bar']) and needs that part of $form_state['values'] to be valid. So, that button needs to set #validate_parents to array('foo', 'bar'), which means: make sure that everything within $form_state['values']['foo']['bar'] is valid, but don't validate any other part of $form_state['values'], because it's not needed for what the submit handler needs to do.

Does this explanation help, and if so, does it point to needing a better name for #validate_parents, or just better documentation somewhere?

By the way, I'm a little torn on forcing #validate to exist, and therefore always bypassing form-level validation handlers, even without the developer explicitly setting #validate. I understand why you don't want form-level validation handlers running, since they may be validating a part of $form_state['values'] outside of what's specified by #validate_parents, but OTOH, they may also be validating a part of $form_state['values'] that is within #validate_parents, so not having them run without the developer explicitly setting #validate seems to me like being more prone to developers thinking something is being validated when in fact it isn't. Given that you don't know what a form-level validation handler will do, if chx, webchick, and others think it's best to never have them run in partial-validation mode, then I can accept this, but because of this concern, I'll leave it to someone else to RTBC.

My POV on the raised issues is this: If you wrote your fair share of forms, and you did not really have to deal with #parents in those forms yet, then you also won't write a form that requires #validate_parents.

Really, Form API has a lot of internal properties due to its awesome flexibility. The more awesome stuff you want to do, the more you have to understand about it. That's nothing new and probably the same with any other API we have in core and in the wild.

Since this particular feature is touching security-related functionality, I'm perfectly fine with this approach, which may not be "easily grokable at first sight", but that's perfectly OK, because if you want to do this, then you really should know what you are doing.

For the very same reason, like effulgentsia, I'd prefer to go back to the previous patch.

We can clarify the PHPDoc, if that's required. But I personally don't want this to be too easy. Which doesn't mean it wouldn't be easy, but requires at least a certain understanding of what you are doing with your form button.

Right; I should clarify... the docs for _form_validate_skip() are great. But these are buried in form.inc, in the headers of a private, internal function. I don't know how an average module developer is ever going to find those docs when seeking the solution to this issue.

Your average module developer is going to go, "I need to add an AJAXy button pressy thing and don't want the title field screaming at me to fill it out. I noticed Poll module does this. Let me go see what Poll module does." And that module developer is probably going to be looking around the Poll's "add more" button for a property like #validate_this_section_only = TRUE.

When that is not found, they'll look at the rest of the properties there and by process of elimination, probably figure out that #validate_parents is the magical sauce that makes it work. Then they'll proceed to bash their heads trying to figure out why Poll module only had to pass an array with one index but they have to pass in an array with multiple, since bazzle and quxxie are two levels deep. (Though the upshot is that this head-bashing might cause them to grep the source for #validate_parents and come across the nice docs in _form_validate_skip(). ;))

However, nowhere else in the form API that I'm aware of do we link two dependent properties together in order to achieve a certain behaviour, so I'm not sure how they would also figure out that they need to add #validate = array() as well, without reading this patch. This is why #80 is preferable to me over #78.

But even more preferable to me would be a simple TRUE/FALSE property and have Form API just be smart about it, so that a human being doesn't need to re-iterate to Form API the form structure she has already written elsewhere.

I agree with sun that I'm not the target audience of this patch. But OTOH, I remember rampant mis-use of the #DANGEROUS_SKIP_CHECK property, back when that was around to deal with AJAX validation issues, so I think you are over-estimating peoples' ability to keep themselves out of trouble. ;)

Didn't mean to reset status.

We can do something else: add the name of the button to the section to be validated. This avoids #parents which is good because you do not always have #parents set, what if you have #tree FALSE and just want to bundle a bunch of elements under a wrapper? We can do that. (And I do not think the verbose and obtuse name of the new form_state/element property is good but we can come up with a better name)

Edit: this is a brand new patch using nothing from previous efforts. Some credits go to webchick because she said "What I want is like #validate_this_chunk = TRUE and form API to just figure nit out".

Added a comment to the form_execute_handlers change. Also it occured to me that this approach allows you to validate as many sections as you want.

This seems like a much, much simpler approach to me, it's just a few if/else changes in form.inc, and the form definition is really simple.

Hopefully we can come up with something better than #validate_exclusively_on_button. Would just #validate_on_button be enough?

While #90 is nicely simple, I don't think it's what's desired. It doesn't make sense (at least to me) for elements to know which buttons they should be validated on. IMO, since the button specifies the #submit handler, it should be the one to specify what needs to be validated given the needs of that handler.

I think one reason why #78 isn't meeting with immediate approval is that we don't yet have within Drupal a standard API or nomenclature for working with trees, and increasingly, Drupal programming requires working with trees. So, this patch adds that much needed API and nomenclature (the hunks in common.inc). It also updates a bunch of Drupal code unrelated to partial validation to use that API (to provide use-cases for the new API and demonstrate why it's handy). Finally, it re-implements #78 with that API.

In the re-implementation, it changes #validate_parents to #validate_partial which is an array of information about what kind of partial validation is wanted. #validate_partial['sections'] is an array of sections (note in #78, #validate_parents defined a single section, so this is a feature expansion, but a nice one, I think) within $form_state['values'] that must be valid. Each section in the array needs to be a tree path (see common.inc) in any one of the supported tree path formats (e.g., array('a', 'b', 'c') OR 'a/b/c' OR 'a[b][c]'). This will need to be documented, so I added a @todo in _form_validate_skip() to document #validate_partial wherever we end up documenting #validate and #element_validate.

Also, I added a #validate_partial['skip_ancestors'] to handle the issue brought up in #82-#86. It may be the case that 99% of use-cases will want to set this to TRUE, but I still think it should default to FALSE (how it is in this patch), because setting it to TRUE might result in the specified sections of $form_state['values'] not being fully valid (see #85 and code comments in _form_validate_skip()). And turning off a security precaution should be done explicitly, not by default. The reason I say 99% of use-cases will probably set this to TRUE, is that I think most use-cases using #validate_partial at all will have submit handlers that don't do anything risky (just AJAX wizziness), but this is just a guess, and I'd rather err on the side of tighter security.

Buttons and the sections are closely interrelated enough that I still think that my patch is better. #92 is 8.x material IMO.

+++ modules/field/field.form.inc	2009-12-25 07:07:21 +0000
@@ -227,6 +227,7 @@ function field_multiple_value_form($fiel
+        $field_elements[$field_name]['#validate_exclusively_on_button'] = array(t('Add another item'));

This approach is ambiguous, because there can be multiple multiple-value fields in a single form. By default, all of those fields have a submit button with the label "Add another item".

I also don't like this approach, because it applies form #validate logic to a form structure - while we are currently applying such logic to the form buttons or the entire form only.
It might make sense to change that in D8, but then we should change it for all #validate + #submit properties - consistently.

In effect, I still would recommend to go back to #78, which only seems to be blocked by documentation issues. Aside from still searching for a better solution for it, the Form API reference documentation for developers/implementors lives in http://api.drupal.org/api/drupal/developer--topics--forms_api_reference...., which is where all of theses properties are explained in detail. Of course, #validate_parents would have to be documented there as well. Maybe that wasn't entirely clear yet?

I do not like validate_parents because it depends on parents and what will you do if you just want to wrap things on containers for rendering and not change their parents? If my solution is problematic because of the buttons have same labels -- good catch -- then add more information in there.

and what will you do if you just want to wrap things on containers for rendering and not change their parents?

errr... exactly the same as you need to do now?

Your consideration would be valid if this was based on #array_parents, but it is not. Hence, there is no difference to all of the current principles for changing a form structure.

If I have $form['foo'] and $form['bar'] and want to validate these and these only how do you specify that? They do not have a common parents "root".

Re: #97, I think we have 3 options:

1) Consider that out of scope for D7. Is there a real use-case for it?
2) Go with #92, which supports it.
3) Change #78 from #validate_parents to #validate_sections, which would be an array of #parents arrays: in the example above, you would set #validate_sections = array(array('foo'), array('bar')).

I agree with sun that I'm not the target audience of this patch. But OTOH, I remember rampant mis-use of the #DANGEROUS_SKIP_CHECK property, back when that was around to deal with AJAX validation issues, so I think you are over-estimating peoples' ability to keep themselves out of trouble. ;)

At its core, FormAPI was designed for monolithic validation/processing of a single $_POST that maps to a single form, and results in success or failure. Period. End of story. We've grafted a lot of behavior onto it since then, but in this issue we are hitting the wall.

We have three conflicting desires:

• Robust automatic security
• À la carte validation
• Ease of use/discovery for developers

I could be wrong -- someone may have an elegant solution that achieves all three. But I have the strong suspicion after spending some time thinking through the early iterations of the patch, and catching up on the more recent ones, that we're going to need to choose two of those and be happy with them until we can dedicate the time and energy for a deeper rewrite of FAPI's internals. Until we can prioritize those three desires I think we're doomed to keep patching in circles.

Automatic security was a consideration both the validate_parents and my patch. A la carte validation was provided by both although my patch provides bigger flexibility however changing validate_parents to validate_sections (aka use an array of #array_parents sections) provides equal flexibility. Easy of use is debatable :)

Ok, here's an attempt to take everyone's wishes into account. I hope you like it.

While being a nice and easy idea, the approach in #101 does not support #97 at all. Aside from that, I really don't like the relativeness in it; it forces the button to have the same #parents as the elements that should be validated.

If we absolutely have to support #97, then I'd suggest to take #78 and just do the change to a list of multiple #parents, i.e.

  '#validate_parents' => array(array('foo'), array('bar'),

Optionally rename to #validate_sections.

While being a nice and easy idea, the approach in #101 does not support #97 at all

Please explain. To my eyes, it's the same as your above code block with #validate_parents renamed to #validate_sections (because #validate_parentses would be kind of weird). It allows a numeric constant for relative-based assigning, because of the desire expressed in #87. But you don't have to use a numeric constant, you can use array('foo') if you want to, and the PHPDoc includes that and the poll use-case uses it. And you can mix-and-match, so for example: #validate_sections = array(VALIDATE_SECTION_PARENT, array('foo'), array('a', 'b', 'c')).

Of all the options discussed, I like #98.3 the most (having a #validate_sections = array(array('foo'), array('bar', 'baz')) property on the buttons that require partial validation). I think it's the easiest to explain within the current FAPI architecture.

OK, on it.

Um, just to be clear, #101 is:

• #98.3
• Plus the ability to use an optional easy constant to address common use-cases as wanted in #87
• Plus the skipping of the form's #validate handlers as wanted in #82
• Plus loads of documentation to address #83

@chx: if you post an alternate patch, can you please also comment as to what you don't like in #101.

effulgentsia , could you please move your superb nice comments to this one? a) you are skipping validation and skipping at the wrong place. I allow instead and at a much nicer place b) the tests are seriously ugly c) biggest problem you use parents but array_parents is what you want. I removed grandparent constant too.

We are now covering two cases nicely: one is a node-like form where the buttons are in a buttons contains and they need to validate a bit of this and a bit of that and the field-case where we generate a complex form subtree where we need to validate only ourselves.

+    if (is_int($section) && $section < 0) {

Could use a code comment that this is where VALIDATE_SECTION_PARENT is handled ?
Seems surprising that the constant is assigned in actual code or tests but never actually checked against, so there's no clear semantic readable in code.

+++ includes/form.inc	2009-12-28 17:18:55 +0000
@@ -884,7 +890,7 @@ function _form_validate(&$elements, &$fo
   // Recurse through all children.
   foreach (element_children($elements) as $key) {
-    if (isset($elements[$key]) && $elements[$key]) {
+    if (_form_validate_allowed($elements[$key], $form_state)) {
       _form_validate($elements[$key], $form_state);
     }
   }

So, if #validate_sections = array(array('foo', 'bar')), won't _form_validate_allowed($form['foo'], $form_state) return FALSE, and we won't recurse to even check if it's allowed for $form['foo']['bar']?

+++ includes/form.inc	2009-12-28 17:18:55 +0000
@@ -947,6 +953,27 @@ function _form_validate(&$elements, &$fo
+    if (is_int($section) && $section < 0) {
+      $section = array_slice($form_state['clicked_button']['#array_parents'], 0, $section);
+    }
+    if (count($element['#array_parents']) >= count($section) && array_slice($element['#array_parents'], 0, count($section)) === $section) {
+      return TRUE;
+    }

I asked in #73 if we wanted to use #array_parents, and sun felt strongly in #75 and later comments that no, we want #parents, and #101 makes a point to comment this.

+++ includes/form.inc	2009-12-28 17:18:55 +0000
@@ -967,8 +994,9 @@ function form_execute_handlers($type, &$
-  // Otherwise, check for a form-level handler.
-  elseif (isset($form['#' . $type])) {
+  // Otherwise, check for a form-level handler. If a per-button validate is in
+  // effect then neither the validate nor the submit handler should fire.
+  elseif (isset($form['#' . $type]) && empty($form_state['clicked_button']['#validate_sections'])) {
     $handlers = $form['#' . $type];
   }

Nice!

I'm on crack. Are you, too?

So, if #validate_sections = array(array('foo', 'bar')), won't _form_validate_allowed($form['foo'], $form_state) return FALSE, and we won't recurse to even check if it's allowed for $form['foo']['bar']? <= Oh. that's one valid concern! If you can fix it, please, I am on update tests now or I can fix tomorrow.

I'm really busy on other work for the rest of the day, and actually, the rest of the week. I think we're making progress here, and the next step will involve merging the best aspects of #101 and #109 with each other, and have that be the next proposed patch. Until whenever someone has the time to do that, some additional community feedback would be most welcome:

Major unresolved issues:

• sun believes (#75, #96) that the conception of what makes an element in a section is where its values live in $form_state['values'] (i.e., based on #parents, and how it's implemented in #101). chx believes (#109) that it should be based on where the element lives within $form (i.e., based on #array_parents). I don't think any further progress can be made without this being settled.
• I'm not sure, but I'm interpreting sun's comment in #102 to mean he doesn't like the idea of having a VALIDATE_SECTION_PARENT constant be an option as a section identifier within #validate_sections, and would prefer that the full array of keys from root be the sole way of specifying the section. But in #87, webchick is asking for the system to support some kind of intelligence based on where the button is in the structure. Both #101 and #109 support this constant, but if sun's against it, more feedback and discussion is worthwhile.

Minor issues:

• #101 calls the function that determines if the element's validation should be run/skipped _form_validate_skip(). #109 calls it _form_validate_allowed(). I really don't care. Does anyone want to express a preference?
• #101 and #109 differ on from where within _form_validate() the above function gets called. #101 passes bot, #109 doesn't. I'm sure this can get resolved in a way that works for everyone.
• I prefer #109 with respect to how it stops form-level submit handlers from running.
• #109 removes the VALIDATE_SECTION_GRANDPARENT constant that's in #101, since maybe it's too gratuitous. I'm okay with this.
• #101 has better comments. But they may need to be updated based on how the major unresolved issues in the above list get resolved.

Let's hope this passes the bot.I made a typo after running tests and that's why it failed. There is no change #114 stil stands etc.

I discussed with sun and he said validate/submit revolves around submitted form values. He is right. Now, despite internally this is handled by using #parents there is no point in exposing this so I renamed the controversial property to #validate_values and gave examples using $form_state['values']. That's nice and easy to explain and no crazy #parents stuff is involved in the explanation. Surely there is in the implementation. I have added lots comments based on previous patches and I think this is a very happy patch now.

So to recap #114: we use $form_state['values'] and everyone agrees here now I think. There is no need or point of having a VALIDATE_SECTION_PARENT constant -- we never used such constants and it does not reflect in $form_state['values'] anyways.

The structure of form_validate is simpler with _form_validate_skip so that's what we use. The code doing that is very simple, too.

I changed an empty to !isset making it possible to set #validate_values to an empty array for the Previous button case I mention in the comments (if you have a multistep form with a Previous button you certainly do not want any validation to fire pressing that).

Thanks!

+++ includes/form.inc	2009-12-29 11:36:46 +0000
@@ -879,15 +879,16 @@ function drupal_redirect_form($form_stat
+  if (_form_validate_skip($elements, $form_state)) {
+    return;

Add a short inline comment above this condition, so people trying to understand the flow don't have to look up that function? That comment should also clarify why we recurse into all children, regardless of whether a parent is skipped (#parents).

+++ includes/form.inc	2009-12-29 11:36:46 +0000
@@ -947,6 +948,46 @@ function _form_validate(&$elements, &$fo
+ * #validate_parents property on buttons to avoid the validation of some

Stale #validate_parents

+++ includes/form.inc	2009-12-29 11:36:46 +0000
@@ -947,6 +948,46 @@ function _form_validate(&$elements, &$fo
+ * elements. For example, pressing the Previous button should not fire
+ * validation errors just because the current step has invalid values.

Wrap "Previous" in quotes. Perhaps also mention AJAX here?

+++ includes/form.inc	2009-12-29 11:36:46 +0000
@@ -947,6 +948,46 @@ function _form_validate(&$elements, &$fo
+ * #submit handlers won't fire. Button level #validate and #submit handlers
+ * will fire and there should be extreme care taken because

s/form level/form-level/

s/fire/executed/

s/Button level/Only button-level/

+++ includes/form.inc	2009-12-29 11:36:46 +0000
@@ -947,6 +948,46 @@ function _form_validate(&$elements, &$fo
+ * typically is not a problem as these buttons (like Previous or Add more) do
+ * not persist anything. Do not use this property if the button triggers saving

Quotes around "Previous" and "Add more".

Not sure whether "do not persist anything" is actually correct.

+++ includes/form.inc	2009-12-29 11:36:46 +0000
@@ -947,6 +948,46 @@ function _form_validate(&$elements, &$fo
+ * This property is a list of form_state['values'] keys. For example

Missing $ before form_state.

Missing colon (:) after "example".

+++ includes/form.inc	2009-12-29 11:36:46 +0000
@@ -947,6 +948,46 @@ function _form_validate(&$elements, &$fo
+ * $form['submit]['#validate_parents'] = array(
+ *   array('foo', 'bar'),
+ *   array('step1'),
+ * );
+ *

Missing @code + @endcode. Ideally, embedded code should have an extra indent of 2 spaces.

Extra blank line afterwards can be removed.

+++ includes/form.inc	2009-12-29 11:36:46 +0000
@@ -947,6 +948,46 @@ function _form_validate(&$elements, &$fo
+  // The complete form does not have #parents set. However, the complete form
+  // validation fires #validate handlers set on the clicked button so in this
+  // case FALSE must be returned.

I still have issues to understand this comment. What exactly do you want to say? :)

+++ includes/form.inc	2009-12-29 11:36:46 +0000
@@ -967,8 +1008,10 @@ function form_execute_handlers($type, &$
+  // Otherwise, check for a form-level handler. If only some elements were
+  // validated because #validate_values was set then neither the validate nor
+  // the submit handler should fire.

I think we can shorten this to:

"Otherwise, check for a form-level handler, but only if the complete form was validated."

+++ modules/simpletest/tests/form.test	2009-12-29 10:24:53 +0000
@@ -231,6 +231,20 @@ class FormValidationTestCase extends Dru
+   * Test button press validates only certain values.

"Tests partial form validation."

+++ modules/simpletest/tests/form.test	2009-12-29 10:24:53 +0000
@@ -231,6 +231,20 @@ class FormValidationTestCase extends Dru
+  function testValidateValues() {
+    $edit = array('test' => 'pass');
+    $path = 'form-test/validate-values';
+    $this->drupalPost($path, $edit, t('Partial validate'));
+    $this->assertNoText(t('!name field is required.', array('!name' => 'Title')));
+    $this->assertText('Test is validated');
+    $this->drupalPost($path, $edit, t('Full validate'));
+    $this->assertText(t('!name field is required.', array('!name' => 'Title')));
+    $this->assertText('Test is validated');
+  }

Can we squeeze some inline comments in here to explain what is being tested?

+++ modules/simpletest/tests/form_test.module	2009-12-29 10:25:20 +0000
@@ -203,6 +210,37 @@ function form_test_validate_form_validat
+function form_test_validate_values($form) {

Missing PHPDoc.

Form constructor functions should end in "_form".

+++ modules/simpletest/tests/form_test.module	2009-12-29 10:25:20 +0000
@@ -203,6 +210,37 @@ function form_test_validate_form_validat
+ * Element handler for the test element.

"Element validation handler for the 'test' element."

I'm on crack. Are you, too?

Calling every form builder _form is stupid and leads to incomprehensible form_alters. But I won't fight that here so I did everything.

Poll module, son of evil: changes $node in the validate handler. We can fix that of course.

+++ modules/field/field.form.inc	2009-12-29 09:33:41 +0000
@@ -214,6 +214,7 @@ function field_multiple_value_form($fiel
+          '#validate_values' => array(array($field_name)),

If we want to be strict, this should be array(array($field_name, $langcode)) (hypohetical case of a form spanning several translations)

I'm on crack. Are you, too?

Fixed stupid typo (of course it is the _form thing!) and added $langcode. Thanks yched.

+++ includes/form.inc	2009-12-29 15:13:16 +0000
@@ -879,15 +879,23 @@ function drupal_redirect_form($form_stat
+  // Check whether validation needs to be skipped according to the
+  // #validate_parents of the clicked button. This can not be moved above
+  // as we must recurse to all form elements even if themselves are not
+  // validated because #validate_parents can contain something like
+  // array('foo', 'bar') and then $form_state['values']['foo'] itself won't be
+  // validated but if recursion skips it, then
+  // $form_state['values']['foo']['bar] would be skipped too.

s/#validate_parents/#validate_values/

+++ includes/form.inc	2009-12-29 15:13:16 +0000
@@ -947,6 +955,52 @@ function _form_validate(&$elements, &$fo
+ * will validate $form_values['step1']['choice'] as the first key is step1.
+ * Anything under $form_values['step2'] won't be validated. $form['foo'] won't
+ * be validated but everything under $form['foo']['bar'] will be.

s/($form_values|$form)/$form_state['values']/g

+++ includes/form.inc	2009-12-29 15:13:16 +0000
@@ -947,6 +955,52 @@ function _form_validate(&$elements, &$fo
+  // The complete form does not have #parents set. However, _form_validate()

Is this true? Does it not have #parents = array()?

+++ includes/form.inc	2009-12-29 15:13:16 +0000
@@ -947,6 +955,52 @@ function _form_validate(&$elements, &$fo
+    if (!$form_state['clicked_button']['#validate_values']) {
+      return TRUE;
+    }
+    foreach ($form_state['clicked_button']['#validate_values'] as $section) {
+      if (array_slice($elements['#parents'], 0, count($section)) !== $section) {
+        return TRUE;
+      }
+    }

This seems faulty, because it means we're skipping if the element is outside 1 of the entries in #validate_values, whereas we only want to skip if it is outside all of them. How about iterate on #validate_values, and if there's a match, return FALSE. At the end of iteration, return TRUE. This would also mean that #validate_values=array() would work as desired without making it have its own if statement.

I'm on crack. Are you, too?

There is no need or point of having a VALIDATE_SECTION_PARENT constant

I'm okay with removing it if webchick is. She's the one who wanted some support for the system just figuring out what to validate based on where the button is. We could rename the constant to something appropriate if that's the issue (given the change from #validate_sections to #validate_values). Or, if she's willing to give this up, then not support relativity. I see no conceptual difference, however, with relativity based on #parents or #array_parents. Buttons have both, and they mean the same thing for a button as they do for every other form element. If we're not gonna support this, there should be a clear reason for why not, such as the desire to have hook_form_alter() implementations move a button to a different part of the form (in a way that changes #parents in addition to #array_parents) without worrying about how that will affect validation. However, I'm not convinced by that argument, because if the argument makes any sense, then it should also apply to other elements, and it's already the case that if hook_form_alter() moves elements in a way that changes #parents, then validation and submit logic is affected due to $form_state['values'] being affected.

@effulgentsia supporting relativity is just complexity. As #parents are known even if #array_parents are not the current solution is sufficient. How about iterate on #validate_values, and if there's a match, return FALSE <= no way, if you have array('foo', 'bar') in #validate_parents then array('foo') as parents needs to be skipped. Please check the complete form but I doubt it has #parents set -- #parents is set when iterating the children.

About the three test failures, dblog is using poll... on them.

+++ includes/form.inc	2009-12-29 15:13:16 +0000
@@ -947,6 +955,52 @@ function _form_validate(&$elements, &$fo
+    foreach ($form_state['clicked_button']['#validate_values'] as $section) {
+      if (array_slice($elements['#parents'], 0, count($section)) !== $section) {
+        return TRUE;
+      }
+    }

#validate_values = array(array('foo'), array('bar', 'baz'))
$element['#parents'] = array('bar', 'baz') should not be skipped.

I'm on crack. Are you, too?

+++ includes/form.inc	2009-12-29 15:13:16 +0000
@@ -879,15 +879,23 @@ function drupal_redirect_form($form_stat
+  if (_form_validate_skip($elements, $form_state)) {
+    return;
   }

How about if (!isset($form_id) && _form_validate_skip(...)) as a way of not skipping the top-level form, instead of checking for the top-level form inside the skip function?

I'm on crack. Are you, too?

Edit: this is here just to see whether it passes the bot. I will roll in the comments some time later. Or someone else can. I did my share.

Enrolled the changes effulgentsia pointed out. I really hope I get an RTBC finally.

I really am sick of this patch, poll and our testing.

We're 136 comments into this issue, everyone's burnt out and anxious to get this functionality in, we've been lucky enough to get as much involvement from chx as we've gotten, we've reached agreement on API and desired functionality, and #136 implements it successfully and elegantly.

So, while I could nitpick the code comments and other trivial details, I'd rather see this be RTBC, land, and then set back to needs work for remaining comment improvement that chx wouldn't need to be bothered with, unless he wants to be.

Given everyone who participated here, I'm not ready to set this to RTBC without at least one other person also approving it. But, as far as I'm concerned, it's commit-worthy.

@chx: You give up early. :P

Streamlined, fixed, and updated a couple of comments. No other changes.

#370537 by quicksketch, chx, sun, effulgentsia: Allow partial validation of forms.

Strange behavior with the patch:
- Edit a node having a multiple field with an 'add more' button
- Empty 'Title' field
- Press 'add more' (-> works as expected, new widget is displayed, no validation error)
- Save the node (-> works as expected, validation error, 'Title field is required')
- Fill the 'Title' back, save the node -> I get redirected to 'system/ajax', showing raw JSON data:

[{"command":"settings","settings":{"basePath":"\/drupal_test_7\/","admin_menu":{"toolbar":[],"destination":"destination=system\/ajax","hash":"292fc027fbf9103b4aa1b3b52277e2f6","basePath":"\/drupal_test_7\/admin_menu","replacements":{".admin-menu-users a":"0 \/ 1"},"margin_top":1},"ajax":{"edit-field-image-und-0-upload-button":{"url":"\/drupal_test_7\/file\/ajax\/field_image\/und\/0\/form-fab64989d4a29e3b060464eb1dc8e789","event":"mousedown","keypress":true,"wrapper":"edit-field-image-und-0-ajax-wrapper","selector":"#edit-field-image-und-0-upload-button","effect":"fade","speed":"fade","method":"replace","progress":{"type":"throbber","message":null},"button":{"field_image_und_0_upload_button":"Upload"},"formPath":"field_image\/und\/0\/upload_button"},"edit-field-image-und-0-remove-button":{"url":"\/drupal_test_7\/file\/ajax\/field_image\/und\/0\/form-fab64989d4a29e3b060464eb1dc8e789","event":"mousedown","keypress":true,"wrapper":"edit-field-image-und-0-ajax-wrapper","selector":"#edit-field-image-und-0-remove-button","effect":"none","speed":"none","method":"replace","progress":{"type":"throbber","message":null},"button":{"field_image_und_0_remove_button":"Remove"},"formPath":"field_image\/und\/0\/remove_button"},"edit-field-multiple-und-add-more":{"url":"\/drupal_test_7\/system\/ajax","event":"mousedown","keypress":true,"wrapper":"field-multiple-wrapper","selector":"#edit-field-multiple-und-add-more","effect":"fade","speed":"fade","method":"replace","progress":{"type":"throbber"},"button":{"field_multiple_add_more":"Add another item"},"formPath":"field_multiple\/und\/add_more"}}}}]

Also note: to my knowledge there's no way to test the above sequence in simpletest - doing a regular drupalPost() after a drupalPostAJAX().

This also causes fatal errors when the form involves taxo autocomplete widget, since it's taxonomy_autocomplete_validate() that does the job of turning a "comma separate terms" string into an array of regular field values. When the function is skipped, fatal errors happen down the road in field_default_extract_form_values() and field_default_submit().

Just saying. This should probably be fixed in a separate patch, by doing this job in a 'value' callback, which sounds more appropriate anyway.
Same pattern in options.module - options_field_widget_validate().

Bottomline is: form_set_value() in validate steps can no longer be trusted.

I had this feeling when fixing up poll. This is terrible. But, this is what I am afraid of now.

Edit: we need to change the form workflow or at least change every validate that also changes something in form/form_state into a #process which checks for submitted and works accordingly. This is not 7.x level. We can't go piecemeal, skipping only #required checks in validate but calling user validate routines because those will throw errors...

Just brainstorming here... What if we allow all the validate handlers to run, and then at the end of drupal_validate_form(), we clear the errors (kind of like form_clear_error() but more selective) for names outside of the #validate_values sections?

That idea sounds spooky at first sight, but in the end, it is the same result.

The only (resolvable) problem is that we currently don't have access to $form_state in http://api.drupal.org/api/function/form_set_error/7 and http://api.drupal.org/api/function/form_error/7, and thus, no access to 'clicked_button' -- however, we could certainly add it as argument, which would be a major improvement on its own.

Perhaps there's even a way without changing those functions via http://api.drupal.org/api/function/form_get_errors/7.

Poor kittens. And, baby jesus just was born five days ago and crying so bad. But, what could I do?

Poll tests will fail because the form level validate handlers still do not fire. On it.

I am posting this version here which will fail testing -- I checked, poll_validate does fire on pressing more choices but we still have the error on preview that yched described. I posted because I might fall asleep debugging this and someone else might want to work on it meanwhile :)

Funny, I was just finishing up my attempt when chx posted his. I'll post mine anyway, and then we can compare and contrast.

:)

I don't know if it is related, but I've seen the error @yched describes in my own AHAH coding, please ignore if way off track, as this comes from my Drupal 6 experiences. I am not as familiar with Drupal 7.

The cause is that $form['#action'] gets cached wrong. This issue will only manifest itself when an AHAH callback is made, and then on form submit, a validation error occurs resulting in the form being re-rendered, without a rebuild.

The reason:
Form is built form first time and cached, #action is node/add, as expected.
AHAH callback is made, and form is rebuilt and re-cached. Action is now path of AHAH callback, in this case system/ajax. (NOTE: At this point, the form in cache is now out of sync with the HTML on the users end, as the user still has node/add as the form action, yet the cached form has system/ajax.
In the event a validation error now occurs on form submit, the form is re-rendered, (not rebuilt), and because it is re-rendered from the cache, the form action is now system/ajax.

Again, I'm not 100% sure if this is relevant to the issue described, but it might help in fixing it.

Hey my patch passed. Then something is wrong locally but it passed!

And mine is a tad bit simpler...

D'oh because I posted the wrong patch. So the patch to consider is #150 and this once I figure out how to make this pass :)

Hey! I got poll working with the new code after doing some cleanup work. It's _incredible_ how poll only worked by accident -- the form level submit handler was doing crucial work for the More choice submit. And yet it did the very same in validate. What a mess!!

Here's the fixed version of #150.

@chx: I'll pop-in on IRC, so we can discuss our two approaches.

@yched: #140 is a bug in HEAD as well: #671184: AJAX form can submit inappropriately to system/ajax after failed validation.

effulgentsia won. The problem with mine is that a form level validate handler -- like the node validate and the hooks therein .. -- will set errors on elements. Some of these might be against values we want to have errors some of these can be outside. My patch only works in a cleaner world... we have our work cut for Drupal 8 for sure.

Do we really need to run the second part of _form_set_error_suppress() even on '1' and '-1' modes ?
I might get it wrong but it seems like _form_set_error_suppress() should be two functions:
- one for modes 1 / -1 handling the $form_states stack workaround, called by drupal_validate_form(),
- one for mode 0 doing the actual error filtering, called by form_set_error().

Yep. Setting to "needs review" since this involves a code change, and in IRC, sun indicated he wants a chance to post feedback.

We can do a lot better and simpler. Really. But now it's not validate_values but validate_show_errors. (Or maybe just show_errors?) Anyways, I moved the functionality into form_set_error itself it became WAY simpler. Might not cover everything as effulgentsia's patch calling drupal_form_submit with a button pressed from a validate handler will likely break it but -- that's edge case enough to not care.

I have restored the security hardening and renamed to #show_errors and renamed the issue too.

+++ includes/form.inc	2009-12-31 11:54:10 +0000
@@ -1005,18 +1005,77 @@ function form_execute_handlers($type, &$
+ * @param $show_errors

unless I'm mistaken, PHPdoc doesn't support line/paragraph breaks nor @code snippets within a @param section.

+++ includes/form.inc	2009-12-31 11:54:10 +0000
@@ -1005,18 +1005,77 @@ function form_execute_handlers($type, &$
+      // which case all errors re suppressed. For example, a "Previous" button

Typo: *a*re suppressed.

+ newly added comments seem to wrap under 80 chars in several places

+++ includes/form.inc	2009-12-31 11:54:10 +0000
@@ -1005,18 +1005,77 @@ function form_execute_handlers($type, &$
+        if (array_slice(explode('][', $name), 0, count($section)) === $section) {
+          $record = TRUE;
+        }

Seems like we could break; out of the loop once we found a match ?

+++ includes/form.inc	2009-12-31 11:54:10 +0000
@@ -1312,6 +1371,12 @@ function _form_builder_handle_input_elem
+      // Prevent insecure execution of form-level submit handlers by not
+      // supporting error suppression unless the button also defines its own
+      // submit handlers.
+      if (isset($element['#show_errors']) && isset($element['#submit'])) {
+        form_set_error(NULL, '', $element['#show_errors']);
+      }

Nitpick: comment is a little off. The code does not exactly prevent a feature if (!A), it enables it if (A).
Maybe something like
"Only support error suppression if the button also defines its own submit handlers, to prevent insecure etc..." ?

This review is powered by Dreditor.

Hey, chx, what do you think of this? Very small change to #164: just a difference of from where form_set_error(NULL, '', $show_errors) gets called. Also using drupal_static_reset() to turn off suppression, which handles all the edge cases I can think of. The only edge case remaining is if a #element_validate handler processes another form and afterwards calls form_error() for an element that should be suppressed on the form it's validating. But this is rare, it's not a security risk (suppression is off when it should be on, which isn't so bad), and the validate handler can easily remedy it by calling form_set_error(NULL, '', $form_state['clicked_button']['#show_errors']).

Also, the turning on/off can be moved from _form_validate() to drupal_validate_form() if you think that's better.

This patch does not implement feedback from #165. I just wanted to get this idea in front of folks.

Very good idea! I have moved it after the maxlength and options checker making those errors impossible to suppress. #required and custom validate handlers are suppressed. Makes it a bit more secure. Enrolled yched's comment changes.

Very nice.

Just fixed PHPDoc formatting and fixed a few comments.

Woohoo! I'm good with this landing, so leaving it as RTBC, but just curious if we really want to stick with #show_errors as the property name. Is it sufficiently descriptive? More descriptive would be #suppress_validation_errors_outside, but that's maybe too long. Some other possibilites: #require_valid_values or #require_valid_input, which don't convey the mechanism by which this is required. Thoughts? Reason I bring this up now rather than as a follow-up cleanup, is that once this lands, changing that name would be an API change. Please suggest better names than these options if you have them.

Yeah, I agree that #show_errors is not a good name. That makes it sound like it's a Boolean property, but it appears this is still taking an array of sections to validate when the button is pressed.

Is there a reason we can't go back to #validate_form_sections or similar?

To avoid bikeshedding via issue queue, which is slow and tedious, could you folks just get on IRC when you get a chance and we can go through some names in real time.

And the winner is......

#limit_validation_errors

This is good because it:

1. Implies that it's doing something incomplete, so that people don't see this property and think they want to use it in all cases ("Oh! #show_errors. I want to show errors!")

2. Relates it to the validation of the form, rather than just the error reporting, yet...

3. Doesn't make an implication that the rest of the validation routines won't be run, only that this section only will be reported.

Effulgentsia is working on a re-roll.

Sorry for the re-roll taking so long. I'm slow at reviewing and polishing comments.

In addition to changing the property name and polishing comments related to it, I also updated the PHPDoc for poll and field "add more" functions, because the old docs were simply wrong.

Straight to RTBC, because the code changes were agreed to in IRC, and webchick is in as good a position as anyone to review the PHPDoc.

The documentation totally exceeds my expectations; some of the tweakiest part of Form API are actually explained quite well now.

Awesome, awesome team work folks!! This was core development at its finest. :) Love the extra test coverage as well.

Happily committed to HEAD! Now we need some follow-up documentation for this API change.

I created a new issue for the documentation needs: #675206: Document new FAPI property, #limit_validation_errors. This way, patches to change the FAPI doc files in CVS can be submitted on that issue.

effulgentsia: Anyone with a contrib CVS account can patch the FAPI documentation. Please just leave this issue as Needs Work / Needs Doc rather than filing a new issue. Thanks! It helps us group together the Needs Doc issues, which are grouped as critical. I'm also changing this to a bug report now because it's a bug that it doesn't have the correct doc (FAPI and update guide).

Just reviewing this in the light of #173/174, about documenting this new feature (and others). We're going to have to get a grip on FAPI docs and maintaining them.

#100680: [meta] Make API module generate Form API documentation discusses this basic issue and suggests that we need to get this all into the code, in the same way we manage the hook documentation.

I'm proposing that we have a BoF at Drupalcon SF to brainstorm how to do this.

I updated the forms api reference with #limit_validation_errors, mostly as a pointer to the discussion in http://api.drupal.org/api/function/form_set_error/7. Please take a look after the rebuild of http://api.drupal.org/api/drupal/developer--topics--forms_api_reference.... and let me know if you'd like anything improved about it.

Is there a version of this for Drupal 5? I've modified the poll module from 5 to allow dynamic creation of new fields but there is a required field that should be filled out just to add new fields, only before submission. This would really be helpful.

@mjlF95: sorry, not for D5 or D6. Too big a change for already released versions.

Well, that's too bad. I got it working anyway. I set a flag (in the form of a hidden form item) to one state at the beginning of the form function to "invalid". Then I use my own node_form_add_preview function that sets this flag to "valid" before running drupal_validate_form. In the validation function I have a conditional that sets the error message only if this flag = "invalid". Therefore it only runs on submit, not on preview. This does not work for items that have "#required" set but it works for stuff specified in the validate function and that's all I need.

Interesting follow-up: #763376: Not validated form values appear in $form_state