Consider:

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

CommentFileSizeAuthor
#140 drupal6.form-must-validate.140.patch864 bytessun
#125 drupal_validate_form-260934-125-D6.patch697 bytesanrikun
#111 drupal_validate_form_simple_fix-d6.diff617 bytesDave Cohen
#110 drupal_validate_form-260934-110-D6.patch864 bytespdrake
#84 drupal_validate_form.patch959 bytescatch
#83 drupal_validate_form.patch0 bytescatch
#82 drupal_validate_form.patch1 KBcatch
#79 form_multiple_validate-260934-79.patch4.88 KBplach
#71 form_multiple_validate-260934-71.patch4.96 KBplach
#72 form_multiple_validate-260934-72.patch4.96 KBplach
#67 form_multiple_validate-260934-67.patch4.1 KBplach
#64 form_multiple_validate-260934-64.patch4.18 KBplach
#60 form_multiple_validate-260934-55_2.patch4.64 KBjoshmiller
#55 form_multiple_validate-260934-55.patch4.64 KBplach
#54 form_multiple_validate-260934-54.patch4.68 KBplach
#48 form_multiple_validate-260934-48.patch4.51 KBplach
#46 form_multiple_validate-260934-46.patch4.11 KBplach
#45 core-form.inc-260934.patch1.65 KBIsland Usurper
#43 core-form.inc-260934.patch1.39 KBJody Lynn
#37 form_multiple_validate-260934-36.patch4.36 KBroychri
#35 form_multiple_validate-260934-35.patch4.18 KBplach
#34 form_multiple_validate-260934-34.patch5.14 KBplach
#27 form_multiple_validate-260934-27.patch2.04 KBplach
#15 260934-15-form.inc_.patch2.48 KBplach
#14 includes-form.inc-5.x-remove-validated-forms.patch1.1 KBShawnClark
#13 includes-form.inc-7.x-remove-validated-forms.patch843 bytesShawnClark
#13 includes-form.inc-6.x-remove-validated-forms.patch824 bytesShawnClark
#13 includes-form.inc-5.x-remove-validated-forms.patch781 bytesShawnClark
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

KarenS’s picture

Priority: Normal » Critical

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.

aclight’s picture

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.

KarenS’s picture

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?

KarenS’s picture

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.

moshe weitzman’s picture

KarenS’s picture

Status: Closed (duplicate) » Active

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.

moshe weitzman’s picture

Sorry - not a dupe. Thanks KarenS. I asked chx and eaton to take a look here.

chx’s picture

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.

alex_b’s picture

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.

eaton’s picture

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.

eaton’s picture

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.

ShawnClark’s picture

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.

ShawnClark’s picture

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.

ShawnClark’s picture

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.

plach’s picture

FileSize
2.48 KB

I tried the 6.x patch, but I could not have successive calls of drupal_execute reporting their "own" validation errors, so here it is my solution: I used a new #current_build_id form field as I was not sure that using the existing #build_id could cause problems if populated in the drupal_execute context.

KarenS’s picture

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.

davidredshaw’s picture

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

Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

markus_petrux’s picture

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)

markus_petrux’s picture

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?

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.

$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);
}
newbuntu’s picture

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!

markus_petrux’s picture

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

