Drupal 7 has a nice AJAX/AHAH improvement that allows a form to use AJAX without adding a menu entry or writing the full guts of a callback function. You can just use #ajax['callback'] to point to a function (see this entry in the module upgrade doc).

However, this nifty new function only works for form elements of type 'submit' or 'button', not for selects or checkboxes or radios, etc. It should really work for all.

This patch allows AJAX with a simple callback to work for all element types.

If I'm not mistaken, it also will improve some serious issues with AJAX losing its bindings on fieldsets with duplicate field names as well.

To ease the reviewing process, I have also attached a very simple module which provides a number of code examples using select- and checkbox-driven AJAX with callbacks instead of paths. This module is just demo code and is not intended to be reviewed for style, but rather to provide an easy on-ramp for reviewing the patch.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

rfay’s picture

This failed the automated testing, and also failed manual testing on IE6/IE7, although it was fine on IE8, and all the others that usually work. I'll give it another go.

rfay’s picture

Here's another round of the patch, along with an updated test module that can be used with it.

Please note that on IE the checkbox-driven AJAX does not work due to #557284: AHAH/AJAX bindings do not work on checkbox or radio in IE6/7/8 (and now IE9), which has been an AHAH issue at least since Drupal 6, and is unrelated to this patch. I had the unfortunate experience of discovering it while testing this, though.

Suggestions for reviewers:

  • Use the attached module as a base for your test, rather than having to write your own.
  • Install and enable and then try the various permutations. Experiment with changing the form settings.
  • If I can help in any way with understanding AJAX, I'll be happy to help. rfay on IRC.
rfay’s picture

Status: Needs work » Needs review
quicksketch’s picture

Status: Needs review » Needs work

This patch makes good sense to me, I think just a little cleanup would be good before committing.

- I believe this approach will break when #name is manually specified on buttons, since we're assuming that all buttons have the name "op", which might not be true.
- We should probably prefix our special hidden element with "ajax_". Though "ajax_triggering_element" is getting pretty long.
- I don't find the comments about #tree to be appropriate for the added code. These docs are more appropriate for the Form API reference pages or in sample modules, most developers aren't going to find the code comments in ajax.js. However, we definitely should describe why we're adding a hidden field, which isn't explained.

sun’s picture

rfay’s picture

Status: Needs work » Needs review
FileSize
2.06 KB
2.51 KB

Thanks for the review, @quicksketch. You were right about #name, of course. This patch fixes that issue. I also prefixed the hidden element (which is no longer sent for submit-type elements). And the #tree comments are pulled, replaced with a more appropriate comment about the reason for the hidden field.

The demonstration module is updated to have a test with many different buttons of varied types.

Status: Needs review » Needs work

The last submitted patch failed testing.

rfay’s picture

The patch failed to apply due to a moving head, but the real problem was that when I re-rolled it, all the AJAX stuff was broken as discussed in #367567: Use AJAX framework for "Add more" / load include file containing form / introduce $form_state['build_info']. So trying to figure out the path forward on this one.

rfay’s picture

Status: Needs work » Needs review
FileSize
2.22 KB
2.68 KB

I was able to demonstrate this patch working correctly if I move all my forms into the test .module. So here is the re-rolled patch, which basically only deals with the removal of drupal_function_exists(), and the working test module. We know that HEAD has some serious issues in this area right now, but I believe this works. Of course the constraint of having to have all forms in the .module is unacceptable...

rfay’s picture

This patch should be absolutely fine now the the AJAX issues are resolved. Hoping to get it in before the code freeze. I'm hoping you'll agree it's an innocuous improvement.

sun’s picture

