Make /batch use proper response objects

By default (i.e., without additional drupal_alter()), a single menu router item only has a single delivery callback. Basically meaning that one router path is expected to result in a certain output.

Drupal core implements two delivery callbacks as of now:

1) HTML (page)
2) AJAX (JSON command array)

The /batch path and page callback http://api.drupal.org/api/function/_batch_page/7 is somewhat special, as it

a) Outputs the initial Batch HTML page
http://api.drupal.org/api/function/_batch_start/7

b) If the client has the has_js cookie, seems to respond with JSON batch status data for progress.js
http://api.drupal.org/api/function/_batch_progress_page_js/7
http://api.drupal.org/api/function/_batch_do/7

c) If the client has no has_js cookie, seems to return a full HTML page
http://api.drupal.org/api/function/_batch_progress_page_nojs/7

d) Outputs the finished Batch HTML page
http://api.drupal.org/api/function/_batch_finished/7

Normally, one would likely say that a clear separation would use two different router items:

batch/page - All HTML pages that are output, using the regular HTML delivery callback.
batch/json - JSON output for the has_js cookie case.

"/page" only added for clarity. Technically doable would also be:

batch
batch/json

Note that batch/json would need a new built-in delivery callback for plain JSON, since http://api.drupal.org/api/function/ajax_deliver/7 relies on AJAX framework and command structures.

Thinking through this, I think that the current patch is all we can do for D7.

The primary problem is that current HEAD

1. does not use a special delivery callback for the /batch path
2. so it uses the default delivery callback drupal_deliver_html_page()
3. drupal_deliver_html_page() is supposed to return HTML
4. but _batch_page() returns sometimes HTML, sometimes JSON (Note: plain JSON, not AJAX commands)
5. which ultimately breaks modules that rightfully assume they can dynamically deliver HTML responses in a different way/format
6. and also invalidates existing hook_page_delivery_callback_alter() API docs (as shown in the OP)

The idea is to do a stopgap fix by adding a dedicated/special delivery callback for the /batch path. This delivery callback can return mixed data formats to its liking without breaking and invalidating the unique purpose of dedicated standard delivery callbacks.

In D8, we can clean this up properly by adding a standard delivery callback for plain JSON and ideally also rewrite the Batch API/UI to remove the current arbitration that happens in _batch_page().

Thanks!

The only thing we do at this point is fix bugs that get us to release.

Right, that's what this patch does. I had to learn the hard way that my module - which is strictly following API docs and examples - unintentionally breaks Batch API's processing.

It took me a couple of hours to figure out what on earth broke all kind of batches I tried to run (tests, updates, etc; in short: all batches). All I got was a JavaScript alert stating "An error occurred.", followed by a dump of some response garbage.

Since debugging is close to impossible, I had to disable all modules one after the other to figure out which code or module is guilty for that. I totally did not expect my module to be guilty, because as mentioned, its hook implementation is 1:1 copypasted from the API documentation.

And it turns out that both my module as well as the API documentation is correct. Batch API's page callback is what is not correct, because it sometimes returns plain JSON within a HTML context; i.e., without ending the request. In turn, the JSON page callback result is handed over to the default delivery callback, drupal_deliver_html_page(), which, as the name implies, is expected to return a renderable array or a string to output as HTML.

Handing a plain JSON data array to drupal_deliver_html_page() is technically and conceptually invalid.

Lastly, we already discussed the "proper fix" and "nice improvement" earlier in this issue, but that is definitely D8 material at this point, as it would require to rewrite most of batch.inc.

Thus, this patch is a stopgap fix to make Batch API's page callback not hi-jack the intended purpose and expected API behavior of page callbacks and delivery callbacks.

The entire concept of delivery callbacks does not exist in D6, it is new in D7.

yched's summary in #12 is correct, especially the part about breaking the "contract" through having a delivery callback, which is intended to deliver HTML, returning JSON.

But of course, this change technically must be considered as an API change. Even though I don't know of any code or module that is trying to re-use any functionality of the /batch menu router item resp. page callback. The only actual API change is that the system_batch_page() callback no longer exists.

AFAICS, the only other way out would be to preemptively end the page request in _batch_do(); i.e., right after the JSON has been generated, so the entire regular page delivery mechanism is not even invoked in the first place. That, however, should be considered as a hack, disallowing any further request modifications by contributed modules, as the request is ended preemptively.

I hope that the additional reasoning provided in #14 sufficiently explains the need for this patch.

As already mentioned in there, we can also squeeze a drupal_exit() right after the JSON output is generated, but that would be a hacky resolution compared to the existing patch.

Re-rolled against HEAD - including HttpKernel changes.

So if I get this right, then attached patch should actually work.

ae71db1 Fixed JavaScript progress callback response contains Ajax settings.

I'd like to even go one step further, and also change the router item definition for /batch to define proper callbacks and arguments.

@Crell: What's the new/post-kernel way to handle the #show_messages flag that exists for theme_page()?

Batch API uses that to mute any messages that may be set during the operations, and to instead display them at the end of the batch run.

d6360da - #929506: Menu router does not load the 'file' before attempting to invoke callbacks, and fails to find/register them during building
47225ec Replaced custom batch loading with proper router item definition.
a3a6837 Fixed install_run_task() to properly handle _batch_page() return values.

The installer works flawlessly with attached patch - if this fails the testbot again, then I'm not really sure what's going on with the testbot.

Coolio, thanks @marvil07!

I guess that leaves us with the #show_messages issue only.

In a progressive batch, Batch API renders the intermediate pages being output for batch operation sets/steps and enforces no messages to be shown for those (as they wouldn't be noticeable, since the page immediately redirects), so that any messages being set are collected during the batch processing, and only output on the final batch finished page.

To experience this behavior, I think one has to disable JavaScript and then execute any progressive batch through the UI; e.g., installing Drupal, importing translations, running update.php, or possibly the most simple, checking for module updates with Update module. (not sure whether there is a simpler one) — OH. Running tests! :-D

However, you'd have to manually implant a call to drupal_set_message() into the batch operation step callback(s), so as to actually make sure that there are messages that should not be output until the batch has finished.

If that still works with this patch, then I think we're ready to go.