AJAX triggered by non-submit element fails if any elements are validated

This looks to me to be a core bug from the AJAX implementation not remembering about validation (of all types).

In ajax_form_callback() the first thing that is done is:

  // Build, validate and if possible, submit the form.
  drupal_process_form($form_id, $form, $form_state);

And that, of course, results in validation errors, for example if there is an empty required element. I haven't tried yet replacing just the element we're working with (chx has, and reported that it fails). I'll give that a shot.

I should mention that the issue here is *not* just preventing the status messages from showing. We can handle status messages explicitly (and could even eat them with drupal_messages()) in a callback that handles the status messages itself like:

function ajax_example_autocheckboxes_callback($form, $form_state) {
  // return $form['checkboxes'];
  $commands[] = ajax_command_replace(NULL, drupal_render($form['checkboxes']));
  return array('#type' => 'ajax_commands', '#ajax_commands' => $commands);
}

Attached is a demonstration module for easy testing of this bug.

This module contains three workarounds that actually let the module work - you can remove them to get the default behavior.

The three workarounds are
1. Putting a #limit_validation_errors in the submit button
2. Throwing away the status messages that may have happened when doing validation
3. Returning an array of AJAX commands instead of the default renderable array. This way we can avoid rendering the status messages.

This example, even with the workarounds, shows the problems described by this issue:

1. Validation happens at the wrong time, before the form is really filled out.
2. Even with workarounds like this, the required field is marked (outlined in red).
3. Even with these workarounds, the status messages are being inappropriately discarded. We might throw away messages we didn't want to discard.

To show the full-blown problem described by chx, just uncomment the first line of ajax_validation_example_form_callback() and let it return the renderable array it should be able to return.

Basically, we're going to have to figure out a good way to solve the validation issues.

Hm... we have a notion of ajax_triggering_element in http://api.drupal.org/api/function/ajax_form_callback/7

I'm not sure I understand the bug report though. Can we do a more focused one?

Here's the problem:

A non-submit element is used with #ajax in a form which has validated elements (like a #required element, for example).

What I expect: Validation doesn't take place until the form is ready to actually use the submit handlers, since it hasn't been filled in yet.

What happens instead: Validation of the entire form happens immediately when any #ajax element is triggered. And of course the form fails validation if a required element is still empty, etc.

Here's a first patch to deal with this. It does not do validation if the element is not a submit element. This is mostly what chx suggested in IRC. However, I removed the call to drupal_validate_form(), as I don't think validation should happen at all.

I'll follow up with a sample module for testing.

There was an error in the javascript part of the previous patch (wrong assumption about the contents of this.button). Here's an improvement.

Also attached is a sample module, with 2 ajax-enabled forms.

The first has #ajax on a non-submit element and (with this patch) functions fine.

The second has #ajax on a submit element, and when it fires, the validation happens.

This seems to work predictably on IE6/7/8 and Firefox 3 and 3.5, and Google Chrome.

Ping... We should get this fixed so nobody has to deal with it. Anybody willing to review?

