Static caching: cannot call drupal_validate_form on the same form more than once
bjaspan - May 21, 2008 - 04:52
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | forms system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
Description
Consider:
<?php
function drupal_validate_form($form_id, $form, &$form_state) {
static $validated_forms = array();
if (isset($validated_forms[$form_id])) {
return;
}
?>If you call drupal_validate_form() on the same form more than once per page request, validation is not performed on any but the first call. This violates the API spec of drupal_execute_form().
#1
I've been bitten by this, too, when trying to use drupal_execute over an array of values (http://drupal.org/node/258572#comment-886048), and Earl ran into it in Views 2 (http://drupal.org/cvs?commit=123549 and http://drupal.org/node/273570) and ended up writing his own form handler to work around it. This is something that really should be fixed both in 7.x and in 6.x.
Anything that ought to happen in validation will not happen after the first time if you do anything with programmatic form submissions. That seems pretty critical to me.
#2
Subscribing
The drupalorg_testing profile uses drupal_execute() extensively to generate projects, issues, and comments. Given that this profile is what dww, hunmonk, and I use for testing project* stuff, the fact that only the first issue/comment/etc. created problematically is actually validated certainly decreases the utility of the profile, since we're less likely to catch nodes/comments that should never have been created in the first place.
#3
The obvious fix is to remove the static variable and the check for whether the form_id has already been validated. The processing then falls through to _form_validate() which has it's own check for $elements['#validated'], which seems like it would keep a form from being validated more than once.
There is no documentation about why someone thought it was necessary to prevent the form from even getting to that function if the form had already been processed. Does anyone know what the reason for that is?
#4
I did a little research on the history of that bit of code -- it's been in place since Drupal 4.7, before we had drupal_execute(). If we need that test at all (still not sure we do) one option is to store the $form['#build_id'] instead of $form_id, something introduced in Drupal 5 when we drupal_execute() and multi-step forms were introduced. The $form['#build_id'] should actually be a unique identifier for the form instance, which is what we really want here.
#5
dupe of #180063: No way to flush form errors during iterative programatic form submission
#6
I don't think this is a duplicate. If this bug isn't fixed, you can not even get to the place where that bug is. I think both bugs must be fixed.
#7
Sorry - not a dupe. Thanks KarenS. I asked chx and eaton to take a look here.
#8
looks a bug to me . I hope Eaton will peek in because I have no idea why we needed such a static... if at all.
#9
I'm also running into this problem (programmatically creating batches of nodes). I assume there is a reason for caching the validated state of the form - not obvious to me, though.
#10
Many, many moons ago we were concerned about the possible processing overhead of validating a form multiple times. In addition, we weren't quite sure if validation might be triggered multiple times erroneously. So a static existed to make sure that we didn't double-validate a given form.
Today, I think it's safe to say that we have a better grasp on the processing lifecycle and the risk of accidental double-validation is no longer a serious concern. If it IS, we should at the very least be storing the build_id rather than the form_id in that static variable. That would ensure that generating multiple copies of the same form in a single page load would not run into problems.
I don't think that we'll see any problems eliminating it, however.
#11
And when i say "Many moons ago," I mean "during the beta of Drupal 4.7, when FormAPI had first been added and we had precious little real world testing for the API." It's definitely not a big concern now.
#12
Using the $form[#build_id] might not work. I was doing some testing right now and using the following pseudo code doesn't populate the #build_id in the form.
$node = array('type' => <content type>);$form_values = array();
... populate $form_values
drupal_execute(<form id>, $form_values, $node);
Within drupal_execute there is a call to drupal_retrieve_form() but the returned form array doesn't include a #build_id as that is built in the drupal_get_form(). Could this be something that needs looking at? Having the build_id generated / assigned in the retrieve and not the get? I don't have enough understanding of the #build_id to know what parts are required when.
#13
Patch to 5.x, 6.x, and 7.x to remove the static variable and the check. If there is feedback about the #build_id in the retrieve_form instead of get_form() then I will update to have #build_id as part of the static variable check.
#14
Minor change to the patch for 5.x. It looks like at some points there are no $form_values initialized so an error would occur as the check for the token would fail. Added a check to see if $form_values are initialized before checking the token.
Can't test on Drupal 6.x or 7.x at the moment as I don't have a test environment for them. I also noticed that 6.x and 7.x have a new $form_state so I don't know if the same check would be needed.
#15
I tried the 6.x patch, but I could not have successive calls of
drupal_executereporting their "own" validation errors, so here it is my solution: I used a new#current_build_idform field as I was not sure that using the existing#build_idcould cause problems if populated in thedrupal_executecontext.#17
Bumping this to get more attention now that #180063: No way to flush form errors during iterative programatic form submission is in, which is a similar problem. I know I should do something more meaningful than bumping this, like testing it, but don't have time right now. I just don't want to see it drop off the radar screen because it is involved in several CCK problems, like #128038: Critical failures using drupal_execute() on nodes with CCK fields, and the problems I noted in #1, so I'm hoping others will have time to move this along.
#18
FYI and in case anyone searches for a similar issues to mine. I'm using 6.x.
This also causes problems importing CCK nodereference fields using the select widget. I've used this (http://drupal.org/cvs?commit=123549) workaround for the moment and I guess I could check for $form['#programmed'].
CCK nodereference fields are rendered as:
$form_state['values']['field_name']['nid']['nid']
but converted to
$form_state['values']['field_name'][0]['nid']
during validation.
Another workaround I used is to set both variables before calling drupal_execute. This way it doesn't matter if the validation runs.
Still inelegant but it doesn't need a patch to core...
#19
The last submitted patch failed testing.
#20
Marked the following issues as duplicate of this one.
#256573: drupal_validate_form() cannot be used twice on the same form
#148530: drupal_validate_form() problem with multiple drupal_execute()-s of the same form_id (this one was reported first, but it contains less information about the issue)
#21
Hi all,
We're also using drupal_execute to import nodes from the old non-drupal site to
Drupal 6, and found this issue. As a workaround, I have created a copy of drupal_execute named _drupal_execute, which then invokes _drupal_process_form, and that one invokes _drupal_validate_form where I have removed the static storage. Seems to be working ok within a loop where we're creating a bunch of nodes. I hope that helps someone.If this was to get fixed in core, a possible way would be to add an argument to drupal_validate_form that could be used to flush the static storage. Something like the following?
<?php
function drupal_validate_form($form_id, $form, &$form_state, $flush = FALSE) {
static $validated_forms = array();
if ($flush) {
$validated_forms = array();
return;
}
if (isset($validated_forms[$form_id])) {
return;
}
// If the session token was set by drupal_prepare_form(), ensure that it
// matches the current user's session.
if (isset($form['#token'])) {
if (!drupal_valid_token($form_state['values']['form_token'], $form['#token'])) {
// Setting this error will cause the form to fail validation.
form_set_error('form_token', t('Validation error, please try again. If this error persists, please contact the site administrator.'));
}
}
_form_validate($form, $form_state, $form_id);
$validated_forms[$form_id] = TRUE;
}
?>
So we could call the following within the loop where drupal_execute is involked.
<?php
$type = 'nodetype_xxx';
while ( ... ) {
// Build $form_state and $node ...
form_set_error(NULL, '', TRUE);
drupal_execute($type .'_node_form', $form_state, $node);
$result = form_set_error();
if (!empty($result)) {
var_dump($result);
}
// Flush static storage.
drupal_validate_form('', NULL, NULL, TRUE);
}
?>
#22
markus_petrux - Can you post your work around here? (_drupal_execute, _drupal_process_form, and _drupal_validate_form as you mentioned)
It's 3 AM on new year's day. I burned a day and finally realized validation is causing the problem. I understand what you said, but my brain is fried right now. I don't want to think. I don't want to patch core either. I just want to cut and paste a work around, so I can import my nodes.
Thx!
#23
@ubuntu #22: Just put the following code into a separate file that you include from your import scripts.
The main goal here was to use an altertative version of drupal_validate_form() that doesn't use static storage, which is the basic problem.
I do not recommend to just copy/paste the code. Please, use it as an example, but build your own from your current copy of Drupal 6, because a patch could change this in the future resulting in unexpected behaviours or worst. All these functions can be found in includes/form.inc.
Steps to reproduce this code:
1) Copy function drupal_execute() as _drupal_execute() and prefix the call to function drupal_process_form() with an underline character, so it can call your own copy.
2) Copy function drupal_process_form() as _drupal_process_form() and prefix the call to function drupal_validate_form() with an underline character, so it can call your own copy.
3) Copy function drupal_validate_form() as _drupal_validate_form() and remove the use of static storage. That's it.
<?php
/**
* Alternate version of drupal_execute.
*
* We need to call an alternative version of drupal_validate_form()
* because the original uses static data that prevents the same form_id
* to be validated more than once per execution unit.
*
* For additional information, please see:
* http ://drupal.org/node/260934
*/
function _drupal_execute($form_id, &$form_state) {
$args = func_get_args();
$form = call_user_func_array('drupal_retrieve_form', $args);
$form['#post'] = $form_state['values'];
drupal_prepare_form($form_id, $form, $form_state);
_drupal_process_form($form_id, $form, $form_state);
}
function _drupal_process_form($form_id, &$form, &$form_state) {
$form_state['values'] = array();
$form = form_builder($form_id, $form, $form_state);
// Only process the form if it is programmed or the form_id coming
// from the POST data is set and matches the current form_id.
if ((!empty($form['#programmed'])) || (!empty($form['#post']) && (isset($form['#post']['form_id']) && ($form['#post']['form_id'] == $form_id)))) {
_drupal_validate_form($form_id, $form, $form_state);
// form_clean_id() maintains a cache of element IDs it has seen,
// so it can prevent duplicates. We want to be sure we reset that
// cache when a form is processed, so scenerios that result in
// the form being built behind the scenes and again for the
// browser don't increment all the element IDs needlessly.
form_clean_id(NULL, TRUE);
if ((!empty($form_state['submitted'])) && !form_get_errors() && empty($form_state['rebuild'])) {
$form_state['redirect'] = NULL;
form_execute_handlers('submit', $form, $form_state);
// We'll clear out the cached copies of the form and its stored data
// here, as we've finished with them. The in-memory copies are still
// here, though.
if (variable_get('cache', CACHE_DISABLED) == CACHE_DISABLED && !empty($form_state['values']['form_build_id'])) {
cache_clear_all('form_'. $form_state['values']['form_build_id'], 'cache_form');
cache_clear_all('storage_'. $form_state['values']['form_build_id'], 'cache_form');
}
// If batches were set in the submit handlers, we process them now,
// possibly ending execution. We make sure we do not react to the batch
// that is already being processed (if a batch operation performs a
// drupal_execute).
if ($batch =& batch_get() && !isset($batch['current_set'])) {
// The batch uses its own copies of $form and $form_state for
// late execution of submit handers and post-batch redirection.
$batch['form'] = $form;
$batch['form_state'] = $form_state;
$batch['progressive'] = !$form['#programmed'];
batch_process();
// Execution continues only for programmatic forms.
// For 'regular' forms, we get redirected to the batch processing
// page. Form redirection will be handled in _batch_finished(),
// after the batch is processed.
}
// If no submit handlers have populated the $form_state['storage']
// bundle, and the $form_state['rebuild'] flag has not been set,
// we're finished and should redirect to a new destination page
// if one has been set (and a fresh, unpopulated copy of the form
// if one hasn't). If the form was called by drupal_execute(),
// however, we'll skip this and let the calling function examine
// the resulting $form_state bundle itself.
if (!$form['#programmed'] && empty($form_state['rebuild']) && empty($form_state['storage'])) {
drupal_redirect_form($form, $form_state['redirect']);
}
}
}
}
function _drupal_validate_form($form_id, $form, &$form_state) {
// If the session token was set by drupal_prepare_form(), ensure that it
// matches the current user's session.
if (isset($form['#token'])) {
if (!drupal_valid_token($form_state['values']['form_token'], $form['#token'])) {
// Setting this error will cause the form to fail validation.
form_set_error('form_token', t('Validation error, please try again. If this error persists, please contact the site administrator.'));
}
}
_form_validate($form, $form_state, $form_id);
}
?>
#24
thanks for sharing and thanks for the advice.
I will only use it as a one-time utility for my module installer. I hope the core fix will catch up soon.
Thanks for again!
#25
Does it also affect multistep forms? http://drupal.org/node/262422
Is there another work around (besides what is mentioned above) for multistep form? I tried to reset form['#validate'] for different steps, didn't seem to help.
#26
I think this is a rather significant issue. Isn't the whole point of using drupal_execute to programatically fill in a form, so that it does everything a normal form submit would do (so the full validation and any other alterations from modules). At the moment its only causing me a problems on cck date fields (other fields seem to work) - but I was originally assuming that each time I called drupal_execute it runs the validation functions, which I now know it doesn't for multiple forms.
I have made use of the solution in #23 for my module where I need to call drupal_execute more then once on the same form.
I haven't had a good look to understand the reasoning behind that static variable, but assuming it is needed the suggestion in #21 of flushing the static variable sounds reasonable to me, although I would suggest just adding this to the start of drupal_execute function rather then having to do it in your own module before/after each call of drupal_execute.
#27
The 7.x version of the latest patch.
#28
The last submitted patch failed testing.
#29
No error on my box, retesting
#30
Eaton in #10 indicated that simply removing the static cache should be sufficient and the very simple patch in #13 did that. The patch in #15 is significantly more complex but it is not clear why all that complexity was added or why the original patch wasn't enough.
If a simple patch (#13) works, we should go with the simple method. @platch, can you provide more information about why you expanded on the original patch?
#31
I also think we could just go ahead and remove the static here.
#32
The simple patch can also be back-ported to D6 and D5, to fix the bug there. I'd be worried about trying to backport anything more complicated.
#33
The last submitted patch failed testing.
#34
Since #322344: (notice) Form improvements from Views went in, the patch needed some restyling.
The patch in #13 did not take into account a side-issue: once able to validate a form multiple times we might have it submitted a first time with incorrect values and a second time with valid ones; since currently
form_set_errorstores errors per form id and not per form build id, once a form element is marked invalid it stays invalid, no matter what actual value it holds. This patch simply registers the form build id being processed and refactorsform_set_errorto store the errors per form build id. I added a simpletest to highlight this behavior.I think this one can be easily ported to D6 (it was born in D6) and probably D5.
#35
Just realized there is a far easier way: simply reset the form errors.
#36
The last submitted patch failed testing.
#37
re-rolled patch
#38
#39
For anyone on D6 wishing to get a functional patch:
1. Adhere to comment #23 http://drupal.org/node/260934#comment-1179304
2. As per comment #35 http://drupal.org/node/260934#comment-1359866 patch up _drupal_execute() so:
<?php
function _drupal_execute($form_id, &$form_state) {
$args = func_get_args();
$form = call_user_func_array('drupal_retrieve_form', $args);
$form['#post'] = $form_state['values'];
// additional patch
// Reset form validation.
// if form validation fails once it fails for all subsequent elements!
$form_state['must_validate'] = TRUE;
form_set_error(NULL, '', TRUE); // reset all form errors
drupal_prepare_form($form_id, $form, $form_state);
_drupal_process_form($form_id, $form, $form_state);
}
?>
#40
I just ran into this today - and tested the solutions in #23 and #39 which work as advertised (for people running into this in D6).
Since #180063: No way to flush form errors during iterative programatic form submission has been fixed and has been back-ported, should we not also back-port whatever solution is ultimately decided for D7 to D6 - considering that #180063: No way to flush form errors during iterative programatic form submission was a requirement for this fix - and no API change is introduced?
#41
The last submitted patch failed testing.
#42
#43
I got to experience this by using Ubercart with product kits. When I save a product node which is included in two different product kits, Ubercart uses drupal_execute to update the product kit nodes. The second product kit updated ends up getting a nodereference value that's 1 character long.
Here's a patch version of #39 for 6.x that's working for me.
#44
The last submitted patch failed testing.
#45
Reroll of #43 for 7.x. I noticed that 'must_validate' was only used in drupal_validate_form(), so I removed all of the other references to it in the code and comments (drupal_form_submit() and drupal_build_form()).
#46
@Jody Lynn: please, use a
.D6suffix in the patch name (es:core-form.inc-260934.D6.patch), otherwise the patch will be interpreted as a 7.x one and passed to the testbed for automatic testing, thus invalidating the status of the issue, which currently concerns D7.The previous patch stripped away the simpletests and introduced changes from #23 that are obsolete for D7, which has introduced a bypass functionality for the validation cache through
$form_state['must_validate']. This one needs to be documented so I reintroduced the related comment.People interested in the 7.x patch should review this one and read carefully what markus_petrux says in #23:
#47
The last submitted patch failed testing.
#48
fixed patch
Edit: now using
form_clear_error()to clear previous errors before processing the form.#49
The last submitted patch failed testing.
#50
HEAD broke.
#51
The last submitted patch failed testing.
#52
mmh, no
#53
The last submitted patch failed testing.
#54
rerolled
#55
Fixed a comment as per catch's suggestion.
#56
Subscribing. Please port this to 6.x. It's killing me.
#57
For 6.x:
Using build_id as the key to the static variable doesn't work, since the value isn't populated on drupal_execute. More importantly, ts not the form_id or build_id that makes a submitted form unique, it's the combination of its build_id and form_state. Checking for uniqueness on these terms sounds very messy. All of this is irrelevant, however. Because:
We validate the same form all the time when we click "Preview" multiple times on the same node.
Multiple calls to drupal_execute with the same form_state is analogous to "Preview"[ing] the same form multiple times, yet it doesn't work.
Conversely, multiple calls to drupal_execute with different form_state should result in validation analogous to "Preview"[ing] the same form with changed values, also doesn't work.
I vote to take the check out and validate as many times as requested. Short of a theoretical performance hit there is no reason not to. In fact, there is a theoretical security threat here. =P
I don't have Drupal properly checked out, so please forgive this mercurial diff on my repository:
@@ -561,12 +561,6 @@
* not be repeated in the submission step.
*/
function drupal_validate_form($form_id, $form, &$form_state) {
- static $validated_forms = array();
-
- if (isset($validated_forms[$form_id])) {
- return;
- }
-
// If the session token was set by drupal_prepare_form(), ensure that it
// matches the current user's session.
if (isset($form['#token'])) {
@@ -577,7 +571,6 @@
}
_form_validate($form, $form_state, $form_id);
- $validated_forms[$form_id] = TRUE;
}
/**
Also fixes, http://drupal.org/node/416126
#58
subscribing... +1 for push to d6 :)
#59
+++ includes/form.inc 5 Jul 2009 22:03:36 -0000@@ -380,6 +380,10 @@ function drupal_form_submit($form_id, &$
+ // Reset form validation.
+ $form_state['must_validate'] = TRUE;
+ form_clear_error();
So this is the solution? Two lines of code?
Wow.
+++ modules/simpletest/tests/form.test 5 Jul 2009 22:06:31 -0000@@ -346,6 +346,60 @@ class FormsFormCleanIdFunctionalTest ext
+class FormsProgrammaticSubmitFunctionalTest extends DrupalWebTestCase {
That is one awesomely long class name. Can we shorten it to something less crazy?
+++ modules/simpletest/tests/form.test 5 Jul 2009 22:06:31 -0000@@ -346,6 +346,60 @@ class FormsFormCleanIdFunctionalTest ext
+ $args = array('%value' => $value, '%errors' => $valid_form ? '' : implode(' ', $errors));
+ $this->assertTrue($result, t('Dummyfield value: %value<br/>Error: %errors', $args));
+ $this->assertTrue(!$valid_form || $form_state['dummy_submit'] == $value, t('Form executed'));
Can we add some whitespace here to make it more readable? And to adhere to the "80 characters wrap" rule?
Patch applied with some fuzz, so re-rolled... If someone can confirm the code is fixing what it supposed to fix, then RTBC'ed.
Don't drink and patch.
#60
The patch that didn't get attached on the last comment?
#61
Subscribe. Interested in a Drupal 6 fix.
#62
also did #23 and #39 in d6, seems to be working perfectly now. although it took me a while to get here because the error message i was getting was
warning: mysql_real_escape_string() expects parameter 1 to be string, array given in /includes/database.mysql.inc on line 321.#63
also confirm #23 and #39 in d6 works, subscribe
#64
Rerolled and fixed some string issue.
@joshmiller:
It's not that long, in the same file we have
FormsElementsTableSelectFunctionalTest.Override the changes in form.inc, launch the simpletest and you'll see what happens :)