I learned that outputting JSON is not always straightforward: For example, when devel.module is enabled, it could add a query log to the request, even if you exit right after your print drupal_to_js(...). Moshe told me that changing the Content-type in the HTTP header to text/javascript solves the problem.

Another issue with JSON output is that hook_exit is not called when you exit right in the menu callback function. Therefore I introduced a new return constant in the menu system: MENU_NO_THEME. Return this value whenever you take care of the page content yourself and don’t want Drupal to add the theme.

The new drupal_json($var = NULL) takes care of both issues: simply return drupal_json($json) in your menu callback and you’re done. It sets the header appropriately, converts the variable to JSON and returns MENU_NO_THEME.

(note that autocomplete is currently broken in 6-dev due to the menu patch)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kkaefer’s picture

FileSize
3.82 KB

Moshe is right on IRC: We don't need a new constant. index.php doesn't theme if the return value of the menu callback is not a string.

moshe weitzman’s picture

Title: Don't bypass the menu system for JSON/non-themed output » Always output text/javascript in http headers when we are returning a JSON object

I just saw the same problem in update.php?op=do_update where we return % complete info. any chance you can patch that too. this patch is definately needed.

moshe weitzman’s picture

Status: Active » Needs review
Steven’s picture

We should document all places that don't use this (but still output JSON) and why. The only one I know of is upload.module: because it uses an (invisible) iframe's contents to return the JSON, and text/javascript makes those contents inaccessible in some browsers.

robmilne’s picture

I've experimented with this patch to common.inc and it has fixed the WebFM module's incompatibility with Devel. As Steven mentioned, the new func doesn't work with the upload iframe. Unfortunately the upload still breaks when Devel is enabled (I just tested with both WebFM and upload modules) even with the original drupal_to_js call. The '$(document).ready(function()...' from Devel still foobars the call to Drupal.parseJson() inside Drupal.redirectFormButton() so I fixed with this minor change to Drupal.redirectFormButton() :

//        response = Drupal.parseJson(response);
        response = Drupal.parseJson(response.substring(0, response.indexOf('}') + 1));

I'm not happy about adding this bit of overhead but at least the upload works fine now. Errors are still generated but at least functionality is restored.

kkaefer’s picture

I'd recommend using lastIndexOf() instead of indexOf().

robmilne’s picture

That won't work because the upload response (when devel enabled) has the following format:

{ "status": true, "data": "..." }<script type="text/javascript">$(document).ready(function() { $("body").append("...");});</script>

The inline function of the ready method uses the same curly braces as the json.

moshe weitzman’s picture

FYI, devel does not append any javascript anymore. it reverted back to a plain old table right after [/html]

robmilne’s picture

OK, I've updated to the latest devel release but the upload is still broken when devel is enabled. Firebug says "missing ) in parenthetical" for the eval inside Drupal.parseJson.

moshe weitzman’s picture

i'm a bit lost now. lets keep devel out of this. it will learn to stay out of the way of the upload callback. what do we need to do to get this in? is it ready now?

nedjo’s picture

Status: Needs review » Needs work

This is a simple improvement that's good for consistency. The header should indeed be sent. The fix to premature exiting is also a good idea.

I don't completely follow the pont of returning a new menu constant. I expect a given constant to be handled somehow in index.php, like the other ones are. Is this any different than returning a zero-length string?

It looks like we need to document the skipping of upload's methods. I suggest the following in upload_js():


  // We send the updated file attachments form.
  // Don't call drupal_json(). Upload.js uses an iframe, and
  // the header output by drupal_json() causes problems in some browsers.
  print drupal_to_js(array('status' => TRUE, 'data' => $output));

kkaefer’s picture

nedjo, see my comment #1.

yched’s picture

bumping moshe's question in #10 :
'what do we need to do to get this in? is it ready now?'

having devel incompatible with ajax upload is sure annoying, and if I get this right, this might be the fix ?

kkaefer’s picture

Status: Needs work » Needs review
FileSize
4.66 KB

Added nedjo's hint to upload.module. I consider this one ready.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

tested - works as designed.

Steven’s picture

Status: Reviewed & tested by the community » Fixed

Since drupal_json() doesn't return anything, the use of return drupal_json($matches); in profile.module is not needed.

I removed it and committed to HEAD.

yched’s picture

It seems upload/module was left as an exception that cannot use drupal_json and therefore outputs no headers.
Moshe : where does that leave the 'devel breaks ajax upload' bug ?

Anonymous’s picture

Status: Fixed » Closed (fixed)