Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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().
Comment | File | Size | Author |
---|---|---|---|
#140 | drupal6.form-must-validate.140.patch | 864 bytes | sun |
#125 | drupal_validate_form-260934-125-D6.patch | 697 bytes | anrikun |
#111 | drupal_validate_form_simple_fix-d6.diff | 617 bytes | Dave Cohen |
#110 | drupal_validate_form-260934-110-D6.patch | 864 bytes | pdrake |
#84 | drupal_validate_form.patch | 959 bytes | catch |
Comments
Comment #1
KarenS CreditAttribution: KarenS commentedI'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.
Comment #2
aclight CreditAttribution: aclight commentedSubscribing
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.
Comment #3
KarenS CreditAttribution: KarenS commentedThe 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?
Comment #4
KarenS CreditAttribution: KarenS commentedI 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.
Comment #5
moshe weitzman CreditAttribution: moshe weitzman commenteddupe of #180063: No way to flush form errors during iterative programatic form submission
Comment #6
KarenS CreditAttribution: KarenS commentedI 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.
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedSorry - not a dupe. Thanks KarenS. I asked chx and eaton to take a look here.
Comment #8
chx CreditAttribution: chx commentedlooks a bug to me . I hope Eaton will peek in because I have no idea why we needed such a static... if at all.
Comment #9
alex_b CreditAttribution: alex_b commentedI'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.
Comment #10
eaton CreditAttribution: eaton commentedMany, 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.
Comment #11
eaton CreditAttribution: eaton commentedAnd 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.
Comment #12
ShawnClark CreditAttribution: ShawnClark commentedUsing 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.
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.
Comment #13
ShawnClark CreditAttribution: ShawnClark commentedPatch 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.
Comment #14
ShawnClark CreditAttribution: ShawnClark commentedMinor 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.
Comment #15
plachI 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 thedrupal_execute
context.Comment #17
KarenS CreditAttribution: KarenS commentedBumping 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.
Comment #18
davidredshaw CreditAttribution: davidredshaw commentedFYI 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...
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedThe last submitted patch failed testing.
Comment #20
markus_petrux CreditAttribution: markus_petrux commentedMarked 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)
Comment #21
markus_petrux CreditAttribution: markus_petrux commentedHi 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?
So we could call the following within the loop where drupal_execute is involked.
Comment #22
newbuntu CreditAttribution: newbuntu commentedmarkus_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!
Comment #23
markus_petrux CreditAttribution: markus_petrux commented@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.
Comment #24
newbuntu CreditAttribution: newbuntu commentedthanks 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!
Comment #25
newbuntu CreditAttribution: newbuntu commentedDoes 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.
Comment #26
univate CreditAttribution: univate commentedI 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.
Comment #27
plachThe 7.x version of the latest patch.
Comment #29
plachNo error on my box, retesting
Comment #30
KarenS CreditAttribution: KarenS commentedEaton 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?
Comment #31
catchI also think we could just go ahead and remove the static here.
Comment #32
KarenS CreditAttribution: KarenS commentedThe 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.
Comment #34
plachSince #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 refactorsform_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.
Comment #35
plachJust realized there is a far easier way: simply reset the form errors.
Comment #37
roychri CreditAttribution: roychri commentedre-rolled patch
Comment #38
plachComment #39
mugginsoft.net CreditAttribution: mugginsoft.net commentedFor 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:
Comment #40
andremolnar CreditAttribution: andremolnar commentedI 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?
Comment #42
webchickComment #43
Jody LynnI 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.
Comment #45
Island Usurper CreditAttribution: Island Usurper commentedReroll 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()).
Comment #46
plach@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:
Comment #48
plachfixed patch
Edit: now using
form_clear_error()
to clear previous errors before processing the form.Comment #50
webchickHEAD broke.
Comment #52
plachmmh, no
Comment #54
plachrerolled
Comment #55
plachFixed a comment as per catch's suggestion.
Comment #56
nick.dap CreditAttribution: nick.dap commentedSubscribing. Please port this to 6.x. It's killing me.
Comment #57
nick.dap CreditAttribution: nick.dap commentedFor 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:
Also fixes, http://drupal.org/node/416126
Comment #58
aaron.r.carlton CreditAttribution: aaron.r.carlton commentedsubscribing... +1 for push to d6 :)
Comment #59
joshmillerSo this is the solution? Two lines of code?
Wow.
That is one awesomely long class name. Can we shorten it to something less crazy?
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.
Comment #60
joshmillerThe patch that didn't get attached on the last comment?
Comment #61
threexk CreditAttribution: threexk commentedSubscribe. Interested in a Drupal 6 fix.
Comment #62
akeimou CreditAttribution: akeimou commentedalso 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.
Comment #63
brianfisher CreditAttribution: brianfisher commentedalso confirm #23 and #39 in d6 works, subscribe
Comment #64
plachRerolled 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 :)
Comment #65
plachComment #66
sunFormsProgrammaticTestCase
1) No t() in getInfo().
2) Rename 'name' to "Programmatic form submissions".
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.
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.
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.
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.
Comment #67
plachImplemented suggestions from #66.
Comment #68
sunComment #69
Dries CreditAttribution: Dries commentedOverall this looks good, and it is great to have extensive tests. However, it feels like this could be streamlined a bit more.
This construct is a little bit awkward IMO.
We don't really check if we get the expected value. $value was passed into submitForm(), and was not returned from the form itself.
Would be good to explain the purpose of this submit handler. It doesn't look like we use it...
Comment #70
sunComment #71
plachI 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.
Comment #72
plachFixed a couple of string issues.
Comment #73
robatsu CreditAttribution: robatsu commentedSubscribe. Interested in a fix.
Comment #74
litwol CreditAttribution: litwol commentedtested and it works :)
Comment #75
litwol CreditAttribution: litwol commentedBesides 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.
Comment #76
chx CreditAttribution: chx commentedGiven that all the changes are two lines in form.inc and it's extremely throughly tested, yes, this is good to go.
Comment #77
litwol CreditAttribution: litwol commented#72: form_multiple_validate-260934-72.patch queued for re-testing.
Comment #78
scor CreditAttribution: scor commentedsince 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
Comment #79
plachrerolled
Comment #80
plachGreen. Setting back to RTBC.
Comment #81
webchickLooks 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.
Comment #82
catchThis 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.).
Comment #83
catchThis 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.
Comment #84
catchComment #85
plach@catch: can we update the tests?
Comment #86
catch@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.
Comment #87
westbywest CreditAttribution: westbywest commentedsubscribing
Comment #88
tomgf CreditAttribution: tomgf commentedSubscribing
Comment #89
sunThe second condition should be kept.
Powered by Dreditor.
Comment #90
pvasener CreditAttribution: pvasener commentedSubscribing
Comment #91
effulgentsia CreditAttribution: effulgentsia commentedsubscribing
Comment #92
skibum3d CreditAttribution: skibum3d commentedSubscribing.
Comment #93
effulgentsia CreditAttribution: effulgentsia commentedRe #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.
Comment #94
effulgentsia CreditAttribution: effulgentsia commentedCNR 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.
Comment #95
catch@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.
Comment #96
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #97
japanitrat CreditAttribution: japanitrat commentedsubscribe
Comment #98
idmacdonald CreditAttribution: idmacdonald commentedThanks 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
Comment #99
Bodo Maass CreditAttribution: Bodo Maass commentedsubscribe
Comment #100
dunx CreditAttribution: dunx commentedsubscribe... 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
Comment #101
Renee S CreditAttribution: Renee S commentedsubscribe
Comment #102
chales CreditAttribution: chales commentedSubscribe
Comment #103
xxparanormalxx CreditAttribution: xxparanormalxx commentedsubscribe
Comment #104
kotnik CreditAttribution: kotnik commentedSubscribing. In the meantime used hint from #23.
Comment #105
gausarts CreditAttribution: gausarts commentedSubscribing. Thanks
Comment #106
zilverdistel CreditAttribution: zilverdistel commentedSubscribing, for now i'm using #23. Seems to work.
Comment #107
apanag CreditAttribution: apanag commentedsubscribe
Comment #108
jarune CreditAttribution: jarune commentedsubscribing.
Comment #109
Remon CreditAttribution: Remon commentedsubscribe
Comment #110
pdrake CreditAttribution: pdrake commentedAttached is a port of #79 to D6.
Comment #111
Dave Cohen CreditAttribution: Dave Cohen commentedShouldn't the static in drupal_validate_form be removed, and instead store info in the form_state?
Something like
This bug has wasted days of my life.
Comment #112
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedsubscribe. This effects authcache and comment_driven combination.
Comment #113
nzcodarnoc CreditAttribution: nzcodarnoc commentedsubscribe
Comment #114
nzcodarnoc CreditAttribution: nzcodarnoc commentedUsing Drupal 6 here, confirming that patch presented in comment #110 has resolved my issue.
Comment #115
incaic CreditAttribution: incaic commentedPatch in Comment #110 worked for me. Using D6 and Services to programmatically create multiple nodes of the same type using drupal_execute.
Comment #116
akeimou CreditAttribution: akeimou commentedpatches 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?
Comment #117
Dave Cohen CreditAttribution: Dave Cohen commentedIt remains a bug in drupal. I have confidence in the patch #111.
Comment #118
akeimou CreditAttribution: akeimou commentedokay, 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.
Comment #119
claar CreditAttribution: claar commentedPatch #111 fixes this for us on PHP 5.3. Why aren't tests being run on #111? Would be great to see this committed.
Comment #120
claar CreditAttribution: claar commentedDidn't meen to RTBC. Needs a pass by the test bot at the least, perhaps additional tests too?
Comment #121
webchickTestbot doesn't run on D6, so need to do manual testing.
Comment #122
anrikun CreditAttribution: anrikun commentedSubscribing.
Comment #123
anrikun CreditAttribution: anrikun commentedPatch #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 :-)!
Comment #124
anrikun CreditAttribution: anrikun commentedJust 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 ofisset()
because$form_state['validated']
might be set but FALSE, we never know.Comment #125
anrikun CreditAttribution: anrikun commentedBelow is the patch.
Comment #126
Dave Cohen CreditAttribution: Dave Cohen commentedNice work. I like it, but will let another tester set status to RTBC.
Comment #127
anrikun CreditAttribution: anrikun commentedBumping, hoping to get some reviews.
Comment #128
litwol CreditAttribution: litwol commentedUnless 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.
Comment #129
pdrake CreditAttribution: pdrake commented@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?
Comment #130
Dave Cohen CreditAttribution: Dave Cohen commentedI 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.
Comment #131
anrikun CreditAttribution: anrikun commentedI 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.
Comment #132
pdrake CreditAttribution: pdrake commentedWe 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.
Comment #133
Gábor Hojtsy$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.
Comment #134
pdrake CreditAttribution: pdrake commentedYes, 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.
Comment #135
Gábor HojtsyRight, 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?
Comment #136
pdrake CreditAttribution: pdrake commentedAt 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
Comment #137
Dave Cohen CreditAttribution: Dave Cohen commentedIn 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.)
Comment #138
Gábor HojtsyIt 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?
Comment #139
Dave Cohen CreditAttribution: Dave Cohen commentedI 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.
Comment #140
sun$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.
Comment #141
Dave Cohen CreditAttribution: Dave Cohen commentedIn 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.
Comment #142
sunThat'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:
Comment #143
pdrake CreditAttribution: pdrake commentedSeems 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.
Comment #144
Dave Cohen CreditAttribution: Dave Cohen commentedThis 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.
Comment #145
Gábor HojtsyI'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.
Comment #146
kotnik CreditAttribution: kotnik commented#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.
Comment #147
Gábor HojtsyThis is clearly being debated.
Comment #148
sun#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.
Comment #149
Dave Cohen CreditAttribution: Dave Cohen commentedIf this is a vote, I vote #140.
I posted #111, and since then convinced by sun that #140 is better.
Comment #150
pdrake CreditAttribution: pdrake commentedRegarding #110/#140 (exact same patch)
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?
Comment #151
anrikun CreditAttribution: anrikun commentedLet's RTBC #140 again then.
Comment #152
kotnik CreditAttribution: kotnik commentedNothing 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.
Comment #153
Gábor HojtsyOk, committed and pushed #140. Thanks all!