+++ misc/ajax.js
@@ -143,6 +143,11 @@ Drupal.ajax = function (base, element, element_settings) {
+      if (element.type != 'submit') {
+        // Server-side code needs to know what button triggered the call, so it can find the
+        // correct #ahah binding, so we pass it the element name through a hidden field.
+        ajax.form.append('<input type="hidden" name="ajax_triggering_element" value="' + element.name + '" />');
+      }

I don't quite understand why we do not simply trigger the submit for the button via JS then - instead of going through the loops of Form API trickery introduced here?

Beer-o-mania starts in 3 days! Don't drink and patch.

sun’s picture

I question this, because the current approach taken would increase the WTF-factor regarding #370537: Allow suppress of form errors (hack to make "more" buttons work properly) even more, i.e. we start to build + validate partial forms instead of full, pass back partial forms, trigger 'clicked_button' of a button that was not actually clicked, etc.

rfay’s picture

@sun, regarding #12: If we don't know which element triggered the AJAX, we can't find the correct #ahah, thus can't support AHAH. Does that answer your question? So just triggering the submit doesn't do the job. Thanks for your review!

rfay’s picture

Per sun's suggestion I'm adding a technical summary of why this patch exists and what it does:

In D6 you had to set up a full (and complicated) callback and a menu entry for every AHAH item.

In D7, the #ajax['callback'] was added to simplify that. Instead of setting up a full path and the callback and all the glue code, you could just grab the form, the form_state, and do whatever processing you wanted and return the HTML piece you wanted. The catch is that this works *only* for submit and button elements, because the $form_state['clicked_button'] is only populated for those items, so we can't find the #ajax for other form elements. (This is a browser/HTTP problem, outside the scope of Drupal.)

This patch aims to make the simple use of #ajax['callback'] available for all form elements that can be used with AJAX.

Here's how it works:

  1. If the triggering element is not a button, the Jquery attached to the form element adds an additional field to $_POST, 'ajax_triggering_element', which is element.name from the form element.
  2. element.name may be either a simple name like 'country' or 'city' or a more complex name like address['country'] (if in a fieldset named 'address', for example.)
  3. The path system/ajax is called by the jquery, executing ajax_form_callback() (no change here)
  4. ajax_form_callback() checks for the existence of $_POST['ajax_triggering_element'] and grabs it if it's there. (new in the patch)
  5. ajax_form_callback() determines the triggering element by using either the traditional $form_state['clicked_button'] if a button was clicked, or the $_POST['ajax_triggering_element']
  6. If $_POST['ajax_triggering_element'] is complex, like 'address['country'], then we break it apart to walk the form and find the correct element, in this case, $form['address']['country'].
  7. At this point, we have the correct element, get the correct #ajax settings from it, and processing continues.

The overall result is that we now have a way to determine the triggering element in order to grab the '#ajax' configuration for it. Elements other than buttons can use the 'callback' technique.

effulgentsia’s picture

I absolutely support having non-buttons capable of triggering AJAX callbacks that can benefit from #ajax['callback']. While other ways of solving this problem might be possible, this seems like a perfectly reasonable way of doing it, and I'd rather see this make its way in rather than not having the problem solved.

I don't see how this patch creates any harm. If an alternate technique is discovered after code-freeze, we could decide at that time whether to use it or stick with this one. I'd rather have this make its way in than wait till D8 for this problem to be solved.

I do think it's conceivable that there are cases where this code alone is insufficient. Since, $form_state['clicked_button'] isn't actually being set (and arguably shouldn't be if the element triggering isn't a button), other parts of FAPI that rely on that property to know what triggered the submission won't have access to the desired information. However, that's already the case. Without this patch, a module can implement an AJAX callback that doesn't benefit from ajax_form_callback(), have that triggered by a non-button, and have FAPI run without a $form_state['clicked_button'], so I don't think this patch takes us backwards at all, and if there's a desire to change $form_state['clicked_button'] to $form_state['triggering_element'], that can be taken up in a separate issue.

My only nits are with the changes to ajax.js.

1. Please change #ahah to #ajax in the comment.
2. Seems like this code can append the hidden element multiple times. How about instead of a hidden element, we just modify form_values in Drupal.ajax.prototype.beforeSubmit()?

quicksketch’s picture

Status: Needs review » Needs work

I'm in support of having #ajax work on non-buttons also. While #370537: Allow suppress of form errors (hack to make "more" buttons work properly) adds support for #validate_section, this property will remain button-specific, the same way that #validate and #submit are button-specific.

I see where sun is coming from though, in actuality if I would've done this on my own I probably would have enforced all #ajax behaviors to trigger by clicking an existing button (which makes for consistent degradation). However I think rfay's approach is excellent and probably makes for a better developer experience than forcing developers to make a button.

Marking needs work per effulgentsia's review, which I agree with on both points. However I don't think form_values may be modified in beforeSubmit() (JavaScript does not pass by reference), but we should definitely ensure the hidden doesn't get appended multiple times. I'd suggest just removing the hidden in ajax.success() and ajax.error().

sun’s picture

Somehow, quicksketch ate my brain, and spit out my thoughts before I could even express them ;)

