AJAX form can submit inappropriately to system/ajax after failed validation

I don't know if it is related, but I've seen the error @yched describes in my own AHAH coding, please ignore if way off track, as this comes from my Drupal 6 experiences. I am not as familiar with Drupal 7.

The cause is that $form['#action'] gets cached wrong. This issue will only manifest itself when an AHAH callback is made, and then on form submit, a validation error occurs resulting in the form being re-rendered, without a rebuild.

The reason:
Form is built form first time and cached, #action is node/add, as expected.
AHAH callback is made, and form is rebuilt and re-cached. Action is now path of AHAH callback, in this case system/ajax. (NOTE: At this point, the form in cache is now out of sync with the HTML on the users end, as the user still has node/add as the form action, yet the cached form has system/ajax.
In the event a validation error now occurs on form submit, the form is re-rendered, (not rebuilt), and because it is re-rendered from the cache, the form action is now system/ajax.

Again, I'm not 100% sure if this is relevant to the issue described, but it might help in fixing it.

Wild assumption, not tested, so please test. ;)

Simpler replication:

1. Go to node add form
2. Click "Add another item" button
3. Click "Save" button
4. Optional: Enter title, hit "Save" button again. #action is 'system/ajax' here already.

Since we rebuild the form, the form is rebuilt. And since the form is rebuilt, system_element_info() assigns a new #action, defaulting to request_uri().

This patch fixes it for me.

Contains some other cleanups, which could technically be removed from this patch, but I left them in, because I tried to fix this at the very wrong location at the beginning...

Patch #5 does fix the issue of "taken to system/ajax URL with JSON output after step 4", but the node that gets saved is empty.
No title, no field value, no nothing :-).

well, so why don't we fix that? :P

I'll check this out a little later.

#8 works for me with manual tests.
But it seems the bot won't install D7 with it. Let's see if a re-test makes us lucky :-/

I didn't know that test slaves do their drupal install by submitting forms, but the patch does break installer forms.

When you reach the 'Database configuration' form, hitting submit takes you back to the 'select install language' page.

Looking into the HTML, the <form> "action" attribute is
- install.php?profile=standard&locale=en with patch
- install.php?profile=standard&locale=en in an unpatched HEAD.

+++ includes/form.inc	10 Jan 2010 03:44:19 -0000
@@ -2881,9 +2882,14 @@ function theme_textfield($variables) {
+  if ($element['#action']) {
+    $element['#attributes']['action'] = check_url($element['#action']);
+  }
+  $element['#attributes']['accept-charset'] = 'UTF-8';
+  $element['#attributes']['method'] = $element['#method'];
+  $element['#attributes']['id'] = $element['#id'];
   // Anonymous div to satisfy XHTML compliance.
-  $action = $element['#action'] ? 'action="' . check_url($element['#action']) . '" ' : '';
-  return '<form ' . $action . ' accept-charset="UTF-8" method="' . $element['#method'] . '" id="' . $element['#id'] . '"' . drupal_attributes($element['#attributes']) . ">\n<div>" . $element['#children'] . "\n</div></form>\n";
+  return '<form' . drupal_attributes($element['#attributes']) . ">\n<div>" . $element['#children'] . "\n</div></form>\n";

drupal_attributes() runs check_plain() :-). Should be OK for the other bits ('accept-charset', 'method', 'id'), but not for 'action'.

Let's try this one.
Note that the hunk dealing with build ids disappeared in patch #8, but I kept sun's patch name for now.

2 days to code freeze. Better review yourself.

hmm. Hold on, aren't literal ampersands supposed to be escaped with entities in XHTML attributes? If so, the patch in #8 is more correct than HEAD.

http://www.w3.org/TR/xhtml1/#C_12

@carlos8f: possibly, but if so that's a different fix in a different issue :-)

Oh, I assume you meant check_plain() was getting run twice on the URL. In that case, ok :P

This is RTBC, unless the bot proves us wrong. ;)

@carlos8f: er, that's not what I meant back then, but that's actually what was happening ;-). check_url() does run check_plain(), so patch #8 ran check_plain() twice, and current HEAD and patch #14 do encode their HTML entities (once) in 'action' attributes.

Anyway. This is good when the bot says so.