/**
 * 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);
}
newbuntu’s picture

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!

newbuntu’s picture

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.

univate’s picture

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.

plach’s picture

Status: Needs work » Needs review
FileSize
2.04 KB

The 7.x version of the latest patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review

No error on my box, retesting

KarenS’s picture

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?

catch’s picture

I also think we could just go ahead and remove the static here.

KarenS’s picture

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
5.14 KB

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_error stores 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 refactors form_set_error to 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.

plach’s picture

Just realized there is a far easier way: simply reset the form errors.

Status: Needs review » Needs work

The last submitted patch failed testing.

roychri’s picture

re-rolled patch

plach’s picture

Status: Needs work » Needs review
mugginsoft.net’s picture

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:

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);
}
andremolnar’s picture

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?

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Status: Needs work » Needs review
Jody Lynn’s picture

FileSize
1.39 KB

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

Island Usurper’s picture

Status: Needs work » Needs review
FileSize
1.65 KB

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

plach’s picture

@Jody Lynn: please, use a .D6 suffix 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:

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.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
4.51 KB

fixed patch

Edit: now using form_clear_error() to clear previous errors before processing the form.

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Status: Needs work » Needs review

HEAD broke.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review

mmh, no

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review
FileSize
4.68 KB

rerolled

plach’s picture

Fixed a comment as per catch's suggestion.

nick.dap’s picture

Subscribing. Please port this to 6.x. It's killing me.

nick.dap’s picture

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

aaron.r.carlton’s picture

subscribing... +1 for push to d6 :)

joshmiller’s picture

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

joshmiller’s picture

Status: Needs review » Needs work
FileSize
4.64 KB

The patch that didn't get attached on the last comment?

threexk’s picture

Subscribe. Interested in a Drupal 6 fix.

akeimou’s picture

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.

brianfisher’s picture

also confirm #23 and #39 in d6 works, subscribe

plach’s picture

Status: Needs work » Needs review
FileSize
4.18 KB

Rerolled and fixed some string issue.

@joshmiller:

That is one awesomely long class name. Can we shorten it to something less crazy?

It's not that long, in the same file we have FormsElementsTableSelectFunctionalTest.

If someone can confirm the code is fixing what it supposed to fix, then RTBC'ed.

Override the changes in form.inc, launch the simpletest and you'll see what happens :)

plach’s picture

Issue tags: +D7 Form API challenge
sun’s picture

Issue tags: -D7 Form API challenge
+++ modules/simpletest/tests/form.test	17 Nov 2009 00:00:15 -0000
@@ -354,6 +354,60 @@ class FormsElementsTableSelectFunctional
+class FormsProgrammaticSubmitFunctionalTest extends DrupalWebTestCase {

FormsProgrammaticTestCase

+++ modules/simpletest/tests/form.test	17 Nov 2009 00:00:15 -0000
@@ -354,6 +354,60 @@ class FormsElementsTableSelectFunctional
+      'name' => t('Programmatically submitted forms test'),
+      'description' => t('Test the programmatic form submit behavior.'),
+      'group' => t('Form API'),

1) No t() in getInfo().

2) Rename 'name' to "Programmatic form submissions".

+++ modules/simpletest/tests/form.test	17 Nov 2009 00:00:15 -0000
@@ -354,6 +354,60 @@ class FormsElementsTableSelectFunctional
+  /**
+   * Test multiple programmatic form submits.
+   */
+  function testMultipleSubmit() {

True, this executes multiple submits, but, this is testing validation of programmatic form submissions. Just testValidation() would be more appropriate.

The PHPDoc summary should be updated accordingly.

+++ modules/simpletest/tests/form.test	17 Nov 2009 00:00:15 -0000
@@ -354,6 +354,60 @@ class FormsElementsTableSelectFunctional
+   * Helper function used to programmatically submit the dummy form defined in 
+   * form_test.module with the given value.
+   *
+   * @param string $value
+   *   The dummyfield submitted value.
+   */
+  private function submitDummyForm($value = NULL) {
+    $form_state = array('values' => array('dummyfield' => $value));
+    drupal_form_submit('_form_test_dummy_form', $form_state);
+    $errors = form_get_errors();
+    $valid_form = empty($errors);
+    $result = empty($value) ? !$valid_form : $valid_form;
+    $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 correctly submitted'));
+  }

As mentioned below, please kill "dummy" everywhere in here.

submitForm() is sufficient.

Like someone else mentioned already, this function body needs more structure and inline comments.

+++ modules/simpletest/tests/form_test.module	16 Nov 2009 23:59:07 -0000
@@ -212,6 +212,34 @@ function _form_test_tableselect_js_selec
 /**
+ * Dummy form used to test programmatic form submits.
+ */
+function _form_test_dummy_form($form_state) {
+  return array(
+    'dummyfield' => array(
+      '#title' => 'Dummyfield',
+      '#type' => 'textfield'
+    )
+  );
+}
...
+function _form_test_dummy_form_validate($form, &$form_state) {
...
+function _form_test_dummy_form_submit($form, &$form_state) {

1) These shouldn't be private, and the function names should have a relation to the test they are for, i.e. s/dummy/programmatic/.

1a) The PHPDoc for the functions should also follow the common pattern:

Form builder to test programmatic form submissions.

Form validation handler for...

Form submit handler for...

2) Wrong function signature for form constructor.

