This patch is a simplification of the current #ahah property. I'm using poll.module to introduce the functionality, but afterwards we should do the same thing to book, upload, and other modules that use #ahah. Basically this makes the AHAH callbacks much, much simpler. Instead of having to create an item in hook_menu() and specifying a #ahah['path'], you can now shortcut this and simply specify #ahah['callback'], which is a function name.
System module provides a global callback at system/ahah, which does all the crazy form rendering stuff and calls the callback function with ($form, $form_state) parameters. Just take a look, you'll see it'll save even more code from the AHAH callbacks. Once this is applied we'll be able to remove ~10 lines of code from every AHAH callback in core, which previously just duplicated all this code over and over.
Note, this patch overlaps a bug introduced in #331708: poll_choice_js uses FAPI2 way of thought. Instead of the last item in the poll choices being faded in, the entire set of options is faded in. This patch corrects that behavior also.
Comment | File | Size | Author |
---|---|---|---|
#18 | form_ahah_callback_security.patch | 1.06 KB | Frando |
#16 | form_ahah_callback_more.patch | 820 bytes | chx |
#10 | drupal_ahah_callback.patch | 7.07 KB | quicksketch |
#5 | drupal_ahah_callback.patch | 7.07 KB | quicksketch |
#3 | drupal_ahah_callback.patch | 7.94 KB | quicksketch |
Comments
Comment #2
quicksketchTests needed to be updated to use the "system/ahah" callback.
Comment #3
quicksketchHere's an implementation per chx's request with a cleaner approach to finding the new choices on the form to fade in with the "ahah-new-content" class.
Regarding this line:
$n = $_GET['q'] == 'system/ahah' ? 1 : 5;
It's very ugly checking the system path directly. This current approach is already used in poll.module. I'm planning on submitting a different patch to correct this, but it will probably take more effort than this patch (which seems like a huge, easy win).
Comment #4
chx CreditAttribution: chx commentedPlease remove the poll classes from this patch, they do not belong here.
Comment #5
quicksketchOkay, here's a super-simplified patch that leaves out the fix to poll module's incorrect handling of new choices. We can fix that in a followup. The only changes here are the addition of #ahah['callback'] and a universal menu handler at system/ahah.
Comment #7
quicksketchArg. Don't listen to testing bot, file naming has nothing to do with this poll patch.
Comment #8
chx CreditAttribution: chx commentedNice, needed and already almost documented, the AHAH handbook page promised that we will create such a function :) awesome! thanks.
Comment #10
quicksketchTry this, testing bot! (Same patch as #5).
Comment #11
webchickMy main concern with this patch is that there's only an implementation for poll, not for other areas such as book and upload modules. Nate pointed out that in order to convert these as well, they need to be re-done in the FAPI3 style (using drupal_rebuild_form() which lets the form rebuild without re-rendering), and doing this for both upload and book would've bloated the patch. So, committed this as an interim first step. Because it changes the AHAH callback for Poll module, it's covered by the existing tests.
Committed to HEAD! Upgrade docs, please. :)
Comment #12
drewish CreditAttribution: drewish commentedsubscribing
Comment #13
quicksketchFormAPI Documentation at api.drupal.org updated.
http://drupal.org/cvs?commit=169744
Comment #15
Frando CreditAttribution: Frando commentedHave the security implications of this patch been considered?
The system/ahah callback is "open to everyone", in contrast to other menu callbacks that were used previously.
If I am correct, this should be highlighted in the documentation.
We do load the form based on its build ID. I am not sure what would happen if I would perform a POST request on system/ahah with an invalid form build ID, but with a valid form ID.
If I am talking nonsense (which is perfectly possible ;) please just reclose the issue. This just spotted my mind, I haven't fully though this out.
Comment #16
chx CreditAttribution: chx commented@Frando, valid question. A brief look at the functions involved reveals that
$form
will become a big fat NULL if you pass in an invalid build_id regardless of the form_id. There will be nothing in$form_state['clicked_button']
and calling the callback finally fatals out. However thedrupal_rebuild_form
runs and that's not good -- although your form generating function should not do anything but read still it's not nice and too easy not to fix.Added a drupal_function_exists while in there. (yes, meow. a little bit.)Comment #17
Dries CreditAttribution: Dries commentedCan we elaborate on
+ // Invalid form_build_id posted, likely a hacking attempt. Just do nothing.
in the code comments. I think it would be good to explain what happened so people can understand.Comment #18
Frando CreditAttribution: Frando commentedI like the fix proposed by chx.
Expanded the documentation on why we exit if the form_build_id is invalid. No other changes.
Comment #19
Frando CreditAttribution: Frando commentedComment #20
desbeers CreditAttribution: desbeers commentedThe poll.module is updated now but the book and upload modules not yet.
I tried to update the book as a proper fix for #322344: (notice) Form improvements from Views but the API reference told me that 'the #ahah['callback'] can only be used on elements with a #type of submit or button because it needs to call that button's #submit and #validate functions'.
The book is using a 'select' so I'm not able to update it with this new simplified AHAH callback as it is right now. I'm giving it a try to see if I can make the callback handle all kinds of elements but FAPI is tough stuff...
Comment #21
Pasqualle2 simple checks, this is ready..
Comment #22
Dries CreditAttribution: Dries commentedCommitted. Thanks for the documentation improvements.
Comment #23
sunI see that this patch introduced 'system/ahah' as a generic JS callback URL. However, with http://drupal.org/project/js, I tried to
a) reserve the namespace (potentially for core)
b) route all JS callbacks to a prefix starting with js/
c) allow modules to implement hook_js() [may or may not be needed in HEAD] to define what (and which includes/modules) they actually need to load (performance)
By doing this, and applying some hacks to well-known modules ;), I was able to minify the JS page callback loading times on large-scale sites (requiring >100 modules) and achieve a performance gain that makes people believe "This is not Drupal."
Comment #25
liquixis CreditAttribution: liquixis commentedsubscribing