Yeah ignore what I said in #15, the install bug was caused by the URL being double-encoded by both check_url() and check_plain().

didn't mean the status change :-/

We need some tests for this, IMO. Sounds like a tweaky bug we are unlikely to catch again if we break it.

@webchick: The simpletest framework cannot test this :-(.
The bug involves #ajax form submission, and there's no working with a form past a $this->drupalPostAJAX(). This would require the internal browser to parse the JSON and manipulate the DOM the same way the ajax.js script does.

Back to RTBC.

Working on #484838: [rfay version] AJAX form can submit inappropriately to system/ajax after failed validation as well. I came to a completely different solution there. It seems wrong to me that system_element_info() sets the default to request_uri() in the first place.

#484838: [rfay version] AJAX form can submit inappropriately to system/ajax after failed validation had a better title.

I have as well tested this bad boy and it solves the issue. This issue also will need to be backported as it does in fact happen on D6.

In regards to a test, I do not believe you need to parse json and change the dom. I think you just need to hit the 'add more button' then submit the form without the title. Then 'rebuild' the form based on the form build id and the form cache.

Its not as simple as Im making it sound, but it is doable. We are not checking to make sure the add more button works but checking to make sure the next time we try to build this existing form that the form action is right.

edit: ok spent time on trying to write one. Still think its possible. just need two urls that pull up the same form_id. The form function sets $form_state['cache'] = TRUE and the second url takes the $form-build-id from as a parameter to rebuild the existing form. Then it will render the form which should have the same action (use xpath to find it).

I tested the patch in #14 against the test module posted in #484838: [rfay version] AJAX form can submit inappropriately to system/ajax after failed validation and confirm that it solves the problem described there.

I also tested against the original complaint (poll module malfunction) in #484838: [rfay version] AJAX form can submit inappropriately to system/ajax after failed validation and it resolves that problem as well.

Ok got a test to work.

So while this bug presents itself on AJAX forms, it really is a bug with the form cache. And by that I mean, when a form is put into the cache and another callback pulls that exact form_build_id out of the cache at a different url, the form action is overridden to the current url.

So my approach to the test was the following, create a bare bones form and set the form_state[cache] = true. Use xpath to find the form_build_id and pass that build id to another menu callback that will pull that form out of the cache and render it to the screen. Use xpath to ensure that the form just render to the screen from a different url has the same action.

This is mimics the essential flow of what ajax_get_form() does, but fetching the form_build_id from a url parameter instead of $_POST.

The old behavior doesn't cause a problem as AJAX would never render the entire form again, just the parts it needs so the $form['#action'] was effectively ignored. But when you try to really submit the form and it fails validation, the form is rebuilt from the cache (which was set by the AJAX callback at system/ajax) and now it renders the whole form with the bad '#action'.

And when you use form_state['storage'] the url is always the same, so the 'bug' went unnoticed there.

Hope that makes sense. This patch is from #14 with a new test.

Text update. Clearly I was tired last night.

'title' => 'Render the same form except the form action should be different'

should have been

'title' => 'Render the same form except the form action should be same'

Doh!

#29 failed the bot. http://qa.drupal.org/pifr/test/26666

in the log for form.test its syntax error was "No syntax errors detected in ./modules/simpletest/tests/form.test". So I tested in on my end as well with php -l -f

Strange. So Im just going to resubmit it and see what happens.

three failed tests

http://qa.drupal.org/pifr/test/26806

Can't help but feel like I hijacked this bad boy with a working Test passing patch and trying to hard to create a test around it....

#31: form_rebuild_form_build_id-671184-31.patch queued for re-testing.

I'm unable to recreate the failure that the testbot reports, so I'm going to resubmit this now that the bot is really back.

#31: form_rebuild_form_build_id-671184-31.patch queued for re-testing.

I was unable to recreate the test failures as well. Not sure whats going on.

It apparently fails the bot again (although it hasn't reported back yet), and it's the same failure.

It looks to me like this is either an environmental difference or a problem with the testbot. Does anybody have experience debugging this type of disconnect? I suppose that I can install a VM with PIFR and run the test. Any other suggestions? I hate to have an obscure problem like this derail this issue.

OK, boombatower helped me with this. The problem was that the test assumed that the url would be "/form-test/form-action", but on the bot, these things run in a subdirectory, "checkout". So the test was missing a url():

    $form = $this->xpath('//form[@action="' . url('form-test/form-action') .'"]');

We can hope that this one will pass :-)

nice! boombatower++ rfay++

Moving to RTBC per #23

+++ includes/form.inc
@@ -731,7 +731,11 @@ function drupal_prepare_form($form_id, &$form, &$form_state) {
+  if (isset($form_state['action'])) {
+    $form['#action'] = $form_state['action'];
+  }
   $form += element_info('form');
+  $form_state['action'] = $form['#action'];

Coming back to this issue after some time and looking at this snippet, my first thought was: WTF? ;)

Let's add an inline comment here to explain the rotation.

+++ modules/simpletest/tests/form.test
@@ -784,3 +784,41 @@ class FormsRebuildTestCase extends DrupalWebTestCase {
+class FormActionFunctionalTest extends DrupalWebTestCase {

FormCacheActionTest

+++ modules/simpletest/tests/form_test.module
@@ -889,3 +905,29 @@ function form_test_form_form_test_state_persist_alter(&$form, &$form_state) {
+ * @param $form_build_id
+ *  The build id of the form.
+ *
+ * @return string
+ *  HTML for the page.

Wrong indentation here.

+++ modules/simpletest/tests/form_test.module
@@ -889,3 +905,29 @@ function form_test_form_form_test_state_persist_alter(&$form, &$form_state) {
\ No newline at end of file

This should not happen.

Powered by Dreditor.

Pasqualle's patch does exactly the cosmetic fixes requested by Sun and adds no other complexity or concerns.

Moving it once again back to RTBC. This is the same functionality approved 4 times already.

subscribing

Seems like that might be one small error. Function form_test_action_persist doesn't actually return an array. Ironically, thats not a problem as the form still gets cached.

The fix makes it impossible for a form constructor to just change the #action during rebuild - that's bad. Also copying around the action that way is quite confusing and worse now the action is in $form and $form_state, so developers might be confused which one has to be changed/used.

Instead why not just prevent the #action from being overwritten when it shouldn't? When the rebuild in ajax_form_callback() sets the #action to request_uri() write back the old value and we are done. That way we don't have confusing multiple values and devs are able to change the #action easily.

Ok well lets see how the bot likes this. This approach I think addresses fago's concerns by only overriding $form['#action'] on ajax callbacks.

The problem that is happening is the cache_form table gets a form saved into it that has the wrong #action on ajax forms. So to address this problem, this patch introduces a ajax_rebuild_form() which is similar to drupal_rebuild_form() except it requires a $form_action string and a $form_build_id string.

This patch also changed drupal_rebuild_form() to remove all the AJAX stuff from it. Kindof a nice cleanup.

This needs a test though, so first just seeing how the bot likes this.

#49: rebuild_form_671184-49.patch queued for re-testing.

This patch introduces a test around ajax_rebuild_form(). After writing this test, not sure it is particularly useful. I couldn't come up with another way to test it without actually calling ajax_rebuild_form directly.

#52: rebuild_form_671184-52.patch queued for re-testing.

+1 having a separate ajax_rebuild_form(), makes completely sense.
However having $form_action as a separate argument looks a bit unlogical to me - I think it would be cleaner if we just pass the previous form to the function. Also the build id can be set to the usual $form['#build_id'] by ajax_get_form() and the form_id is already there - that way we could just pass the previous form and $form_state ?

Apart from that:
* The comment of ajax_rebuild_form () talks about AHAH - we should consistently use AJAX.
* What is a "csid" ? I guess that should be "cid". Anyway "cache id" would be probably easier to read ;)
* The comment should note what's difference to drupal_rebuild_form().

+  // Contexts may call drupal_rebuild_form() even when $form_state['rebuild']
+  // isn't set, but _form_builder_handle_input_element() needs to distinguish a 
+  // rebuild from an initial build in order to process user input correctly.
+  // Form constructors and form processing functions may also need to handle a
+  // rebuild differently than an initial build.
   $form_state['rebuild'] = TRUE;

There shouldn't be any other contexts left - but anyway we should keep that just to be sure. Thus the comment should say so.

@test:
There are already some AJAX related tests, see AJAXFormValuesTestCase. I think doing it analogously, thus invoking a drupalPostAJAX() and then checking the #action of the cached form should do it.

* What is a "csid" ? I guess that should be "cid". Anyway "cache id" would be probably easier to read ;)

Ya csid is carried over from the rebuild form. I will update both spots.

However having $form_action as a separate argument looks a bit unlogical to me - I think it would be cleaner if we just pass the previous form to the function. Also the build id can be set to the usual $form['#build_id'] by ajax_get_form() and the form_id is already there - that way we could just pass the previous form and $form_state ?

I not sure I agree with this. By what metric is it cleaner? Passing in two separate arguments it makes it clearer what the form is going to do. Its going to create a form from the $form_id, set the form's action to the passed in variable and then cache the form away with the supplied form build id.

Being separate makes the documentation clear and the code itself easier to read and understand.

hm, I wasn't sure about that either. However just passing the action as now is unclear - which form action should that be and why should one pass that? So at least we could improve the docs, but I'd prefer if the caller of that function wouldn't have to care about that at all.
Anyway, another possible way to go would be to read it from $form_state['complete form'], which is only available if the form has been processed previously, but that should be the case when someone does 'rebuilding'.

but I'd prefer if the caller of that function wouldn't have to care about that at all.

Why? Thats the whole point to address your comments in #48. The caller is the function that processes the ajax response. That is the single entity that is responsible for knowing what form was posted and what the next steps should be.

#41: drupal.system_ajax_inappropriate_671184_41.patch queued for re-testing.

... wrong patch to test, but we already determined that this patch needs work.

ad #59:
Yes, of course. But that doesn't mean the developer calling ajax_rebuild_form() has to think about that and has to explicitly pass the form action. So all I say is it would be nice if that would work automatically, or if not it needs at least better docs. Either way is fine for me as it keeps changing $form['#action'] during a regular form rebuild working (= #48).

I ended up deciding I agree with you actually. The developer calling ajax_rebuild_form() calls it by saying "Rebuild this form, with this build id and here is the previous built form" and I think that makes sense.

Great! But still we need to comment why we do this:
+ $form['#action'] = $old_form['#action'];

Also we can read the build_id out of $old_form, so we don't have to pass that separately.
We could do the same way for $form_id, but I think it makes sense to have consistent parameters for the API functions: ($form_id, $form, $form_state) ? That way we could also remove the build id from the returned list of ajax_get_form().

subscribing.

Let's get this one finished and landed. It's just way too annoying.

+++ includes/ajax.inc
@@ -536,6 +536,63 @@ function ajax_process_form($element, &$form_state) {
+ * callback will need to do the same as what drupal_get_form would do when the
...
+ * it and then if it needs rebuild, run ajax_rebuild_form over it. Then send

Missing () after function names.

+++ includes/ajax.inc
@@ -536,6 +536,63 @@ function ajax_process_form($element, &$form_state) {
+ * $form_state['clicked_button']['#array_parents'] will help you to find which

Should refer to 'trigger_element', not 'clicked button'.

+++ includes/ajax.inc
@@ -536,6 +536,63 @@ function ajax_process_form($element, &$form_state) {
+ *   The unique string identifying the desired form. If a function
+ *   with that name exists, it is called to build the form array.
+ *   Modules that need to generate the same form (or very similar forms)
+ *   using different $form_ids can implement hook_forms(), which maps
+ *   different $form_id values to the proper form constructor function. Examples
+ *   may be found in node_forms(), search_forms(), and user_forms().

I don't think we need to duplicate the entire docs of drupal_rebuild_form(). The first sentence is sufficient.

+++ includes/ajax.inc
@@ -536,6 +536,63 @@ function ajax_process_form($element, &$form_state) {
+ * @param $old_form
+ *   The previous build of this form.
+ * @return
+ *   The newly built form.

1) $old_form should clarify why it is needed.

2) Missing blank line before @return.

+++ includes/ajax.inc
@@ -536,6 +536,63 @@ function ajax_process_form($element, &$form_state) {
+  // Do not call drupal_process_form(), as that would validate and submit the
+  // form

Missing trailing period.

+++ includes/form.inc
@@ -303,34 +295,25 @@ function form_state_defaults() {
-function drupal_rebuild_form($form_id, &$form_state, $form_build_id = NULL) {
-  // AJAX and other contexts may call drupal_rebuild_form() even when
-  // $form_state['rebuild'] isn't set, but _form_builder_handle_input_element()
-  // needs to distinguish a rebuild from an initial build in order to process

We should now add a note to the function's PHPDoc that drupal_rebuild_form() should only ever be called if 'rebuild' is TRUE.

+++ includes/form.inc
@@ -303,34 +295,25 @@ function form_state_defaults() {
+function drupal_rebuild_form($form_id, &$form_state) {

The signature of drupal_rebuild_form() is changed here, but functions calling drupal_rebuild_form() are not updated accordingly.

133 critical left. Go review some!

The signature of drupal_rebuild_form() is changed here, but functions calling drupal_rebuild_form() are not updated accordingly.

Not true actually. drupal_rebuild_form is called sparingly and the three places it is called it has been updated. File_upload_ajax, ajax_callback, and drupal_rebuild_form (which doesn't use the now removed arguments).

This patch addresses all but

We should now add a note to the function's PHPDoc that drupal_rebuild_form() should only ever be called if 'rebuild' is TRUE.

Cause Im not sure that is true. Please explain.

Thanks. Right, I was mistaken about drupal_build_form().

I'll let effulgentsia comment on the "do not invoke drupal_rebuild_form() without 'rebuild' == TRUE" WTF factor, as he's much more familiar with that portion of the code ;)

At the beginning of drupal_rebuild_form() there is a comment explaining that:

  // AJAX and other contexts may call drupal_rebuild_form() even when
  // $form_state['rebuild'] isn't set, but _form_builder_handle_input_element()
  // needs to distinguish a rebuild from an initial build in order to process
  // user input correctly. Form constructors and form processing functions may
  // also need to handle a rebuild differently than an initial build.
  $form_state['rebuild'] = TRUE;

My point is that - since AJAX uses ajax_rebuild_form() now - I don't know of any "other context" that invokes drupal_rebuild_form() outside of a true rebuild scenario.

Looks like the test(s) were lost out of this version.

I may be off base here, but I noticed this when making my own D6 AJAX callbacks, that rebuilding the form when there are validation errors could cause strange things to happen.

For example, if I have a form that checks the value of a field to determine how it should be rebuilt, and another field which also is used to help me rebuild the form, and I mark both of these fields as required, but one is not filled in, on a normal (non-ajax) build, we will get some form errors, and the form will not be rebuilt, but will simply be re-rendered as it was from the cache, with the fields with errors being flagged.

In the current ajax flow, if there is a validation error, the message will be displayed, but the form will also be rebuilt, which may cause UI problems when the rebuild removes the field that had the validation error. Not to mention that in the event of an error, submit callbacks are not usually called, so we may get an inconsistent state in the rebuild, by the rebuild expecting certain flags to be set.

The normal form flow does not rebuild the form if there are form errors, should AJAX submits also be the same?

From form.inc, in drupal_build_form():

if ($form_state['rebuild'] && $form_state['process_input'] && !form_get_errors()) {
  $form = drupal_rebuild_form($form_id, $form_state);
}

Am I missing something here? Is it up to the module author to check for errors and not rebuild if there are? My understanding of AJAX forms is that they should be handled like a normal multi-step form, and if the user wants a rebuild, they should set that on the form state in there submit callback, which is only called where there are no validation errors.

I'm just wondering why the AJAX process has been made to work differently than a normal form process in regards to rebuilding when we have form errors, and the explicit forcing of a "rebuild" when it might not be right to do so.

Please let me know if I am off base here, its just one of the things I've noticed that has puzzled me a little, and I'm wondering what I'm missing.

Thanks! I agree with this simplification.

+++ modules/simpletest/tests/ajax.test	30 Mar 2010 01:32:11 -0000
@@ -204,6 +204,81 @@ class AJAXFormValuesTestCase extends AJA
+class AJAXFormActionTestCase extends AJAXTestCase {
...
+  function testFormAction() {

Hm. Normally, this would have a better home in FormsRebuildTestCase in form.test. But I see you used AJAXTestCase helper methods...

Also not sure why $old_form was renamed to $previous_form; I liked the former.

But anyway, nothing to hold up this critical fix.

131 critical left. Go review some!

Also not sure why $old_form was renamed to $previous_form; I liked the former.

The comments talked about 'previous form' when it used $old_form. That is the only rationale.

The reason I created a similar function to drupal_rebuild_form is because I felt that drupal_rebuild_form was overloaded and did things in two modes, ajax and non-ajax. And by separating things out, it cleaned up documentation (no more "If this is passed in then it does that.." and "ajax context this, normal context that"). And! it allows for further fixes for just rebuilding ajax forms, (i.e. HTML ids would be one example where the form would need to behave differently when rebuilt.)

+++ includes/form.inc	30 Mar 2010 01:32:09 -0000
@@ -306,14 +306,14 @@ function form_state_defaults() {
+ * @param $previous_form
+ *   If called from AJAX, where only part of the form will be returned to the
+ *   browser, then pass the previous build of this form, so that the properties
+ *   of the newly built form can be consistent with the prior build.

When I read the above paragraph, it wasn't clear what the 'previous build' is. Is it the $form_state? Is it the form definition? Is it the submitted form? What format is it in, and how does one obtain the 'previous build'. I think that could be clarified.

+++ includes/form.inc	30 Mar 2010 01:32:09 -0000
@@ -323,17 +323,24 @@ function drupal_rebuild_form($form_id, &
+  // During AJAX, we need to retain the build id and action of the previous
+  // form build. When the entire form will be resent to the browser, we're not
+  // passed $previous_form, and can generate a new build id and use the action
+  // set by drupal_retrieve_form() or request_uri().

It would also be good to explain 'why' we need to do that. It looks like it is trying to explain why, but it is still quite abstract.

Tracking.

I've struggled with this before, unaware that it was a core bug. It affects modules that interact with Facebook-style Statuses.

"Slightly" optimized the comments.

Looks good. Committed to CVS HEAD.

Yeah, nice work! Thanks for all the great effort.

Setting to D6, as this has haunted D6 forever, history in #484838: [rfay version] AJAX form can submit inappropriately to system/ajax after failed validation. It's a big difference there, though. We could do one of the cheap fixes, perhaps even the little one in http://drupal.org/node/484838#comment-2512712.

Couldn't figure out how to backport this to d6, with the significant changes between versions, but, the small fix mentioned in http://drupal.org/node/484838#comment-2512712 does seem to fix the issue (at least for me) when applied manually.

Line #167 in system.module has to change from:

$type['form'] = array('#method' => 'post', '#action' => request_uri());

to:

$type['form'] = array('#method' => 'post', '#action' => NULL);

As stated in that comment, this just removes the assertion that the action should be the request_uri, which apparently causes the form submit button to submit to the current url, which is the desired behavior.

I'd submit a patch, but for whatever reason I can never get a patch to pass the bot test.

This is still annoying in D6, so we should probably get it fixed. As I remember, chx doesn't approve of the approach in #484838-7: [rfay version] AJAX form can submit inappropriately to system/ajax after failed validation. However, the slightly fancier approach in this issue should be do-able.

@#86 I believe that change will break things.

I'd submit a patch, but for whatever reason I can never get a patch to pass the bot test.

Just put make a patch with name_of_patch-D6.patch and the testbot won't try to test it. As testbot should ignore D6 patches.

Any ever go through with porting this back to D6?

Another follow-up #1414510: Remove drupal_reset_static for drupal_html_id() when form validation fails

#14: form_rebuild_form_build_id-671184-14.patch queued for re-testing.

Sorry... got trigger-happy with the link clicking.

To others that are still encountering this error and stumble upon this issue, it's critical that you pass the old form into drupal_rebuild_form() as the 3rd argument to get the form rebuilt as expected:

<?php
function my_ajax_callback(&$form, &$form_state) {
  // do something really cool

  // all done, but we want to return a completely rebuilt form that takes advantage of something really cool.
  return drupal_rebuild_form('my_cool_form', $form_state, $form);
}
?>

Fixed in 7.x, just not backported to 6.x, see #83.