3) The form construction code looks weird, please use the regular way to define and return a $form.

+++ modules/simpletest/tests/form_test.module	16 Nov 2009 23:59:07 -0000
@@ -212,6 +212,34 @@ function _form_test_tableselect_js_selec
+/**
+ * To test the validation handler the dummy value is explicitly required here.
+ */
+function _form_test_dummy_form_validate($form, &$form_state) {
+  if (empty($form_state['values']['dummyfield'])) {
+    form_set_error('dummyfield', t('This field is required although dummy.'));
+  }
+}

Why do we need this validation function? It's the same as #required, and, just using #required should be sufficient for the purpose of this test.

This review is powered by Dreditor.

plach’s picture

Implemented suggestions from #66.

sun’s picture

Priority: Critical » Normal
Status: Needs review » Reviewed & tested by the community
Dries’s picture

Overall this looks good, and it is great to have extensive tests. However, it feels like this could be streamlined a bit more.

+++ modules/simpletest/tests/form.test	28 Nov 2009 13:22:58 -0000
@@ -354,6 +354,64 @@ class FormsElementsTableSelectFunctional
+    $errors = form_get_errors();
+    $valid_form = empty($errors);
+    $result = empty($value) ? !$valid_form : $valid_form;

This construct is a little bit awkward IMO.

+++ modules/simpletest/tests/form.test	28 Nov 2009 13:22:58 -0000
@@ -354,6 +354,64 @@ class FormsElementsTableSelectFunctional
+    $this->assertTrue($result, t('Submitted field value: %value<br/>Error: %errors', $args));

We don't really check if we get the expected value. $value was passed into submitForm(), and was not returned from the form itself.

+++ modules/simpletest/tests/form_test.module	28 Nov 2009 13:22:32 -0000
@@ -212,6 +212,26 @@ function _form_test_tableselect_js_selec
+function form_test_programmatic_form_submit($form, &$form_state) {
+  $form_state['storage']['programmatic_form_submit'] = $form_state['values']['submitted_field'];
+}

Would be good to explain the purpose of this submit handler. It doesn't look like we use it...

sun’s picture

Status: Reviewed & tested by the community » Needs work
plach’s picture

I hadn't worked on this for a while, so I forgot that the test purpose was to test all the programmatic submission workflow.
The validation and submission handler do apparently useless tasks but get executed, so we can test this actually happens.

I changed the test name accordingly and introduced some more inline comments.

Suggestions from #69 should be incorporated.

plach’s picture

Status: Needs work » Needs review
FileSize
4.96 KB

Fixed a couple of string issues.

robatsu’s picture

Subscribe. Interested in a fix.

litwol’s picture

tested and it works :)

litwol’s picture

Status: Needs review » Reviewed & tested by the community

Besides running this in production i did read over the changes requested in #69 and it seems they were addressed. RTBC and hoping it will go in. thx.

chx’s picture

Given that all the changes are two lines in form.inc and it's extremely throughly tested, yes, this is good to go.

litwol’s picture

scor’s picture

Status: Reviewed & tested by the community » Needs work

since the testbot is too lazy these days to report back in the issue, setting this to 'needs work' as #72 does not apply: http://qa.drupal.org/pifr/test/20590

plach’s picture

Status: Needs work » Needs review
FileSize
4.88 KB

rerolled

plach’s picture

Status: Needs review » Reviewed & tested by the community

Green. Setting back to RTBC.

webchick’s picture

Version: 7.x-dev » 6.x-dev
Priority: Normal » Critical
Status: Reviewed & tested by the community » Patch (to be ported)

Looks like this fixes a pretty nasty bug, has the +1 from a form API maintainer, and comes with lots of nice tests. Lovely.

Committed to HEAD. Marking down for D6.

catch’s picture

Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs review
FileSize
1 KB

This was not the right fix. When using multiform to combine two entity forms (same form, different entities) on the same page, when both are submitted, drupal_validate_form() only runs on the first form. This just cost me hours of debugging since I knew this patch had been committed so it couldn't possibly be the same bug..

http://drupal.org/node/260934#comment-898761

http://drupal.org/node/260934#comment-910177

http://drupal.org/node/260934#comment-1355506

