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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
7.54 KB

Tests needed to be updated to use the "system/ahah" callback.

quicksketch’s picture

FileSize
7.94 KB

Here'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).

chx’s picture

Status: Needs review » Needs work

Please remove the poll classes from this patch, they do not belong here.

quicksketch’s picture

Status: Needs work » Needs review
FileSize
7.07 KB

Okay, 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

quicksketch’s picture

Status: Needs work » Needs review

Arg. Don't listen to testing bot, file naming has nothing to do with this poll patch.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Nice, needed and already almost documented, the AHAH handbook page promised that we will create such a function :) awesome! thanks.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

quicksketch’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
7.07 KB

Try this, testing bot! (Same patch as #5).

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

My 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. :)

drewish’s picture

subscribing

quicksketch’s picture

Status: Needs work » Fixed

FormAPI Documentation at api.drupal.org updated.
http://drupal.org/cvs?commit=169744

Status: Fixed » Closed (fixed)

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

Frando’s picture

Status: Closed (fixed) » Active

Have 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.

chx’s picture

Status: Active » Needs review
FileSize
820 bytes

@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 the drupal_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.)

Dries’s picture

Status: Needs review » Needs work

Can 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.

Frando’s picture

Status: Needs work » Needs review
FileSize
1.06 KB

I like the fix proposed by chx.
Expanded the documentation on why we exit if the form_build_id is invalid. No other changes.

+    // If $form cannot be loaded from the cache, the form_build_id in $_POST must
+    // be invalid, which means that someone performed a POST request onto
+    // system/ahah without actually viewing the concerned form in the browser.
+    // This is likely a hacking attempt as it never happens under normal
+    // circumstances, so we just do nothing.
Frando’s picture

Title: Simplify AHAH Callbacks #ahah['callback'] (Poll implementation) » Security fix for simplified AHAH callbacks
Category: task » bug
desbeers’s picture

The 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...

Pasqualle’s picture

Status: Needs review » Reviewed & tested by the community

2 simple checks, this is ready..

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed. Thanks for the documentation improvements.

sun’s picture

I 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."

Status: Fixed » Closed (fixed)

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

liquixis’s picture

subscribing