So, quicksketch's follow-up pretty much covered everything.

Amendment: This has the potential to replace autocomplete. Maybe not completely, but definitely allowing it to be based on the new AJAX framework.

sun.core’s picture

Status: Needs work » Needs review
effulgentsia’s picture

Here's a version with my nits from #16. Also, I changed it from using the element name (which resulted in problems if #tree wasn't set at the top level of the form) to passing formPath (array_parents) to the js settings. And I made it do this regardless of whether the element is a button or not, which I think removes a little of the WTF factor, and also helps with edge cases where FAPI gets confused what the clicked button is (for example, 2 buttons have same name and value, but in different parts of the tree).

effulgentsia’s picture

Oh, and @quicksketch, in javascript, arrays are objects, so are always effectively passed by reference.

rfay’s picture

@effulgentsia, thanks for the outstanding and elegant revision of the patch. I really like what you did - it's a significant improvement. I've tried it in a number of contexts and found it to be solid. I was worried at first that the issue of #name being set on a submit would break it (the reason for falling back on $form_state['clicked_button'] in the earlier version) but it does not because of your use of formPath.

The attached patch is just a small incremental change to your patch:

  • Per a discussion with @sun, destroy $_POST['ajax_triggering_element'] before it ever has a chance to confuse or conflict with form validation now or in the future.
  • I removed the fallback to using $form_state['clicked_button'], as the ajax_triggering_element is added unconditionally, and works perfectly.
  • A couple of comments added.

The test module that I've been using is also attached. The menu item "select-driven AJAX experiment" has quite a bit of diversity, including submits and named submits in nested fieldsets, in addition to selects and checkboxes.

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

This *needs* to check for #access:

+  if (!empty($triggering_element_path)) {
+    $triggering_element = $form;
+    foreach (explode('/', $triggering_element_path) as $key) {
+      if (!empty($triggering_element[$key])) {
+        $triggering_element = $triggering_element[$key];
+      }
+      else {
+        $triggering_element = NULL;
+        break;
+      }
+    }
+  }
rfay’s picture

Status: Needs work » Needs review
FileSize
3.92 KB

Per @Damien's request, the #access is now checked.

In addition, the previous patch failed testing due to the removal of the
if (empty($ajax_triggering_element)) { use the clicked button }

This was really a result of assumptions that imitation tests were making, rather than a true failure. Since we don't have a way to really test AJAX, poll.test (and I guess field.test) have some bogo-tests that call system/ajax pretending they are the javascript. However, they're not the javascript, so when it does new things (like add the formPath, as it does now) the tests don't reflect that.

After this goes in (hopefully) we'll want to do some things like figuring out better ways to at least write imitation tests.

sun’s picture

#access needs to be checked for each element when recursing down the path to the target element in the foreach.

rfay’s picture

This patch has #access checked at each level, per #24 and #26. Thanks for your reviews.

mattyoung’s picture

subscribe

effulgentsia’s picture

For what it's worth, #27 fully meets with my approval. Adjusting the tests to allow for removal of degradation to $form_state['clicked_button'] might be a nice follow up task, but shouldn't cause this to miss the code-freeze deadline. If it were up to me, I'd call this RTBC, but I don't feel I have the authority to make that call.

RobLoach’s picture

Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community

I'm going to go on a limb and set this to RTBC. It takes care of Sun's request to check the #access. Once this gets in, we have to create follow up issues to make use of it in places where we use AJAX calls on non-button elements. Book module? Autocomplete? Taxonomy? AJAX support on non-button elements is almost a requirement, so pushing this to critical.