Patch which does that. Haven't run tests, fixed the bug for me. The issue was field API form validation, which resulted in fatal errors (cannot unset string offset in field.default.inc and etc.).

catch’s picture

FileSize
0 bytes

This isn't right either, since we get back to the error issues plach had originally which prompted this direction. Aso spoke to chx:

if you have two forms (multiform,not multiform, who cares?) on the same page with an element in both that share the same name then an error in one will trickle to the other.

Ie. form A gets validated, has error on 'foo', form B gets added to the page , end of request, now we render, form element 'foo' from A gets an error mark and then comes B and it's form element 'foo' also gets an error mark because form error is per name. End of story,alas.

So, new patch which leaves what we already have, and just removes the static caching in drupal_validate_form(). This isn't perfect, just less bad than what we've had since 4.7.

catch’s picture

FileSize
959 bytes
plach’s picture

@catch: can we update the tests?

catch’s picture

@plach: I'm not sure how to write a test for this yet, the steps to reproduce with real forms are a bit complex. I guess we could just put two identical forms on a page, and count the times that validate runs or something basic like that. Good to see that existing tests pass though at least.

westbywest’s picture

subscribing

tomgf’s picture

Subscribing

sun’s picture

Status: Needs review » Needs work
+++ includes/form.inc	11 Mar 2010 14:10:41 -0000
@@ -781,12 +781,6 @@ function drupal_prepare_form($form_id, &
 function drupal_validate_form($form_id, &$form, &$form_state) {
...
-  if (isset($validated_forms[$form_id]) && empty($form_state['must_validate'])) {
-    return;
-  }

The second condition should be kept.

Powered by Dreditor.

pvasener’s picture

Subscribing

effulgentsia’s picture

subscribing

skibum3d’s picture

Subscribing.

effulgentsia’s picture

Re #89: @sun: huh? Without the first part, the second part is meaningless.

+1 on #84. But, I don't know why the static is there to begin with, so I'm hoping whoever knows that history can speak up.

Please also see #766146: Support multiple forms with same $form_id on the same page and #799356: _form_set_class() is too aggressive in assigning the 'error' class for related issues not solved by #84 alone.

effulgentsia’s picture

Status: Needs work » Needs review

CNR pending sun clarifying #89. Re #86: having #766146: Support multiple forms with same $form_id on the same page land will facilitate writing tests for this.

catch’s picture

@effulgentsia, no-one knows why it's there, #82 has some comments along those lines, however chx did some archaeology and reckons it went in with the very first form API patch in 4.7. I think it's "paranoia caching" - validating a form might be expensive, so let's cache it just in case, I can't see another reason to have it there.

I looked at updating the test for this, originally it looked like it might be a simple addition to the existing test, but I think it needs more work than that, which I don't have capacity to do at the moment - that doesn't mean I think we should skip a test, but IMO that's the only thing holding it up, and if it ends up being me who writes it it'll be a couple of weeks at least.

edit: just re-read #94 - if we have some better test foundation in that other patch that'd be great.

effulgentsia’s picture

Title: Static caching: cannot call drupal_validate_form on the same form more than once » Static caching: when drupal_execute()ing multiple forms with same $form_id in a page request, only the first one is validated
Version: 7.x-dev » 6.x-dev
Status: Needs review » Patch (to be ported)

I merged #84 into #766146: Support multiple forms with same $form_id on the same page. I also merged #799356: _form_set_class() is too aggressive in assigning the 'error' class into that issue, which addresses the points in #83. I will work on completing the tests in that issue, but they all need to be done together, as they are fairly intertwined, given the multiple bugs that exist in processing a POST request on a page containing multiple forms sharing the same $form_id.

Given the other issue for dealing with browser-submitted forms, I'm reducing the scope of this issue to its original description, which #79 fixes adequately, but needs to be ported to D6.

japanitrat’s picture

subscribe

idmacdonald’s picture

Thanks for the solutions involving the local _drupal_execute function. That seems to have worked for me. However, it would be really great to be able to reliably use the core drupal_execute function in D6. Any progress here? It's amazing how complex a couple of lines of code can be!

-Ian

Bodo Maass’s picture

subscribe

dunx’s picture

subscribe... 16 months?

I have almost 10,000 nodes to import from a 14 year old site currently on a non-standard Postnuke. Been banging my head for a while until I Googled just the right words.

Using the _drupal_execute method just fine stuck at the top of my migration script.

Also, for anybody have issues creating node references...

http://drupal.org/node/726868#comment-3205150

Renee S’s picture

subscribe

chales’s picture

Subscribe

xxparanormalxx’s picture

subscribe

kotnik’s picture

Subscribing. In the meantime used hint from #23.

gausarts’s picture

Subscribing. Thanks

zilverdistel’s picture

Subscribing, for now i'm using #23. Seems to work.

apanag’s picture

subscribe

jarune’s picture

subscribing.

Remon’s picture

subscribe

pdrake’s picture

Status: Patch (to be ported) » Needs review
FileSize
864 bytes

Attached is a port of #79 to D6.

Dave Cohen’s picture

Shouldn't the static in drupal_validate_form be removed, and instead store info in the form_state?

Something like


Index: form.inc
===================================================================
--- form.inc	(revision 3708)
+++ form.inc	(working copy)
@@ -574,9 +574,7 @@
  *   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])) {
+  if (isset($form_state['drupal_validate_form_already'])) {
     return;
   }

@@ -590,7 +588,7 @@
   }

   _form_validate($form, $form_state, $form_id);
