Unify page, AJAX 'path', and AJAX 'callback' callbacks

.

Subscribing

I'm sorry. I really really love this patch. But I also really doubt that we find a valid, sustainable, working, and approved solution within the next 5 days. :-/

Feel free to try. Pulling some people from IRC into this issue might help, too.

Ok, I was dubious when I started looking at this patch. After reading the code, I think I really like this. I have two issues.

1) I normally use URL switching mechanism to do javascript degradation. i.e, you visit panels/ajax/do-something or panels/nojs/do-something and my code checks the argument and I use a menu _load() function to do the test. In this system I believe I would have to have 2 different menu entries for this.

2) Similar, other systems like to use ?js=1 as a query parameter to say that this is an ajax call. In that system, you can't do it with menu entries.

Both of these could be solved with a menu_set_delivery_callback() function. We could also theoretically introduce a standard where a query variable could *automatically* change the delivery callback. In that case, I think the order of precedence would be:

a) Was a callback set with menu_set_delivery_callback() -- if so, use that.
b) Was a type requested with a query string? -- note, we'd obviously have to map identifiers to functions for safety here.
c) Was a delivery callback set from the menu?
d) Fallback to normal HTML delivery.

3) drupal_access_denied() and drupal_not_found() should be modified respect the delivery callback. This goes well with the comment you had to @todo about error handling in either case. For both cases, ajax_command_error(t('Page not found')) and ajax_command_error(t('Access denied')) are certainly acceptable.

We already have a pretty good fallback for "I have HTML, you want it as AJAX, package it up and send it". It does require that the client know what to do with it, but I think we're well prepared for that.

This looks really good now. My only problem is with drupal_alter('menu_router_item', $router_item, $path); this is not just an ordinary menu router item but the active menu handler item. So what about drupal_alter('menu_active_handler', $router_item, $path);?

Since we introduced the AJAX framework in D7, I think this can be treated as a clean-up - hence, applying the critical tag.

Conceptually, it should be no surprise that I support this. :-) The specific implementation here is a bit ugly and does about a quarter of what it could potentially do, but as effulgentsia has noted we don't really have time for D7 to do all of the cool things that this could do. So I'm +1 in principle on this approach.

I've not gone over the code in minute detail, but some comments from just reading the patch:

- We shouldn't be hard coding the "html page" version of drupal_not_found() et al. Why do those have to be a special case instead of sub-call functions like everything else?

$delivery_callback = (isset($router_item) && $router_item && !empty($router_item['delivery_callback'])) ? $router_item['delivery_callback'] : 'drupal_deliver_html_page';

That line in menu_execute_active_handler() can easily be simplified by pre-choosing the delivery callback in menu rebuild. Just make that function name the default if one is not set, and then the DB has an accurate callback record for every router item.

OK, the additional alter hooks are just sexy. :-)

That's all I have time for right now, I'm afraid.

I apologize that I haven't had time to do a real review of this due to "real life" issues using up all my time.

However, it looks to me like there is a consensus that this is a great thing for Drupal's architecture.

Let's go ahead and take the risk and get it in.

There are times that "doing the right thing" pays off enormously, so many times over, that it's worth doing even when we're very conservative, and in this case, even at the very end of the cycle.

My 2 cents.

This is RTBC for me - aside from the following comment tweaks: (please mark as RTBC after fixing these)

