Allow Partial Validation of Forms (aka, make "more" buttons work properly)
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | forms system |
| Category: | task |
| Priority: | critical |
| Assigned: | sun |
| Status: | reviewed & tested by the community |
| Issue tags: | ahah, API change, D7 API clean-up, D7 Form API challenge, formapi, forms, ImageInCore, JavaScript, Usability |
This patch is a continuation of #360128: Security fix for simplified AHAH callbacks and #331708: poll_choice_js uses FAPI2 way of thought. Basically since we've switch Poll module to use the "better" method of FAPI processing, we've incurred a major usability problem that you need to first fill out all required fields before you can click the "more choices" button, since the entire form is validated.
This patch solves the problem of "unnecessary validation" by creating a new property for form and button elements, "#validate_portion". This allows you to specify an array of callbacks (similar to #validate or #submit) that will return portions of the form that need to have their core validation performed (such as #required, #element_validate, and the #options checking).
In this implementation, it is now possible to use the "more choices" button with or without JavaScript and Drupal will not scold you for not filling out choices or any required fields. This doesn't touch the #ahah implementation at all, but I think it critical to fixing this problem before continuing other conversions of #ahah to this new approach using #ahah['callback'] (see the first issue linked here for more information).
This patch also extends the current tests for FormAPI validation to validate 2 sections of a form while not validating a separate element in the form. Note that this callback only defines areas of the form that will be checked against Drupal's built-in validation for element level properties, form-level validation will still occur on the entire form.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal_form_partial_validation.patch | 11.4 KB | Idle | Failed: 9684 passes, 0 fails, 2 exceptions | View details | Re-test |
#1
Sorry I wasn't entirely clear, this patch specifically targets poll.module and the problems it experiences with the "more" button. We can distribute this to other #ahah enabled buttons as we convert them to the new FAPI approach.
#2
#3
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.
#4
The last submitted patch failed testing.
#5
Updating the SimpleTests to avoid notices.
This patch also renames the property from "#validate_portion" to "#validate_section". Here's the complete example from poll.module:
<?php
/**
* Implementation of hook_form().
*/
function poll_form(&$node, $form_state) {
...
$form['choice_wrapper']['poll_more'] = array(
'#type' => 'submit',
'#value' => t('More choices'),
'#description' => t("If the amount of boxes above isn't enough, click here to add more choices."),
'#weight' => 1,
'#submit' => array('poll_more_choices_submit'), // If no javascript action.
'#validate' => array(), // No validation needed for the more button.
'#validate_section' => array('poll_more_validate_section'),
'#ahah' => array(
'callback' => 'poll_choice_js',
'wrapper' => 'poll-choices',
'method' => 'replace',
'effect' => 'fade',
),
);
...
}
/**
* Validate section callback. Only validate the poll section of the form.
*/
function poll_more_validate_section($form, &$form_state) {
return $form['choice_wrapper']['choice'];
}
?>
#6
subscribing
#7
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.
#8
Some issues now dependent on this issue:
#370823: Allow Nodes to Be Previewed Without Required Fields
#370819: Upgrade Upload Module to FAPI3
#9
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.
#10
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)
#11
Things we discussed and agreed on
#12
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.
#13
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.
#14
Subscribing
#15
Alright, here's a new approach that makes #validate_section a single array of form keys, rather than as a separate callback. The code is a bit simpler this way, and certainly is much easier for developers to implement since they don't have to make a callback just to return an element.
I also updated the tests to check that both #validate_section and #submit properties are required on the button in order for the partial validation to have any effect. Basically this patch completes the 1-3 requirements of #13, though a review of the implementation would still be welcome.
#16
I found a bit of cruft left over from #5 that's no longer necessary. Removed from this version.
#17
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.
#18
Similar to settings #submit on the button prevents form-level submit handlers from firing, setting #validate on the button (even if it's empty) keeps form-level validate handlers from firing. In the case of poll, we have a form-level validator that requires 2 poll choices, meaning you couldn't click the "more" button unless you'd already filled out 2 choices.
The changes to _form_validate() are for efficiency. Previously we were passing in the form by value, then as we recursed through it, we made a copy of each sub section of the form and passed that in by value. Not to mention every single form piece contains a complete copy of $_POST, so we're copying that too. It would be a good thing to keep from copying large amounts of data when we could be passing it by reference to save memory.
We also get rid of a static variable by moving the form-level validate outside of the recursive function. We didn't need to put it in a recursive function, when it was only run it if $form_id is passed in (so basically it's only run once at the end of the first call). We can just move it out of the function and the form is processed in the exact same way.
This still seems ready for review to me. Are there other problems with it?
#19
Subscribing! Will review ASAP.
#20
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.
<?php- $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.#21
Nope, our #submit and #validate handlers override any form level handlers. They do not add up. Otherwise this patch would be a lot bigger to prevent such behavior. This means when we click this "more" button, the only submit handler that gets fired is the one assigned to it. This should reduce accidental execution of submit handlers by quite a bit. However, if no #validate callback is defined on this button, then the default form-level validators fire, which we don't want.
Ah, sure enough. There's a lot of misinformation and assumptions about pass by reference vs. pass by value I suppose. I'll change these back to value.
Finally,
Yes that's right. $form is now the complete form. There's no point in calling it $complete_form when the other elements are called $elements. The values passed to element level validators is unchanged. This patch should not introduce any API changes in any way, just an API addition.
#22
Alright, this version makes less changes. It keeps the
static $complete_formvariable and leaves the form-level validation inside _form_validate(). Also contains more docs. Still has the exact same functionality.#23
Ha, my upload patch slipped in there too. You can use #22 if you'd like 2 use-cases of this functionality. :)
Here's the patch with only FAPI and Poll module's changes.
#24
Grammar mistake in the Simpletest comments.
#25
Chx gave me a much, much better way of getting the section of the form to be validated. Rerolling.
#26
Revised approach with less changes to _form_validate, though the signature is still changed. Now we pass in the the entire form and set an $init Boolean value on the first call, rather than having a $form_id variable that indicates the top of the form.
This approach gets rid of the static $complete_form variable, since now we can just pass in the $form value directly into each recursive call.
#27
A version with less strenuous form checking and so, less code.
#28
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.
#29
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'].
#30
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?
#31
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
#32
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.
#33
The last submitted patch failed testing.
#34
subscribing
#35
the summary list of followups:
#370823: Allow Nodes to Be Previewed Without Required Fields
#216064: Delete button triggers form validation
#280465: #required (and #element_validate too?) not skippable
#370819: Upgrade Upload Module to FAPI3
#36
subscribe.
#37
Okay, here's an updated patch that takes extra security precautions per Heine's #29. Now if using partial form validation, the $form_state['values'] array is only populated with the validated portion of the form. After making this change, the dangerous node-building that Heine pointed out in #29 blows up spectacularly (as it should) because $form_state['values'] didn't contain any of the values it needed to build the $node object.
Unfortunately due to the way the node form works (the form actually gets a built $node variable rather than using $form_state), this approach has terrible side-effects on the non-JS behavior. If you disable JavaScript and click the "More choices" button, it wipes out all the values of the form except for the the poll choices. Doh! This is because that node_form_submit_build_node() must be called on every build of the form, otherwise the $node variable passed into hook_form() doesn't get populated.
So, this means that for the node form, non-JS partial validation will probably not be possible in a secure fashion. Because it is impossible to rebuild the pseudo-node variable without a complete $form_state['values'] variable. In the future, if we convert hook_form() to not take a $node object, this could be fixed.
However, the full node object is definitely not necessary when we're only rendering a portion of the form (such as the choices), so we can still render just the choices part of the form with the validated values and update it via JavaScript without any problem.
Note this patch moves the location of drupal_set_value() in form.inc, which could have strange side-effects for form elements that use the $form_state['values'] variable within #process callbacks. However this is never the case in core, and $form_state['values'] just gets replaces with the $element['#value'] later anyway. So this could break some modules, but only if they're using $form_state is a very strange way that could be written differently anyway.
#38
I forgot to mention another important change, in order to sometimes validate the whole form and sometimes only validate what's specified by the button, an additional property is required on the $form variable itself.
$form['#validate_section_allowed']. If this property is not set (or FALSE), then the entire form will be validated. This makes it possible for our JS callbacks to validate partially, and our non-JS callbacks to validate the entire form (as needed for the node_form_submit_build_node() function).#39
An unrelated issue not caused by this patch #445736: Poll does not respect weight in Preview or More button.
#40
The last submitted patch failed testing.
#41
Arg, thwarted by #440894: Poll.module whitespace & coding style cleanup committed this morning. Reroll.
#42
The last submitted patch failed testing.
#43
subscribing, will review in, ahem, 2 weeks
#44
This has now become a prerequisite to #391330: File Field for Core, so I anticipate picking it up again shortly.
#45
subscribing.
#46
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
#47
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.
#48
The last submitted patch failed testing.
#49
Reroll based on sun's #47 that should fix the majority of the notices we're getting (mostly from comment module). We need to maintain NULL values in $form_state['values'], so we had to use
array_key_exists('#value', $elements[$key])instead ofisset($elements['key']['#value'])Still doesn't pass a lot of tests but I'm letting test bot have another look.
#50
Removing dsm().
#51
The last submitted patch failed testing.
#52
This version of the patch includes only the Form API changes and doesn't actually update poll.module to use it. Just checking if we'll break anything else with this approach.
#53
The last submitted patch failed testing.
#54
Ugh, I realized that #367567: Use AJAX framework for "Add more" / load include file containing form / introduce $form_state['build_info'] "formalized" the practice of using node_form_submit_build_node() during the submit handler (just like poll module does). Meaning that it's another implementation that will somehow need to be rewritten to work with only part of $form_state().
This problem is extremely, extremely hard. The node form is just a complete mess, since all implementations of hook_form() require $node, not $form_state. And if you ever give them just part of the node or form state, the forms just wipe their values out and throw a bunch of notices.
I'm really positively stumped on how to fix this problem. I think at best we'll be able to make this work for AJAX callbacks but non-JavaScript forms will remain requiring all the required fields to be filled out before clicking any buttons.
#55
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: 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 :-/
#56
Well after coming to the conclusion that a partial $form_state variable will be almost impossible to get working, I'm considering this alternative approach. Now instead of stripping down $form_state, this patch is just less lenient with the validation that can be skipped. The #validate_section property is still used, but instead of skipping all validation, it only allows skipping of #element_validate and #required. Other properties like #maxlength and (most importantly) #options are still checked regardless of the setting.
This approach is much, much easier to implement, and requires almost no work to fix existing modules.
#57
It will do until we clean up node - form relationship in D8 if we do.
#58
tagging
#59
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.
#60
Oh. Excuse the size of the patch. I review a lot of patches. :P
#61
+++ 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.
#62
That sounds like a "needs work."
#63
Patch needs at least one test? Might be as easy as adding another file upload and making sure there is no validation error message.
#64
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.
#65
.
#66
Re-rolled against HEAD and added comments.
This looks ready to fly to me.
#67
Revamped the comments once again, 'cause I didn't like them.
#68
#69
Discussed this some more with chx in IRC.
#70
Writing this test felt PLAIN AWESOME.
#71
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.
#72
Added one more assertion to make this test even more bullet-proof.
#73
+++ 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.
#74
Sorry, wrong post.
#75
Thanks! Incorporated those suggestions.
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.
#76
err. Fixed a stale, err, missing comment in there ;)
Looks ready to fly for me now.
#77
+++ 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.
#78
yay, nitpicks! :)
#79
Thanks!
#80
Any chance to get this in?
#81
Though it does not appear in later comments this approach has my approval.