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.
Comment | File | Size | Author |
---|---|---|---|
#51 | ajax_callback_556438_14.patch | 10.07 KB | rfay |
#50 | drupal.ajax-triggering-element.patch | 10.07 KB | sun |
#45 | ajax_callback_556438_13.patch | 4.5 KB | rfay |
#37 | ajax_callback_556438_12.patch | 3.85 KB | rfay |
#36 | ajax_callback_556438_11.patch | 4.07 KB | rfay |
Comments
Comment #2
rfayThis 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.
Comment #3
rfayHere'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:
Comment #4
rfayComment #5
quicksketchThis 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.
Comment #6
sunsubscribing
Comment #7
rfayThanks 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.
Comment #9
rfayThe 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.
Comment #10
rfayI 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...
Comment #11
rfayThis 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.
Comment #12
sunI 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.
Comment #13
sunI 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.
Comment #14
rfay@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!
Comment #15
rfayPer 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:
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.
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedI 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()?
Comment #17
quicksketchI'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().
Comment #18
sunSomehow, 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.
Comment #19
sun.core CreditAttribution: sun.core commentedComment #20
effulgentsia CreditAttribution: effulgentsia commentedHere'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).
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedOh, and @quicksketch, in javascript, arrays are objects, so are always effectively passed by reference.
Comment #22
rfay@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:
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.
Comment #24
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis *needs* to check for #access:
Comment #25
rfayPer @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.
Comment #26
sun#access needs to be checked for each element when recursing down the path to the target element in the foreach.
Comment #27
rfayThis patch has #access checked at each level, per #24 and #26. Thanks for your reviews.
Comment #28
mattyoung CreditAttribution: mattyoung commentedsubscribe
Comment #29
effulgentsia CreditAttribution: effulgentsia commentedFor 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.
Comment #30
RobLoachI'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.
Comment #31
sunWe should really use inline comment syntax (// ) here ;)
In both cases, we can skip the ' == TRUE'.
We should also remove the strange wrapping in the second condition.
It would be good to explain this else condition in a short inline comment.
There should be spaces before the first and after the last object element, i.e. after { and before }.
This review is powered by Dreditor.
Comment #32
RobLoachSomehow, 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.
Comment #33
rfayI think this addresses sun's style comments - @RobLoach maybe we can get this quickly back to RTBC.
Thanks all.
Comment #34
sunThank 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. ;)
Comment #35
merlinofchaos CreditAttribution: merlinofchaos commentedThe above will cause a notice if ajax_triggering_element is not set. This should probably be broken into an if and combined.
Comment #36
rfayThis addresses the concern in #35.
Thanks, all.
Comment #37
rfay#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.
Comment #38
sunIntroducing a new tag for feature freeze: API clean-up.
Comment #39
rfayJust 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.
Comment #40
Dries CreditAttribution: Dries commentedThis 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?
Comment #41
rfay@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.
Comment #42
sun1) 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.
Comment #43
rfay#D7AHC: I pledge to do a consolidated docs followup on AJAX as soon as these issues have landed.
Comment #44
Dries CreditAttribution: Dries commentedLet'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.
Comment #45
rfay@dries, here's the patch with much more explanation on ajax_form_callback().
Thanks!
Comment #46
sunWe're still missing all the information from #17.
Comment #47
rfaySun, 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
Comment #48
sunPlease revert - PHPDoc summary needs to be on one line.
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.
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.
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?
Comment #49
rfay@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)
Comment #50
sunThis 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.
Comment #51
rfayHere'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.
Comment #52
sunAwesome! :)
Comment #53
Dries CreditAttribution: Dries commentedThanks for the hard work all. sun's reviews can blow my mind -- incredible.
Comment #55
rfayThose 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.
Comment #56
mdshields CreditAttribution: mdshields commentedDefinitely 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).
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.
Comment #57
rfayThis 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.