+++ includes/ajax.inc	15 Oct 2009 04:41:40 -0000
@@ -291,30 +291,56 @@ function ajax_form_callback() {
+ * Package and send the result of a page callback to the browser as an AJAX
+ * response.

(and elsewhere) PHPDoc summary shouldn't wrap at 80 chars. Just remove the newline.

+++ includes/ajax.inc	15 Oct 2009 04:41:40 -0000
@@ -291,30 +291,56 @@ function ajax_form_callback() {
+        break;
+      case MENU_ACCESS_DENIED:
...
+        break;
+      case MENU_SITE_OFFLINE:

(and elsewhere) There should be blank line between case statements of a switch (as long as you don't fall-through).

+++ includes/common.inc	15 Oct 2009 04:41:40 -0000
@@ -665,75 +665,45 @@ function drupal_goto($path = '', array $
+ * Whenever possible, write page callback functions to return MENU_SITE_OFFLINE
+ * instead of calling drupal_site_offline(). However, until Drupal implements
+ * exception handling, if you're writing a function that is called by a
+ * page callback function, and returning MENU_SITE_OFFLINE from your function
+ * would not chain up back to menu_execute_active_handler(), then calling
+ * drupal_site_offline() is appropriate.
  */
 function drupal_site_offline() {

(and elsewhere)

Please re-phrase into 3rd person here. So instead of referring to "you", we refer to "page callback functions", or, "the/a module".

Also, we do not talk about the future, so let's drop that part about exception handling ;)

+++ includes/common.inc	15 Oct 2009 04:41:40 -0000
@@ -2592,6 +2562,155 @@ function l($text, $path, array $options 
+ * When a user requests a page, index.php calls menu_execute_active_handler()
+ * which calls the 'page callback' function registered in hook_menu(). The page
+ * callback function can return NULL (to indicate no content), an integer
+ * menu status constant to indicate an error condition, a string of HTML
+ * content, or a renderable array of content. It is recommended wherever

This should be formatted as PHPDoc list.

+++ includes/common.inc	15 Oct 2009 04:41:40 -0000
@@ -2592,6 +2562,155 @@ function l($text, $path, array $options 
+ * For example, the same page callback function can be used for an HTML
+ * version of the page and an AJAX version of the page. The page callback
+ * function just needs to decide what content is to be returned and the
+ * delivery callback function will send it as an HTML page or an AJAX
+ * response, as appropriate.

How can a page callback decide? From the previous paragraph, I understand that the page callback is invoked first, returns its results, and meah() determines the delivery callback. My missing link is how my page callback can define/override this delivery callback, because this paragraph states I can.

+++ includes/common.inc	15 Oct 2009 04:41:40 -0000
@@ -2592,6 +2562,155 @@ function l($text, $path, array $options 
+ * @param $default_delivery_callback
...
+ *   to be appropriate for the page request. If not given, it is determined
+ *   from the menu router information of the current page. In either case,
+ *   modules have a final chance to alter which delivery function is called. 

Outdated?

+++ includes/common.inc	15 Oct 2009 04:41:40 -0000
@@ -2592,6 +2562,155 @@ function l($text, $path, array $options 
+  // Give modules a final chance to alter the delivery callback used. This is
+  // for modules that need to decide which delivery callback to use based on
+  // information made available during page callback execution and for pages
+  // without router items.
+  drupal_alter('page_delivery_callback', $delivery_callback);

Now I think I understand - so the PHPDoc description above should just be appended by a

@see hook_page_delivery_callback_alter()

+++ includes/common.inc	15 Oct 2009 04:41:40 -0000
@@ -2592,6 +2562,155 @@ function l($text, $path, array $options 
+  if (function_exists($delivery_callback)) {
+    $delivery_callback($page_callback_result);
+  }

What's the result if the function doesn't exist?

If it's intentional to return NULL, let's append a comment at the end to clarify so (and perhaps why).

+++ includes/menu.inc	15 Oct 2009 04:41:41 -0000
@@ -401,29 +401,52 @@ function menu_get_item($path = NULL, $ro
+ * @param $deliver
+ *   A boolean. If set to TRUE, the page is sent to the browser using the
+ *   appropriate delivery callback. If set to FALSE, the result of the page
+ *   callback is simply returned to the client code.
  */
-function menu_execute_active_handler($path = NULL) {
+function menu_execute_active_handler($path = NULL, $deliver = TRUE) {

@param description should start with:

(optional) A boolean to indicate whether the content should be sent to the browser using the appropriate delivery callback (TRUE) or whether to return the result to the caller (FALSE).

+++ includes/menu.inc	15 Oct 2009 04:41:41 -0000
@@ -401,29 +401,52 @@ function menu_get_item($path = NULL, $ro
+      drupal_alter('menu_active_handler', $router_item, $path);

Short inline comment about the purpose of this alter?

+++ modules/system/system.module	15 Oct 2009 04:41:42 -0000
@@ -307,6 +307,21 @@ function system_element_info() {
+  $types['ajax_commands'] = array(
+    '#ajax_commands' => array(),
+    // AJAX commands are used by AJAX callbacks to return a set of javascript
+    // commands for the browser to execute. When using the ajax_deliver()
+    // delivery callback, these get rendered by ajax_render(). It's possible
+    // (though not necessarily useful) to create a page callback that returns
+    // AJAX commands that are delivered to the browser using
+    // drupal_deliver_html_page(). In this case, this element would be rendered
+    // using #theme and #theme_wrappers functions as is the case with other
+    // renderable elements. By default, we don't want AJAX commands being
+    // rendered in the context of a normal HTML page, so we don't provide
+    // defaults for #theme or #theme_wrappers. However, modules can set these
+    // properties if they would like (for example, to provide a debugging page
+    // that displays rather than executes AJAX commands).
+  );

This comment should be above the type definition, not within the array.

Should also be shortened... probably all up to (and including) "In this case" can more or less go, because that's documented in ajax.inc. Here, we should explain renderable array internals/tricks only.

This review is powered by Dreditor.

Better title?

I have to let this slip in because, well, this is exactly the architecture that I outlined 3-4 years ago!

Unfortunately, the patch no longer replies so it needs a reroll. I did a review so it should be RTBC after a reroll.

This is hopefully a straight re-roll, setting CNR. Please do not commit until tests are green.

http://testing.drupal.org/pifr/file/1/15236

Alright, committed! Great job all.

Nice patch!

We should emit the standard http header only for http delivery. This helps command line scripts a lot. 1 line patch attached.

What do folks think about turning drupal_serve_page_from_cache() into a delivery callback? Thats what does the http headers and such for cached pages. Not sure how we go about terminating the request flow and immediately delivering the page. Still use exit?

I can reproduce those test failures, but have no clue how I've triggerred that. Anyone hav a clue. Failure is on line 139 of openid.test?

Nice work, effulgentsia.

Mmm, that feels like a hack.

Whats a hack? We used to unconditionally set a text/html http header very early in page processing. We did that for all requests including CLI requests and XML requests and so on. Thats hackish. This patch moves that to the html delivery callback and does so in a safe way. Thats a significant improvement, no?

Hey all,
I've added a documentation task which could really use the eyes of the people involved in this thread: #610068: Document AJAX no-js and use-ajax
thanks!

And I hijacked katbailey's thread for a general to-do list and discussion of Ajax documentation. #610068: Document AJAX no-js and use-ajax.

Fixing upgrade path, previous patch still required.

Please ignore tiny kitten.

I committed #49 to fix the upgrade path. Haven't had a chance to read the rest of this issue yet, sorry.

Catch moved his upgrade path fixing efforts to #613238: Add missing columns and tables required for upgrade. Sorry folks. Carry on!

Here's a re-upload of #43 so testing bot stops complaining.

I'm not sure why Dries feels uneasy about this fix.

The only thing that makes me feel a bit uneasy is the NULL condition. Otherwise, this looks ready to go.

I asked Dries to take another look at this.

Committed to CVS HEAD.

I dislike that ajax_render() issues an exit. It sucks for unit testing or reusing anything that ends up calling it. But I decided not to deal with that in this issue. However, this patch landing would lend itself to a fairly straightforward follow up issue to change ajax_render() to not exit. That would require changing all AJAX callbacks to use the ajax_deliver() delivery callback, but that's not a big deal to do, and is arguably a good idea anyway.

Hasn't happenned yet, AFACT. Is there an issue for it? Lets unify, people. Anyone up for rolling the patch?

You may be interested in #838408: Remove hook_menu_active_handler_alter if you participated here, since this is where it was introduced.