sun’s picture

+++ includes/ajax.inc
@@ -208,10 +213,38 @@ function ajax_form_callback() {
+  /**
+   * $triggering_element_path in a simple form might just be 'myselect', which
+   * would mean we should use the element $form['myselect']. However, if
+   * fieldsets are in use, it might be fieldset1/fieldset2/myselect in the case
+   * of a double-nested fieldset, so we would have to find our way to
+   * $form['fieldset1']['fieldset2']['myselect'].
+   * Here we break up the formpath and use it to traverse the form
+   * to find the element.
+   */

We should really use inline comment syntax (// ) here ;)

+++ includes/ajax.inc
@@ -208,10 +213,38 @@ function ajax_form_callback() {
+    if (!isset($form['#access']) || $form['#access'] == TRUE) {
...
+        if (!empty($triggering_element[$key]) &&
+          (!isset($triggering_element[$key]['#access']) || $triggering_element[$key]['#access'] == TRUE)) {

In both cases, we can skip the ' == TRUE'.

We should also remove the strange wrapping in the second condition.

+++ includes/ajax.inc
@@ -208,10 +213,38 @@ function ajax_form_callback() {
+        else {
+          $triggering_element = NULL;
+          break;
+        }

It would be good to explain this else condition in a short inline comment.

+++ misc/ajax.js
@@ -183,6 +183,10 @@ Drupal.ajax.prototype.beforeSubmit = function (form_values, element, options) {
+  form_values.push({name: 'ajax_triggering_element', value: this.formPath});

There should be spaces before the first and after the last object element, i.e. after { and before }.

This review is powered by Dreditor.

RobLoach’s picture

Status: Reviewed & tested by the community » Needs work

Somehow, sun is the only one that notices these small important things. How he does it, I have no clue! Now that we have a little more time through code slush to get this in, it will give us the ability to do these small changes. Thanks, sun.

rfay’s picture

Status: Needs work » Needs review
FileSize
4.06 KB

I think this addresses sun's style comments - @RobLoach maybe we can get this quickly back to RTBC.

Thanks all.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! However, this is a clean-up patch for a completely new API anyway, so it wouldn't necessarily have to be ready today - but it is now. ;)

merlinofchaos’s picture

+  $triggering_element_path = !empty($_POST['ajax_triggering_element']) ? $_POST['ajax_triggering_element'] : NULL;
+  // Destroy field so it can't conflict in validation.
+  unset($_POST['ajax_triggering_element']);

The above will cause a notice if ajax_triggering_element is not set. This should probably be broken into an if and combined.

rfay’s picture

This addresses the concern in #35.

Thanks, all.

rfay’s picture

#559368: Trailing commas in JavaScript cause syntax errors in IE was committed and fixed an IE bug that was also accounted for in this patch, so the patch would no longer apply. This is just a reroll, with no difference except that it applies now that #559368 is in there.

sun’s picture

Issue tags: +API clean-up

Introducing a new tag for feature freeze: API clean-up.

rfay’s picture

Just a quick mention that I'm hoping to get this committed before long, as it will help contrib module updaters enormously, since it dramatically simplifies AJAX.

I'd like to get this committed and then we can update the docs for the various items that changed concerning AJAX - the closely-related #551694: Standardize the return of status messages in AJAX callbacks just went in, and we nearly have the entire feature set complete.

Dries’s picture

This looks good to me, but I don't see this being documented anywhere in the phpDoc or in API doc? It sounds like we need to be a bit more explicit about this feature?

Any existing places in core where we can leverage this?

rfay’s picture

@Dries, this is currently documented in the change documentation http://drupal.org/update/modules/6/7#ahah-now-ajax and in the Form API page, but only for submit elements at this time. What happens is that we no longer have to put in the exception saying "it only works for submit elements".

My plan is to completely redocument the AJAX stuff when it's all landed (especially this piece). Since I've already changed the docs twice I'm holding off for just a bit :-) I will also publish the example module currently at http://randyfay.com/ahah/drupal7 in the example modules.

As far as how we can use this in core, almost everywhere there is simple AJAX form stuff. There is no consistency at all in how this is done in the existing places (book, poll, taxonomy). It would be way cool if we could re-do them with this very simple approach.

There's huge benefit for all users of AJAX, because you no longer have to build a complicated path callback and set it up in hook_menu, and the callback function is really easy, and all set up for you.

sun’s picture

1) We want to incorporate all the information quicksketch mentioned in #17 into the PHPDoc of ajax_form_callback().

2) As mentioned in #18, a suitable candidate for leveraging this could probably be textfield autocomplete. That's a different issue though.

rfay’s picture

#D7AHC: I pledge to do a consolidated docs followup on AJAX as soon as these issues have landed.

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Let's write the documentation for ajax_form_callback() first so I can commit this patch. After that, we can worry about the documentation in the handbook.

rfay’s picture

Status: Needs work » Needs review
FileSize
4.5 KB

@dries, here's the patch with much more explanation on ajax_form_callback().

Thanks!

sun’s picture

We're still missing all the information from #17.

rfay’s picture

Sun, thanks for your incredible scope in keeping up with all these things.

I'm baffled about what you want from #17, which was primarily expressing opinions from a different approach. Please help my befuddlement with a restatement of what you want.

Thanks,
-Randy

sun’s picture

+++ includes/ajax.inc
@@ -196,9 +196,25 @@ function ajax_get_form() {
 /**
- * Menu callback for AJAX callbacks through the #ajax['callback'] Form API property.
+ * Menu callback for AJAX callbacks through the #ajax['callback'] Form API
+ * property.

Please revert - PHPDoc summary needs to be on one line.

+++ includes/ajax.inc
@@ -196,9 +196,25 @@ function ajax_get_form() {
+ * 'ajax_form_callback' is the default value for #ajax['path'], and is
+ * sufficient for many AJAX needs. If #ajax['callback'] is set
+ * and #ajax['path'] is left set to 'ajax_form_callback', then the user
+ * function in #ajax['callback'] will be called with $form and $form_state.

This is already covered in http://api.drupal.org/api/group/ajax/7. However, we should mention ajax_form_callback() as being the default callback there.

+++ includes/ajax.inc
@@ -196,9 +196,25 @@ function ajax_get_form() {
+ * The user code need only return results as either an HTML string which will
+ * replace the appropriate content on the page (#ajax['wrapper'], or  an array
+ * of AJAX framework commands which will be executed.

This is already described in the inline comment of ajax_form_callback().

Instead of adding/repeating the info here, we should add something like that to the end of http://api.drupal.org/api/group/ajax/7, where the ajax commands are already explained.

Please additionally note that http://api.drupal.org/api/function/ajax_command_replace/7 contains a similar piece, so we might just want to move that piece into the group description.

+++ includes/ajax.inc
@@ -196,9 +196,25 @@ function ajax_get_form() {
+ * @see @link ajax_commands AJAX framework commands @endlink

While being there, we want to fix all those '@see @link' instances by replacing "@see" with "See".

This instance, however, is not needed, because we are in the 'ajax' group already, and should therefore be removed. See http://api.drupal.org/api/function/ajax_get_form/7 for the current output.

Hence, the PHPDoc added in this patch should go elsewhere. Instead, we want to provide more API details about the magic we're introducing here.

If I parse #17 correctly:

- Form values can be submitted via AJAX without a submit button. To do this, a form element may set the #ajax property (in the same way like for #type submit/button).

- ajax_form_callback() supports a magic 'ajax_triggering_element' HTTP POST value, which is used to determine the form element that specified #ajax and triggered the AJAX request.

- 'ajax_triggering_element' is automatically appended to the submitted form values by Drupal.ajax.prototype.beforeSubmit().

- For this to happen, Drupal.ajax.prototype.beforeSubmit() depends on the 'formPath' JavaScript setting injected by ajax_process_form().

- Custom implementations of ajax_form_callback() (i.e. #ajax['path'] or #ajax['callback']) may need to re-implement support for this. That said, shouldn't we abstract this functionality into an own function?

rfay’s picture

@sun, thanks. I'll roll again after finishing this discussion.

On #17, I'll be happy to give a technical explanation of what's going on in the phpdoc comment; I think that's what you're after. Perhaps the miscommunication is that that info is in #15, so that's why I never understood you.

All is good on the other PHPDoc changes. If I understand you:

1. Change the phpdoc on ajax_form_callback to a technical description, per #15.
2. Fix the wrap on the first line of comment
3. Change @see @link xxx @endlink to just See xxx throughout the file (please confirm)

sun’s picture

This is almost complete, but needs another pass to complete the PHPDoc of ajax_form_callback() for regular buttons. Could you do that?

I now realized that the entire AJAX framework API documentation is a bit misleading and badly needs a revamp. We should open a separate issue to fix that. It is my belief that APIs like this should be self-documenting on api.drupal.org, and the handbooks on d.o should only point to the API documentation. So the handbooks mainly contain code examples and tutorials, but don't explain the API.

rfay’s picture

Here's the improved ajax_form_callback() PHPDoc. There is really no difference in handling for regular buttons, although there was at the time of #15. I do plan to try to simplify and rebase the Ajax docs as the season progresses.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Awesome! :)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for the hard work all. sun's reviews can blow my mind -- incredible.

Status: Fixed » Closed (fixed)

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

rfay’s picture

Those of you who worked on this should probably take a look at #684846: AJAX triggered by non-submit element fails if any elements are validated.

We forgot about validation when we added non-submit AJAX. validation and submission is done in ajax_form_callback() even when a non-submit element is being processed. And of course it breaks, because the form is not really filled in yet.

Anyway, your attention is requested over on #684846: AJAX triggered by non-submit element fails if any elements are validated.

mdshields’s picture

Definitely found a bug.

I'll start off what examples would look like that would help the general Drupal user (your details above are for those who already dug deep into areas the rest of us have no idea what you are saying - whereas jQuery documentation all over the internet has examples - wish this was like jQuery posts).

  $form['search_feature_in_form'] = array(
    '#type' => 'button',
    '#input' => FALSE,
    '#name' => 'search_feature_in_form',
    '#title' => t('Search Function'),
    '#value' => t('Search Function'),
    '#description' => t(''),
    '#required' => FALSE,	
    '#prefix' => '<div class="form-item"><label></label>',
    '#suffix' => '</div>',
    '#ajax' => array(
      'callback' => 'ajax_search_function_dependent_dropdown_callback',
      'wrapper' => 'something-id-replace',
    ),
    '#attributes' => array('onclick' => 'return false;'),
  );

So, I'm trying to get a simple button to kick off an AJAX request to update options in a selection form field.

First, for some reason, the button automatically renders as a 'submit' button, so I have to set 'onclick' to return false so it actually doesn't submit. Doesn't stop it though, still submits. I am forced to ignore that part. It submits and I have trails of code elsewhere to try it's best to keep the form in place.

This ajax callback will hopefully search for data, compile form select options and build an option list in the form.

I keep getting an error:
"Notice: Undefined index: #ajax in ajax_form_callback() (line 376 of C:\Program Files (x86)\Zend\Apache2\htdocs\proj\includes\ajax.inc)."

When I go into the ajax.inc code, it seems a bit hard coded to derive the #ajax from the 'triggering_element' without any conditionals whether it is a submit button or not.

So, I stumbled on a bug.

Found that if you have TWO buttons in your form. No matter which button you click, the last submit button on the form gets processed by the ajax code :)

So what does that mean.

This means that the hard coded lookup of 'triggering_element' needs to consider the instance when two different submit buttons may have separate triggering_element's.

BTW, I don't usually post this much information unless I found a significant bug. So, please try to recreate and find the source of the error. Build a normal form, then put the submit button that I provided above into the middle of the form. Then trace ajax.inc code. You will see, every time you click the 'Search Function" button will result in ajax.inc inspecting the other submit button to see where the callback is.

rfay’s picture

This is definitely not the place for a support request - adding questions to a long-since-closed issue won't help much.

I recommend http://drupal.stackexchange.com and also the AJAX Example in the Examples project (http://drupal.org/project/examples). It has many starter AJAX usages for you.