Posted by bjaspan on May 21, 2008 at 4:52am
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | forms system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
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().
Comments
#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 :)
#65
#66
+++ 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.
#67
Implemented suggestions from #66.
#68
#69
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...
#70
#71
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.
#72
Fixed a couple of string issues.
#73
Subscribe. Interested in a fix.
#74
tested and it works :)
#75
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.
#76
Given that all the changes are two lines in form.inc and it's extremely throughly tested, yes, this is good to go.
#77
#72: form_multiple_validate-260934-72.patch queued for re-testing.
#78
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
#79
rerolled
#80
Green. Setting back to RTBC.
#81
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.
#82
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.).
#83
This isn't right either, since we get back to the error issues plach had originally which prompted this direction. Aso spoke to chx:
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.
#84
#85
@catch: can we update the tests?
#86
@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.
#87
subscribing
#88
Subscribing
#89
+++ 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.
#90
Subscribing
#91
subscribing
#92
Subscribing.
#93
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.
#94
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.
#95
@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.
#96
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.
#97
subscribe
#98
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
#99
subscribe
#100
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
#101
subscribe
#102
Subscribe
#103
subscribe
#104
Subscribing. In the meantime used hint from #23.
#105
Subscribing. Thanks
#106
Subscribing, for now i'm using #23. Seems to work.
#107
subscribe
#108
subscribing.
#109
subscribe
#110
Attached is a port of #79 to D6.
#111
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.
#112
subscribe. This effects authcache and comment_driven combination.
#113
subscribe
#114
Using Drupal 6 here, confirming that patch presented in comment #110 has resolved my issue.
#115
Patch in Comment #110 worked for me. Using D6 and Services to programmatically create multiple nodes of the same type using drupal_execute.
#116
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?
#117
It remains a bug in drupal. I have confidence in the patch #111.
#118
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.
#119
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.
#120
Didn't meen to RTBC. Needs a pass by the test bot at the least, perhaps additional tests too?
#121
Testbot doesn't run on D6, so need to do manual testing.
#122
Subscribing.
#123
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 :-)!
#124
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_statehas a 'submitted' key, I think 'validated' makes more sense.- uses
!empty()instead ofisset()because$form_state['validated']might be set but FALSE, we never know.#125
Below is the patch.
#126
Nice work. I like it, but will let another tester set status to RTBC.
#127
Bumping, hoping to get some reviews.
#128
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.