-  $validated_forms[$form_id] = TRUE;
+  $form_state['drupal_validate_form_already'] = TRUE;
 }

 /**

This bug has wasted days of my life.

SocialNicheGuru’s picture

subscribe. This effects authcache and comment_driven combination.

nzcodarnoc’s picture

subscribe

nzcodarnoc’s picture

Using Drupal 6 here, confirming that patch presented in comment #110 has resolved my issue.

incaic’s picture

Patch in Comment #110 worked for me. Using D6 and Services to programmatically create multiple nodes of the same type using drupal_execute.

akeimou’s picture

patches in #23 and #39 have worked for our production site since oct 2009.
site is currently on PHP 5.2.10, MySQL 5.5.11, Apache 2.2.14, Ubuntu 10.04.3, D6

again having the problem of only the first of multiple calls to drupal_execute going through if PHP is upgraded to 5.3.

tested this on a Windows box with PHP 5.3.8, MySQL 5.5.16, Apache 2.2.21, Windows XP S3. as soon as i remove the patch, and any calls to _drupal_execute changed back to drupal_execute, the problem goes away.

no complaints here. wondering what has changed in PHP that fixed this and how. or is it something in drupal, written specifically for the new PHP, that somehow made this a non-problem? or has the problem been addressed somewhere else in drupal so that this patch is now unnecessary?

Dave Cohen’s picture

It remains a bug in drupal. I have confidence in the patch #111.

akeimou’s picture

okay, switched to patch #111 and it works for both PHP 5.2 and 5.3.

what that means then is that patches #23 and its extension #39 may not work for PHP 5.3, at least it didn't for me.

claar’s picture

Status: Needs review » Reviewed & tested by the community

Patch #111 fixes this for us on PHP 5.3. Why aren't tests being run on #111? Would be great to see this committed.

claar’s picture

Status: Reviewed & tested by the community » Needs review

Didn't meen to RTBC. Needs a pass by the test bot at the least, perhaps additional tests too?

webchick’s picture

Testbot doesn't run on D6, so need to do manual testing.

anrikun’s picture

Subscribing.

anrikun’s picture

Status: Needs review » Reviewed & tested by the community

Patch #111 works for me.
Thanks Dave: this bug has wasted days of my life too.
A critical bug that has been reported more than 3 years ago :-)!

anrikun’s picture

Status: Reviewed & tested by the community » Needs review

Just rerolled Dave's patch (#111) into a "cleaner" one. Changes are:
- conforms to patch guidelines
- uses 'validated' instead of 'drupal_validate_form_already' as key name. Given $form_state has a 'submitted' key, I think 'validated' makes more sense.
- uses !empty() instead of isset() because $form_state['validated'] might be set but FALSE, we never know.

anrikun’s picture

Below is the patch.

Dave Cohen’s picture

Nice work. I like it, but will let another tester set status to RTBC.

anrikun’s picture

Bumping, hoping to get some reviews.

litwol’s picture

Unless you guys can show this to exist in d7/d8 i think this issue needs to be marked as wont fix. otherwise why not resurrect issues for d5? d4?

put this to rest to stop spamming 100+ people.

pdrake’s picture

@litwol, this did exist in D7 and the fix was committed (patch #79). The issue remains open so that the fix can be applied to D6 as well.

@ Dave Cohen, @anrikun, the patch in #110 is a backport of the fix committed in D7 - is there a reason you would prefer to change the approach for a D6 fix?

Dave Cohen’s picture

I would prefer #111 because it gets rid of a static variable $validated_forms which as far as I can tell serves no purpose. Also because it changes only one function (drupal_validate_form) as opposed to passing a flag between drupal_execute() and drupal_validate_form().

That said, it is not a strong preference. I haven't tested the other patch but I believe it works. And I recognize it may be quicker and easier to get a backport committed.

anrikun’s picture

I agree with Dave, and considering that $form_state has a 'submitted' key, adding a traceable 'validated' key to $form_state makes more sense than using a static variable separated from the form state.

pdrake’s picture

Status: Needs review » Reviewed & tested by the community

We have been running #110 in production on a large site for 6 months now with no negative effects. Both #110 and #111 have multiple people saying they have implemented them and they are working fine, so I am going to mark this RTBC to see if we can get one of these committed in D6.

Gábor Hojtsy’s picture

$form_state is a one of the "playgrounds" for people putting in all kinds of stuff, no? What if some module (d.o or custom) uses this key that we are introducing here already? Should we be more specific about this key? Looks like D7 just reused existing functionality, but we are introducing a new status here.

pdrake’s picture

Yes, it does introduce a new key in form_state. The key is in common use in the community for this purpose in D6 (ctools, views, node_import all respect or utilize this key to simulate D7's functionality in this regard). This patch is compatible with those modules, however, the key name could be changed to something more obscure to prevent collisions with custom code if desired.

Gábor Hojtsy’s picture

Right, so I'm at least concerned that maybe not all those modules are 100% compatible with how we use it here. Is there any advantage of using the same key that all the contribs do or introduce a different key name instead?

pdrake’s picture

At the moment, we use this patch in conjunction with views and ctools without negative effects. I have not run node_import in production, but suspect from the code that it would be compatible.

Advantages to retaining the same key name are: compatibility with D7 (for module backports / forward ports), elimination of duplicate code (if views and ctools remove their custom versions of this)

Advantages of a new key name are: prevent potential conflicts with existing code that is using this key in a different manner

Dave Cohen’s picture

In the first patch I proposed, I used a key that started with 'drupal_validate_form_', specifically because drupal_validate_form() was the only function that set or observed the key. The idea was to avoid conflicts. I don't know that such a convention is used anywhere else in Drupal's form API.

That said, I'm not particularly afraid that using 'validated' will cause any conflicts. And if there are conflicts, I'd blame the third-party module, not drupal core. (Unless that third party module happened to be named 'validated.module', and even then I'd blame the module, come to think of it.)

Gábor Hojtsy’s picture

And if there are conflicts, I'd blame the third-party module, not drupal core. (Unless that third party module happened to be named 'validated.module', and even then I'd blame the module, come to think of it.)

It is hard to blame a contrib module for a backwards incompatible change in core. If you update core but none of your contrib modules, and suddenly your website breaks, the culprit is clear, right?

Dave Cohen’s picture

I won't argue with that. I'm being a little snarky when I say I'd blame the module. I just mean that if you take a data structure created by core and throw 'validated' into it, you're asking for trouble. I would use 'mymodule_validated' or something along those lines.

sun’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
864 bytes

$form_state['must_validate'] has an entirely different logic than the proposed $form_state['validated'].

Form API never allowed to control whether a form may skip validation (which is what the proposed 'validated' condition effectively would be). The must_validate property is the opposite - it allows to enforce form validation, even if it ran on the form already.

AFAICS, the static cache exists to prevent form validation from running twice - because form/element validation handlers may use form_set_value() to adjust and override the submitted form values after successful validation. These manipulations may only be done once, as they replace the values in $form_state['values'].

Furthermore, various form validation handlers in earlier versions of Drupal performed things that shouldn't have been performed during form validation; e.g., actual triggering and execution of related/dependent actions, such as creation of relationships/referenced entities, etc. Proper separation of concerns in form validation and submission steps was not fully guaranteed.

That said, must_validate seems to have been purposively introduced for programmatic form submissions, since it is only ever set in drupal_form_submit(). Thus, I'd recommend to backport it identically, because this means it's known to work, identical to D7, and the scope of the change is limited to programmatic form submissions.

Dave Cohen’s picture

Form API never allowed to control whether a form may skip validation

In the case described in this issue, setting your $form_state['must_validate'] to FALSE will cause it to skip validation. I'm not particularly concerned about that. Just mentioning it as a caveat.

I haven't yet tested that patch. But I follow the logic of post #140 and it makes sense to me. Just looking at the patch it seems right, and if D7 works the same way that's a plus.

sun’s picture

setting your $form_state['must_validate'] to FALSE will cause it to skip validation

That's not true. If $form_state['must_validate'] is undefined, NULL, 0, or FALSE, validation is only skipped for $form_ids that have been seen before, it has no meaning and no effect at all. Identical behavior to now. In all other cases, the form goes through validation.

Matrix:

$form_id seen before? no yes no yes
$form_state['must_validate'] TRUE TRUE
will be validated? yes no yes yes
pdrake’s picture

Status: Needs review » Reviewed & tested by the community

Seems like we're going in circles here... I am marking this RTBC, again, because we have been running my original patch #260934-110: Static caching: when drupal_execute()ing multiple forms with same $form_id in a page request, only the first one is validated (identical to the recently posted #260934-140: Static caching: when drupal_execute()ing multiple forms with same $form_id in a page request, only the first one is validated) in production for 6 months and there have been multiple comments as to its successful use by others.

Dave Cohen’s picture

In the case described in this issue, setting your $form_state['must_validate'] to FALSE will cause it to skip validation.

This is what I said, and it is true for the edge case described in the original post. This issue is about using drupal_execute to submit more than one form with the same form_id, and the problems with the second and subsequent submissions. I still agree with your argument that #140 is better than previous patch, just pointing out that one minor thing. You express concern that a malicious coder could tweak the $form_state to prevent validation. I'm saying that malicious coder can still do that, if they bother to use drupal_execute and submit more than one form. A malicious coder can do pretty much whatever they want.

I agree #140 is RTBC.

Gábor Hojtsy’s picture

I've seen a couple people validated that #110 worked for them, but then the focus shifted to #111 (with feedback from people that that worked very well), which was a different approach (and then proved incorrect). Would be great to have more people verify #110/#140.

kotnik’s picture

#125 is the best solution, IMHO (it's improved #111). It beats #110/#140 since it shaves off one static variable, which is why people shifted from #110 to #111.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

This is clearly being debated.

sun’s picture

#140 is a 1:1 backport of the code in Drupal 7.

Obviously, that's the best we can do in terms of maintenance. Since the code is identical to D7, the functionality is also covered by tests (in D7 only).

As explained in #140, the approach taken in #125 is not acceptable, as it allows to entirely skip the form validation.

For D8+, we should look into changing this validation check into $form_build_id - or alternatively remove it entirely, although we'd have to double-check the possible pitfalls of form_set_value() calls in validation handlers, as explained in #140. Anyway, that's a separate issue.

Dave Cohen’s picture

If this is a vote, I vote #140.

I posted #111, and since then convinced by sun that #140 is better.

pdrake’s picture

Regarding #110/#140 (exact same patch)

  • RTBC by multiple folks (pdrake, nzcodarnoc, incaic)
  • direct backport of D7
  • does not allow skipping of form validation entirely (as #125 does)
  • originally backported in 8/2011, running smoothly in production since
  • same method was implemented by views and ctools as a workaround for this bug, plays nicely with these modules
  • preferred by the original author of the alternative solution (#111)

Two questions:
1. Even if someone puts forward another alternative fix, will we really commit a fix that diverges from the direction taken in D7?
2. Is there anything left to debate on this?

anrikun’s picture

Status: Needs review » Reviewed & tested by the community

Let's RTBC #140 again then.

kotnik’s picture

Nothing left to debate, after some testing last night, and careful re-reading of this thread, I agree with #140 approach. Sorry for the noise, lets merge #140.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Ok, committed and pushed #140. Thanks all!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.