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)
Comment | File | Size | Author |
---|---|---|---|
#14 | drupal_json_1.patch | 4.66 KB | kkaefer |
#1 | drupal_json_0.patch | 3.82 KB | kkaefer |
drupal_json.patch | 4.49 KB | kkaefer | |
Comments
Comment #1
kkaefer CreditAttribution: kkaefer commentedMoshe 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.
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedComment #4
Steven CreditAttribution: Steven commentedWe 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.
Comment #5
robmilne CreditAttribution: robmilne commentedI'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() :
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.
Comment #6
kkaefer CreditAttribution: kkaefer commentedI'd recommend using lastIndexOf() instead of indexOf().
Comment #7
robmilne CreditAttribution: robmilne commentedThat won't work because the upload response (when devel enabled) has the following format:
The inline function of the ready method uses the same curly braces as the json.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedFYI, devel does not append any javascript anymore. it reverted back to a plain old table right after [/html]
Comment #9
robmilne CreditAttribution: robmilne commentedOK, 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.
Comment #10
moshe weitzman CreditAttribution: moshe weitzman commentedi'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?
Comment #11
nedjoThis 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()
:Comment #12
kkaefer CreditAttribution: kkaefer commentednedjo, see my comment #1.
Comment #13
yched CreditAttribution: yched commentedbumping 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 ?
Comment #14
kkaefer CreditAttribution: kkaefer commentedAdded nedjo's hint to upload.module. I consider this one ready.
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedtested - works as designed.
Comment #16
Steven CreditAttribution: Steven commentedSince 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.
Comment #17
yched CreditAttribution: yched commentedIt 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 ?
Comment #18
(not verified) CreditAttribution: commented