+++ includes/ajax.inc
@@ -253,10 +253,42 @@ function ajax_form_callback() {
+    // drupal_html_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 scenarios that result in
+    // the form being built behind the scenes and again for the
+    // browser don't increment all the element IDs needlessly.
+    drupal_static_reset('drupal_html_id');

Besides removing the comment about drupal_validate_form(), my only question is whether we need the drupal_static_reset(). I'm unfamiliar with it, so deferring to you FAPI experts.

Powered by Dreditor.

@chx, Validation is not skipped - it's just deferred until the entire form is processed when an actual button is pressed.

I certainly defer to your experience and knowledge in this, but I suspect that when a non-submit element is active, it can just update itself or another part of the form (just like a human would) and then when submission actually takes place, all the validation stuff will happen just fine.

The flip side: If additional AJAX processing needs to know about semantic changes that normally occur during validation, we would have to take an approach like you suggest.

I do hate to make an obscure new requirement on the developer if we can avoid it at all.

Thanks for your attention to this.

OK, coached by chx, here's a version that implements his strategy of #8.

This one changes validation to not issue form errors when we're on a non-submit ajax activation.

I left in a comment that shouldn't have been there. Improved comment is the only change in this patch.

chx gave the thumbs up in IRC (he should, since he coached from beginning to end :-). Need to have @quicksketch review it, love to have @effulgentsia as well.

i thing that next question must have answers:
for example I have 2 selects. Option in the second select depends on value in the first one. Why should I post ALL the form elements? What about performance? For what purpose we need to transfer unnecessary data each time we change first select? I think that we need some property where we can store only those elemets that are required for generating answers and then pass them to ajax callback-function

@tbazadaykin:You can open an issue questioning the form submission approach for Drupal 8, but the form-based submit for AJAX, using jquery form, has been the approach since AHAH (now AJAX) was introduced. This issue will certainly not take on an architectural change to AJAX in Drupal 7.

quicksketch pointed out an indentation problem in the patch. Ironically, it was apparently because I used -b to make the patch.

This patch looks good. The logic is a bit confounding (as it often is in FAPI patches) but it certainly fixes the problem experienced when trying out the demo module. I also ran into #671184: AJAX form can submit inappropriately to system/ajax after failed validation (or #484838: [rfay version] AJAX form can submit inappropriately to system/ajax after failed validation) while testing, but clearly that's an issue that exists with or without this patch and is a separate problem.

I really appreciate that this patch continues to make AJAX silly-easy for module developers. It doesn't make any API changes, but instead just makes validation during AJAX behave the way it should.

My only qualm is that we duplicate several pieces of the drupal_process_form() instead of just calling it unconditionally and putting an extra condition inside that function. That function contains the calls to voodoo functions like form_builder() and drupal_validate_form(), which I think might be best if we kept all in one place.

Quicksketch suggested this refactor - moving the logic change into drupal_process_form instead of having it hanging out in ajax_form_callback().

This patch does not work yet - I'm passing it to quicksketch.

This patch takes the functionality from rfay's #14 and re-implements the logic as part of form_process_form(). Rather than adding a new $form_state property that contains the word "ajax", I renamed our new property to "no_submit", since that's what the property does and it matches our other existing properties (no_cache, no_redirect). If set, it does not run submit handlers and suppresses validation errors since no submission occurs.

Just a note, I think the "no_submit" property makes a lot of sense in that it prevents submission. However I'm not positive about it suppressing error messages. Maybe we should have a "no_errors" property in addition to "no_submit" and set them both to TRUE? Though then you could set no_errors to TRUE and not set no_submit... and then you'd effectively submit even if errors exist. Perhaps not a desirable ability.

I don't have time to get on IRC right now (sorry), but I want to throw this option into the mix. First of all, thanks all for the excellent work so far. This option very much builds on that foundation.

Taking a step back, what's really all that different between a clicked button and an ajax triggering element? Why can't a non-button that submits a form specify "button-level" validate and submit handlers, as well as #executes_submit_callback and #limit_validation_errors (to something other than empty array)? This patch tries to reduce that difference and address these questions. There's @todos in the patch with questions regarding the #executes_submit_callback property.

x-post with effulgentsia on #19. This doesn't consider his suggestions, just works with quicksketch's #17.

+++ includes/form.inc	25 Jan 2010 02:53:01 -0000
@@ -396,6 +396,7 @@
+    'no_submit',

This should be moved to the private section of the array (just a nit)

+++ includes/form.inc	25 Jan 2010 02:53:01 -0000
@@ -617,7 +618,7 @@
+    if ($form_state['submitted'] && !$form_state['no_submit'] && !form_get_errors() && !$form_state['rebuild']) {

$form_state['no_submit'] is not necessarily defined, so needs to be !empty($form_state['no_submit']). That will catch the not defined and the FALSE cases, without E_NOTIFY objections.

The attached patch changes only those two things from #17, which works for me, both conceptually and testing with the module in #6.

Powered by Dreditor.

BTW: #20 looks good to me, so if folks think #19 is too much for this issue, and if chx and bot approve of #20, then I'm on board. I still prefer the direction of #19 if there's community interest in going that way.

@effulgentsia, your creative solutions are ALWAYS worthy of careful study, so it's my intention to study your proposal carefully. I just haven't gotten to it. It takes some time to change gears.

Why can't a non-button that submits a form specify "button-level" validate and submit handlers

I would say that at least philosophically, if you're submitting a form on something that's not a button, then there is something wrong with the way the form is built because it means that the form has absolutely no way to provide a fall-back, non-JS behavior. My personal feeling is that it would be more worthwhile to make it so that a non-button element would trigger the handling of an existing button, so in the case JS is not available the button could still be pressed manually.

However I realize there may be situations where non-JS fallbacks are not necessary, such as an auto-saved form that also has a manual Save button (though then you've got auto-save and a manual save at the same time, more UI issues). So having non-button submits may be useful in some situations, but generally I think we should discourage them in practice. Thus I think the option would be nice to have but a bad practice to actually use in most situations.

effulgentsia's #19 is an elegant refactoring of the AJAX post process, and definitely improves the organization of the code, but it doesn't actually solve the problem in this issue, which is form_set_errors happening during vaildation on a non-button AJAX post. I read it, tested it and debugged through the code, and it just doesn't address this as far as I can tell. I believe it *can* do that with a little tweaking, but as far as I can tell it doesn't yet address it.

I guess given quicksketch's #24 and the inertia of this phase of D7 development, I would say we should probably work with the patch in #20, which doesn't refactor, but just adds a layer to resolve the issue. We have a known-working framework, and we all agree on a way to fix a particular bug in it. It's fairly easy to just go with that approach as opposed to refactoring.

I'm certainly open to pursuing #19, but I'll be we'll go the other way.

+1 to adding effulgentsia to the forms maintainers list. He may have something to say about that :-)

So unless somebody sees this a different way, I think #20 is what's on the table. If one of you FAPI gurus thinks its RTBC, please mark it.

This is a continuation of #19, but is more complete. It has the same net effect as #20, but IMO, makes more sense and cleans up a couple FAPI and AJAX things along the way. This implements my desired unification of buttons and non-buttons while still retaining some differences between the two that are commented in the code. Because it has the same net effect as #20, it addresses quicksketch's point that basic AJAX enabled forms should not trigger validation or submission from non-buttons, since doing so may likely lead to non-accessible forms. However, because it implements the refactoring that it does, it makes it very straightforward to change this default behavior. One can simply add a #executes_submit_callback to the non-button and make it behave like a button, and this can be useful in intranets or other environments where more of a full on rich application is being built and there's no need to support non-JS. This patch also allows a #ajax['trigger_as'] variable that can be set to a button (name => value) to make a non-button trigger emulate a button click (so using quicksketch's auto-save example, the form could include a "save" button, but JS can hide it and use a non-button element to trigger an AJAX submission but as though the "save" button were clicked). Finally, by moving some of the code that used to be in ajax.inc into form.inc (i.e., everything having to do with 'trigger_element'), rich applications aren't limited to AJAX. With everything D7 can do now in terms of page delivery callbacks, hook_page_alter(), and the rendering layer, we should be considering that a contrib module can implement a Flash-based or iPhone-app-based front-end to a Drupal back-end.

Anyway, please review (either before or after bot is back). On the one hand, it's kind of a lot. But OTOH, I think it all makes sense as part of this issue, since otherwise, we continue on the path of propogating code that special-cases buttons from non-buttons much more than necessary.

This one includes a fix for image buttons.

#27 does fail the bot (reporting seems to be turned off right now, but http://qa.drupal.org/pifr/test/26848).

I verified that it's an actual failure by testing with and without the patch, as the bot and HEAD have been a bit unreliable lately. The fail is in revision testing in modules/file/test/file.test (not the other file.test, in simpletest).

To run the offending test you only need to run the "file" set.

Oh, quicksketch, what would we do without file and poll providing such nice edge cases for us?

I spent the whole evening on this, and am in awe, as usual.

It cleans up AJAX FAPI in a most delightful way, organizing the code better and removing the accumulating JS-to-PHP cruft that was there. Big time.

Every example I put against it worked fine, including the validation which is the core of this issue.

A definite +1 from me. If we miss the opportunity to do this, we'll be sorry we did.

The only changes in this patch are in the testing. effulgentsia had put a #name into the test module on every submit element, and it seemed odd to me, because the code didn't seem to require that. Looking farther, I found that it was the *test* that was at fault - no change needed to be made to the mock module.

It's a strong point of effulgentsia's patch that it did not require a change to the mock *module* - no interface change is required. It's just that the AJAX tests, which are terribly fragile because they only emulate what really happens when the JS submits, had to be slightly revised to parallel the new submit.

So this patch takes the mock module back to normal, and makes the tests submit the array that they ought to to describe _triggering_element_name and _triggering_element_value.

This looks great to me! However I want to point out that we've got some pretty kludgy code here:

+      if (is_array($settings['trigger_as'])) {
+        // There should only be one key/value pair, but PHP doesn't provide a
+        // convenient syntax for getting them other than with a foreach
+        // statement.
+        foreach ($settings['trigger_as'] as $name => $value) {
+          $settings['submit']['_triggering_element_name'] = $name;
+          $settings['submit']['_triggering_element_value'] = $value;
+          break;
+        }
+      }

Instead we can write this as:

+      if (is_array($settings['trigger_as'])) {
+        $settings['submit']['_triggering_element_value'] = reset($settings['trigger_as']);
+        $settings['submit']['_triggering_element_name'] = key($settings['trigger_as']);
+      }

Which will take the first item from trigger_as even if there are multiple elements (though there shouldn't be).

A few more comments:

+      // Lots of Drupal core code and contributed modules access the
+      // $form_state['clicked_button'] variable instead of the newer
+      // $form_state['triggering_element'] variable.
+      $form_state['clicked_button'] = $form_state['triggering_element'];

For the above change, should we put a TODO in there? Seems like this makes $form_state['clicked_button'] deprecated.

+  // Determine which element (if any) triggered the submission of the form.
+  // @todo We should add a #access check here, so that someone can't fake the

@todo should be TODO: in code comments that are not PHPdoc.

+    if (isset($element['#button_type'])) {
+      $button_type = $element['#executes_submit_callback'] ? 'submit' : 'button';

This seems pretty convoluted to me. If $element['#button_type') is set why don't we just do $button_type = $element['#button_type'] (or get rid of the assignment entirely)?

I totally agree with the improvements in #30. Thanks, rfay. I also agree with #31 and that's implemented in this patch. I also agree with #32.3 and that's implemented in this patch, though I'd appreciate a review of how that affects everything having to do with $form_state['buttons'] from quicksketch and/or chx (I think it's a nice clean up, but I'm not sure if I'm missing something here).

Regarding #32.1: What's our policy on deprecating something after alpha1? Seems too late to tell module authors "don't use $form_state['clicked_button']". Given that, is the comment in the patch appropriate, or is quicksketch right about adding a TODO anyway?

Regarding #32.2: Really? I've been told in the past to use @todo instead of TODO, even within inline comments. I'm fine with either, I just want a double-check on what our current standard is, so I know for here, and for other patches I work on.

Additionally, this patch implements the following change:

+++ includes/form.inc
@@ -936,14 +937,29 @@ function _form_validate(&$elements, &$form_state, $form_id = NULL) {
+    elseif (!empty($form_state['triggering_element']) && !isset($form_state['triggering_element']['#executes_submit_callback'])) {
+      // By default, buttons have either TRUE or FALSE for
+      // #executes_submit_callback, while non-buttons have NULL for it. A
+      // non-button can set #executes_submit_callback to either TRUE or FALSE to
+      // behave like a button and trigger validation as buttons do. But if it
+      // leaves #executes_submit_callback as NULL and doesn't set
+      // #limit_validation_errors, then by default we suppress all validation
+      // errors, which is safe, because without #executes_submit_callback of
+      // TRUE, no submit handlers will execute. A non-button that wants some
+      // validation to occur must either set #limit_validation_errors or
+      // #executes_submit_callback (to either TRUE or FALSE) in order to
+      // override this default behavior of suppressing all validation errors.
+      form_set_error(NULL, '', array());
     }

Yuk. Magic behavior based on the distinction between FALSE and NULL of an unrelated property is a WTF in the making (why would any sane person expect validation behavior to be different when #executes_submit_callback is NULL vs. FALSE?). This was just me being silly and propagating HEAD's magic behavior around this property, but since the rest of the patch removes such magic behavior, we should just remove it fully. The magic behavior we're looking for is to treat buttons differently from non-buttons with respect to their #limit_validation_errors default. Hm. Sounds like a job for system_element_info(). That's what this patch does.

This review is powered by Dreditor.

With the status change this time.

Fixed test failures.

I have no idea what the invalid syntax that bot is reporting in #36 is referring to. This is just a re-roll for latest HEAD, hoping that fixes it.

A rundown of todo comments in core:

54 current // TODO comments
67 current // @todo comments
29 current @todo comments in PHPdoc

So it looks like @todo is the winner here for majority. I just don't like it because phpEclipse only identifies @todo's as PHPdoc or TODO: in normal code comments. @todo in normal comments doesn't get read.

So basically I'd prefer TODO:, but clearly we don't have a consistent standard for any format so I don't think it's a valid complaint (or if it is, we'll decide this in another issue).

I really wanted to review the actual patch, but first, to stop the current confusion: It's always @todo. Without any leading or trailing colons or punctuation. When exceeding more than 80 chars, ideally indent following lines belonging to the same @todo with 2 spaces. I thought I had clarified that in the commenting standards, but will double-check.

EDIT: Done: http://drupal.org/node/1354

Overall, this looks quite impressive. I wasn't able to understand all implications by just reading the patch, so here comes a first patch review; will have to apply it to review all changes in context.

+++ includes/ajax.inc	30 Jan 2010 02:15:24 -0000
@@ -483,13 +446,43 @@ function ajax_process_form($element, &$f
+      // uniquely, in which case a match on value is also needed. @see
+      // _form_button_was_clicked().

+++ includes/form.inc	30 Jan 2010 02:15:25 -0000
@@ -280,19 +280,20 @@ function form_state_defaults() {
+ * which part. @see ajax_form_callback() for an example.

(and elsewhere) @see should be on its own line.

+++ includes/form.inc	30 Jan 2010 02:15:25 -0000
@@ -1303,22 +1318,50 @@ function form_builder($form_id, $element
+  // Final stuff for the form element after form_builder() has run for all other

s/stuff/tasks/

+++ includes/form.inc	30 Jan 2010 02:15:25 -0000
@@ -1303,22 +1318,50 @@ function form_builder($form_id, $element
+      // Buttons have flat (without brackets) #name, regardless of their
+      // #parents property.
+      $form_state['values'][$form_state['triggering_element']['#name']] = $form_state['triggering_element']['#value'];

@@ -1415,52 +1458,72 @@ function _form_builder_handle_input_elem
-      // In most cases, we want to use form_set_value() to manipulate
-      // the global variables. In this special case, we want to make sure that
-      // the value of this element is listed in $form_variables under 'op'.
-      $form_state['values'][$element['#name']] = $element['#value'];

I'm not sure what this comment tries to tell me. Seems like the actual info about 'op' got lost?

+++ includes/form.inc	30 Jan 2010 02:15:25 -0000
@@ -1303,22 +1318,50 @@ function form_builder($form_id, $element
+      // Lots of Drupal core code and contributed modules access the
+      // $form_state['clicked_button'] variable instead of the newer
+      // $form_state['triggering_element'] variable.
+      $form_state['clicked_button'] = $form_state['triggering_element'];

ugh. Really required?

+++ includes/form.inc	30 Jan 2010 02:15:25 -0000
@@ -1415,52 +1458,72 @@ function _form_builder_handle_input_elem
+  // Determine which element (if any) triggered the submission of the form and
+  // keep track of all the buttons in the form for form_state_values_clean().
+  // @todo We should add a #access check here, so that someone can't fake the
+  //   click of a button they shouldn't have access to, but we can't do that
+  //   without changing the way file_managed_file_process() toggles between the
+  //   upload and remove buttons.
+  if (!empty($form_state['input'])) {
...
+    if (isset($element['#button_type'])) {
+      // All buttons in the form need to be tracked both for
+      // form_state_values_clean() and for _form_builder_ie_cleanup().
+      // @todo When #access is checked in an outer if statement (see above), it
+      //   won't need to be checked here.
+      if ($form_state['programmed'] || !isset($element['#access']) || $element['#access']) {
+        $form_state['buttons'][] = $element;

Awesome to have those @todos!

So the issue is that there may be #process callbacks that run after input handling of the current element? Could we move the #access check to the end of form_builder()?

Powered by Dreditor.

Since sun also asks about this, this includes #32.1, so now, all of quicksketch's feedback is incorporated. This also incorporates all of #41, except:

So the issue is that there may be #process callbacks that run after input handling of the current element? Could we move the #access check to the end of form_builder()?

Wow, that's an interesting point, but no, we don't handle that anywhere actually, and until we do, I don't think we need to do it for buttons. If, a #process or #after_build function runs after input processing for a particular element and messes with #access of an element that's already been processed, that would cause problems with the standard (non-button) input processing in HEAD, and it's probably beyond the scope of this issue to fix that.

The specific issue we're dealing with here is that file_managed_file_process() toggles the upload/remove buttons during the initial form build rather than during a rebuild, so the initial form build doesn't match the build of what the user submitted. I added comments in this patch that clarify this.

Re #23: #704078: Add effulgentsia to the form system maintainers in MAINTAINERS.txt

#42: drupal.ajax_validation_684846_42.patch queued for re-testing.

If you click on "view details" in #42, it shows that the patch passes (not sure why it's not green in the comment). Anyway, now that bot is back and the patch passes, what's left for it to be RTBC?

#42: drupal.ajax_validation_684846_42.patch queued for re-testing.

I took a look at #42 again, installed it, ran the manual tests.

It passes the bot.

It's had lots of participation in review.

All the issues seem to be resolved.

I'm going to call it RTBC.

Subscribing to look at this a bit later. Thanks for the awesome collaboration here folks!

#42: drupal.ajax_validation_684846_42.patch queued for re-testing.

awesome work!

I just missed the availability of the triggering_element during form rebuilding so I wanted to roll a patch for that. Luckily I discovered this issue beforehand, as it fixes that too. The changes make totally sense and are just necessary to properly use the #ajax system without a button!

I tried the patch and it's working fine now! I also managed to trigger the ajax replacement with a link that triggers as a button. So the button can still serve as fallback without js while else the link can take over. However for the link to submit the form I needed a small fix to ajax.js, but let's tackle that in a follow-up as it's not directly related to this issue.

Also code + comments is fine, the comments are clear to me and add some todos that should be done anyway. -> Let's get this great work in asap!

Subscribing

Regarding the patch in #42, I've applied it and tested the weather module with it. Without the patch, the dependend dropdown is not working, but I can confirm that it's working fine with the patch applied.

Regards,
Tobias

+++ includes/ajax.inc	30 Jan 2010 18:58:56 -0000
@@ -324,32 +308,11 @@ function ajax_form_callback() {
+  if (!empty($form_state['triggering_element'])) {
+    $callback = $form_state['triggering_element']['#ajax']['callback'];

If this element is tied closely with AJAX, is there a reason not to include 'ajax' in its name somewhere as a general clue? I guess because in D8 you're hoping to remove the 'clicked_button' property and use this instead?

+++ includes/ajax.inc	30 Jan 2010 18:58:56 -0000
@@ -483,13 +446,43 @@ function ajax_process_form($element, &$f
+    // Add special data to $settings['submit'] so that when this element
+    // triggers an AJAX submission, Drupal's form processing can determine which
+    // element triggered it.
+    if (isset($settings['trigger_as'])) {

Same question here?

+++ includes/ajax.inc	30 Jan 2010 18:58:56 -0000
@@ -483,13 +446,43 @@ function ajax_process_form($element, &$f
+      if (is_array($settings['trigger_as'])) {
+        // Use the first key/value pair in the array.
+        $settings['submit']['_triggering_element_value'] = reset($settings['trigger_as']);
+        $settings['submit']['_triggering_element_name'] = key($settings['trigger_as']);
+      }
+      else {
+        $settings['submit']['_triggering_element_name'] = $settings['trigger_as'];
+      }

This looks really messy. Can't we just always make $settings['trigger_as'] an array?

+++ includes/form.inc	30 Jan 2010 18:58:56 -0000
@@ -280,19 +280,21 @@ function form_state_defaults() {
+    'buttons' => array(),

Rather than "buttons" (which was removed from ajax.js) should we make this "submissions" or similar, so they more closely match? Or is the context completely different?

+++ includes/form.inc	30 Jan 2010 18:58:56 -0000
@@ -936,22 +939,35 @@ function _form_validate(&$elements, &$fo
+    // If submit handlers won't run (due to the submission having been triggered
+    // by an element whose #executes_submit_callback property isn't TRUE), then
+    // it's safe to suppress all validation errors, and we do so by default,

Hm. This sounds scary to me. Why would we want to suppress all validation errors by default?

+++ includes/form.inc	30 Jan 2010 18:58:56 -0000
@@ -1303,22 +1319,60 @@ function form_builder($form_id, $element
+      // @todo To support AJAX form submissions from non-button elements,
+      //   the older Drupal variable $form_state['clicked_button'] got
+      //   abstracted to $form_state['triggering_element'], but this was done
+      //   after Drupal 7 alpha release. Until we're ready to break
+      //   compatibility with modules using $form_state['clicked_button'], we
+      //   continue to provide this variable.

I would shorten this to just "@todo: Legacy support. Remove in Drupal 8." unless you feel the added details here are particularly relevant.

+++ includes/form.inc	30 Jan 2010 18:58:56 -0000
@@ -1415,52 +1469,78 @@ function _form_builder_handle_input_elem
+  // @todo We need to add a #access check here, so that someone can't fake the
+  //   click of a button they shouldn't have access to, but first we need to
+  //   fix file.module's managed_file element pipeline to handle the click of
+  //   the remove button in a submit handler instead of in a #process function.

Um. WOAH. We are trying to introduce known insecure code into core's Form API because an optional module has weird behaviour with it? Nuh uh.

How bad is the breakage in File module? If it's "can't upload files at all" it might make sense to postpone this patch on cleaning that one up. If it's "can't add more than 3 files to a node" then I'd file a separate issue about it and we can clean it up afterward. But no way should we introduce security problems into Form API for which there's a possibility we might forget to clean up before release.

+++ includes/form.inc	30 Jan 2010 18:58:56 -0000
@@ -1468,38 +1548,22 @@ function _form_button_was_clicked($form,
 function _form_builder_ie_cleanup($form, &$form_state) {

Hm. This function got a hell of a lot shorter. Are you sure it's still doing what it needs to be doing? I remember eaton spending a good month or two of hair-pulling and tears trying to get that working the first time.

Was this tested incredibly thoroughly on image buttons in IE6?

+++ modules/poll/poll.test	30 Jan 2010 18:58:57 -0000
@@ -341,7 +341,7 @@ class PollJSAddChoice extends DrupalWebT
-    $commands = $this->drupalPostAJAX(NULL, $edit, 'poll_more');
+    $commands = $this->drupalPostAJAX(NULL, $edit, array('op' => t('More choices')));

I don't really understand these changes, and they seem to make the tests more brittle; it's far more likely we change the name/label of buttons going forward than the internal name of properties or whatever the heck that was doing before.

Also, drupalPost() has a $form_id argument as of a few days ago. Does it make sense to add that to drupalPostAjax and use it here, or is that irrelevant?

Also, related to tests... I would've expected some new tests to help codify the problems that trigger the bugs we're trying to fix to happen, but the only thing I see are these types of renames and some changes to drupalPostAjax(). What's up with that?

Powered by Dreditor.

Re-roll for HEAD changes plus incorporated some of #55. Here's a point-by-point response to #55:

If ['triggering_element'] is tied closely with AJAX, is there a reason not to include 'ajax' in its name somewhere as a general clue? I guess because in D8 you're hoping to remove the 'clicked_button' property and use this instead?

I'm hoping in D7 people stop using 'clicked_button' as well. This patch makes that possible, but leaves the legacy 'clicked_button' variable around, since it's too late to break API for modules that have already been ported. The change from 'clicked_button' to 'triggering_element' is driven by AJAX, since that's one context in which something other than a button can submit a form. But once we accept the fact that form submissions can occur for reasons other than a button click, there is no reason to tie the concept of a triggering element to AJAX. Even a normal "button click" without AJAX is still just a specific example of an element triggering the form submission. I'm also hoping to see D7 contrib modules that implement UIs in Flash and other RIA technologies, where form submissions can be triggered by buttons or non-buttons, and it's a bit of a stretch to call those environments "AJAX".

This looks really messy. Can't we just always make $settings['trigger_as'] an array?

Yes. Done in this patch.

Rather than "buttons" (which was removed from ajax.js) should we make this "submissions" or similar, so they more closely match? Or is the context completely different?

Different context. $form_state['buttons'] tracks all the buttons on the form and has nothing to do with what triggered the form submission other than being an aid for detecting single-button forms to handle the "use of ENTER key on single button forms" logic in _form_builder_ie_cleanup(). Since the variable exists, perhaps contrib modules use it for something else as well. This patch does not require it to be added to form_state_defaults() per se, but I think it's a good idea to do so, since other $form_state defaults are added there. Adding it there fixes it from being NULL to being an empty array for forms with no buttons, which we don't have in core, but may in edge case contrib modules that present an AJAX-only interface with no ability to submit the form if JavaScript is disabled.

Hm. This sounds scary to me. Why would we want to suppress all validation errors by default?

This goes back to the OD and discussion that led to #20. So basically, this issue is about not triggering validation (or more precisely, validation errors) when a non-button submits the form, since presumably, a non-button is only used for minor AJAX stuff and not for the form's "real submission". The solution to a similar problem for buttons is to set #limit_validation_errors => array() on the button. IMO, this would be an okay requirement to place on non-buttons too (if you don't want validation errors, set this property). But, others have argued that this should be the default for non-buttons rather than needing to be set explicitly. I'm not really sold on that, but I'm okay with it as long as it's done safely, which it is in this patch (and every patch since #20). What keeps it safe is the check for !$form_state['submitted'], which means submit handlers won't run, so whether or not validation errors are suppressed is strictly a UI thing and not a security thing. I'm not entirely happy with treating buttons that don't run submit handlers (i.e., "button"s as opposed to "submit"s) differently from non-buttons that don't run submit handlers (i.e., the former don't limit validation errors by default and the latter do), but at least this patch encapsulates that difference in system_element_info(). I tried to comment this hunk as best I could, but please let me know if there's a way to capture all this better.

I would shorten this to just "@todo: Legacy support. Remove in Drupal 8."

Done.

Um. WOAH. We are trying to introduce known insecure code into core's Form API because an optional module has weird behaviour with it? Nuh uh.

No, we uncovered an existing possibility of a security hole in FAPI, and are adding a @todo to fix it. Fixing it without fixing file.module results in file.module tests failing. I opened #736298: Refactor file.module to use proper button click detection, enabling FAPI to improve button click detection security and added links to that issue in this patch. I say "possibility of a security hole", because whether or not it results in an actual security hole depends on whether any modules use #access on buttons with submit handlers that would be unsafe to have triggered by an untrusted user.

Hm. This function got a hell of a lot shorter. Are you sure it's still doing what it needs to be doing? I remember eaton spending a good month or two of hair-pulling and tears trying to get that working the first time. Was this tested incredibly thoroughly on image buttons in IE6?

Most of the shortening was just due to code re-organization. For example, moving the check if we're on the 'form' element to outside the function, and centralizing what to do once you have a 'triggering_element' to a single place outside this function rather than having it duplicated in 3 places. But the "if" statement itself did change from requiring no buttons of the 'button' type and returning the first submit button if there are more than one to requiring a single button of any type and returning that one. The change more closely matches what the code comments for this in HEAD say the function does (but doesn't actually do). But a review from eaton would be great, since maybe there's some IE-quirk he remembers that's different from what is captured in the HEAD code comments. The actual HEAD code seems a little odd, pointed out by quicksketch in #32, since whether something is in the 'button' or 'submit' bin is based on the #executes_submit_callback property, which can be overridden on a button-by-button basis in a way that can't possibly have impact on how IE handles an ENTER key.

I don't really understand these changes, and they seem to make the tests more brittle; it's far more likely we change the name/label of buttons going forward than the internal name of properties or whatever the heck that was doing before.

This brittleness exists for all buttons of all forms, not just AJAX. Everywhere in Drupal, we determine which button is responsible for the form submission by its #name and #value, and every test that calls drupalPost() passes in the button value. The change in this patch makes AJAX consistent with this, so the tests needed to be changed accordingly.

Also, drupalPost() has a $form_id argument as of a few days ago. Does it make sense to add that to drupalPostAjax and use it here, or is that irrelevant?

Separate issue. The $form_id is to identify the form (not the element) in a situation where multiple forms exist on a page and the same button name and value is used across the different forms. Whether drupalPostAjax() also needs a $form_id added seems like a good question to ask as a follow up to whatever issue added it to drupalPost().

I would've expected some new tests to help codify the problems that trigger the bugs we're trying to fix

Good point. I added the "Needs tests" tag to this issue.

CNR for bot and webchick. But in addition to review, this issue needs tests as per above.

Thanks for the reroll, effulgentsia. Nice to see bot happiness on this one.

I should be able to add a test that addresses the original issue (form has validated element, AJAX activation of another element).

Does anybody see other tests that should be added for this one?

Here's a reroll that does nothing but add a test for the original failure case. The test does fail against HEAD and succeed in this context.

If you're interested in just the test, I've attached that as a text file.

[Sorry - timeout on submit.... but d.o got it.]

[timeout on submit.... but d.o got it.]

[timeout on submit.... but d.o got it.]

re-uploading #58, cause d.o. seems to have hiccuped on it.

+1 for keeping 'triggering_element' - that way it's also possible to write code that works regardless the triggered element is a button or not.

I tested the patch in #63 with my trigger-by-link use case and it's working fine. When triggering as a button it also correctly respects its #limited_validation settings. Imo not doing validation for stuff that doesn't execute a submit callback is completely fine.

@test-case:
Minor, but still the test case form does violate the testing modules' namespace. Thus setting back to CNW.

Also the function name ajax_validation_non_submit_form_submit() is weird, perhaps we can come up with a better form_id? Then the test only uses a assertNoText() assertion, so I fear it could break silently. Better also assert somehow the form is basically working fine, e.g. make sure the callback is invoked properly.

+++ includes/form.inc
@@ -1469,38 +1545,22 @@ function _form_button_was_clicked($form, &$form_state) {
+  if (!$form_state['programmed'] && !isset($form_state['triggering_element']) && count($form_state['buttons']) == 1) {
+    $form_state['triggering_element'] = $form_state['buttons'][0];
   }

I did some IE testing and this code is wrong. I will roll a patch tomorrow that fixes this.

145 critical left. Go review some!

Here's a patch to respond to @fago, #64. It improves the test, fixes the function naming. Thanks for the review.

@fago, I studied the JSON that results from the POST looking for something more authoritative than a NoText assertion, and didn't find one. I did, however, find a better thing to trigger the NoText on. As you point out, this approach is fragile, but I didn't find anything better. All we have is the error message coming back with the status messages.

effulgentsia will still work on this tomorrow per #65.

#66: drupal.ajax_validation_684846_66.patch queued for re-testing.

Fails with a different set of fails each time? What's up with that?

[Edit] I just ran both of the failing tests using an environment with clean urls turned off and in a subdirectory, like the bot has. Success, no problems.

The imagefield validation test is one that has failed randomly on this patch before.

This has updated code and comments for button click detection in response to webchick's review in #55. No more _form_builder_ie_cleanup(), yay! It got shortened to the point of making sense to just inline into form_builder(). I also added tests for it. Note that some of these tests fail in HEAD, but now pass, and these tests and comments are based on what I observed when testing in IE. Note that I observed other bugs unrelated to this issue when using image buttons that have #value. I'll take that up in a separate issue though: it's bugs that exist in HEAD and are unaffected by the code in this patch, but if you look at HEAD's DrupalWebTestCase::handleForm(), you'll notice that we don't have proper testing of the way browsers actually submit image button clicks.

Wow, I keep forgetting that this one hasn't gone in yet. It's so important. I was just working on an AJAX graceful degradation example... and it's going through submit and messing everything up! Have to get this one done and in.

Thanks for all the excellent work on this, effulgentsia.

I sent a PM to eaton asking him to review #71's change to _form_builder_ie_cleanup(). His input on anything else about the patch would be great too. @fago: given #64 and #66, can you add a follow-up comment as to whether you're satisfied. I'm hoping that with a +1 from fago and eaton, this can be RTBC again. I do plan on getting to #736298: Refactor file.module to use proper button click detection, enabling FAPI to improve button click detection security, which needs to happen regardless of this patch and can happen before or after this one lands, but I have some other issues I'm trying to finish first, and if anyone wants to help on that other issue, that would be awesome.

Everything I noted in #64 has been addressed properly, so +1 to get this in from my side!

I'm going to say that we should push this to RTBC and commit. It's had outstanding work done on it, excellent community participation and review. And we're in the early part of a new alpha/beta phase, which would be just the right time for this to be committed.

I just did another round with this, tested it on IE6/7/8, ran the new test effulgentsia put in by hand, and it all looks good to me.

Let's get it in.

#71: drupal.ajax_validation_684846_71.patch queued for re-testing.

Yay! Still passes.

Spent 20 minutes looking at this patch and it looks good. Well documented, and I like the extra tests. Unfortunately, when I tried to apply the patch after 20 minutes, it failed to apply. Can we get a reroll?

Ok I rerolled this.
I'm not a coder of this, so don't ask me to give details.
The part that was failing was the ajax testing.

I changed (for example)

$commands = $this->drupalPostAJAX($form_path, $edit, array('op' => t("AJAX 'restripe' command")));

to

$commands = $this->discardSettings($this->drupalPostAJAX($form_path, $edit, array('op' => t("AJAX 'restripe' command"))));

I think the discardSetting part has been introduced somewhere this week.
Hopefully this doesn't brake things...

@aspilicious: thanks! Confirming that #79 is a good re-roll. It doesn't have function names identifying the patch hunks; doesn't matter for this one, since it's already been reviewed, but please add the "-p" switch when running "diff" to make future patches: that provides function names in the hunk headers.

I use eclipse don't know how to do that...
It would be nice if someone can tell me that...

Bumped #736298: Refactor file.module to use proper button click detection, enabling FAPI to improve button click detection security to critical.

Sorry, been out sick for a number of days but I'm back in the saddle.

I'm definitely out of the loop for some of this stuff, as I haven't been watching the progress of the AJAX code, but I'm definitely in favor of the shift from clicked_button to trigger_element as we move forward with AJAX and AHAH.

I'm nervous about the validation skips, but at the end of the day we're going to have to skip it somehow. Long term,we need to address the underlying FAPI flow; it was never intended for partial validation and processing, and a lot of it is tremendously awkward. For D7 though, this looks like a smooth fix that will help solve a lot of problems.

Committed to CVS HEAD.

see #753270: Impossible to trigger form submit with a link for a follow-up to fix ajax triggers with links

+    // If a form contains a single textfield, and the ENTER key is pressed
+    // within it, Internet Explorer submits the form with no POST data
+    // identifying any submit button. Other browsers submit POST data as though
+    // the user clicked the first button. Therefore, to be as consistent as we
+    // can be across browsers, if no 'triggering_element' has been identified
+    // yet, default it to the first button.
+    if (!$form_state['programmed'] && !isset($form_state['triggering_element']) && !empty($form_state['buttons'])) {
+      $form_state['triggering_element'] = $form_state['buttons'][0];
+    }

At #1008644: Programmatic form submissions should always get a triggering element, just like regular form submissions do, we are proposing to remove the $form_state['programmed'] check from the above if() statement, which looks like it was added here.

Because of that, @webchick wanted to know if someone who participated in this issue could jump on over there and comment on whether removing that makes sense. If you have time for a review, thanks in advance!