Currently, AJAX requests end with ajax_render() which calls exit, which means drupal_page_footer() isn't called. This is understandable as there's page caching code we might not want running. And a hook_exit() that prints something would mess up the JSON response. But it's still bad, because there's legitimate stuff a hook_exit() may want to do even for AJAX requests. Patch coming momentarily.
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | drupal-exit-properly-601570-13.patch | 10.35 KB | effulgentsia |
| #12 | drupal-exit-properly-601570-12.patch | 10.07 KB | effulgentsia |
| #8 | ajax-exit-properly-601570-8.patch | 1.77 KB | effulgentsia |
| #6 | ajax-exit-properly-601570-6.patch | 891 bytes | effulgentsia |
| #1 | ajax-exit-properly-601570-1.patch | 1.57 KB | effulgentsia |
Comments
Comment #1
effulgentsia commentedUnfortunately, the only solution I came up with that made sense to me required adding a parameter to hook_exit(), making this an API cleanup and not only a bug-fix.
Note: my preferred solution would be to not have ajax_render() exit at all, but #599804: Unify page, AJAX 'path', and AJAX 'callback' callbacks needs to land first to make that happen in a clean way. I'll be happy to re-roll this patch accordingly if that other one lands first.
Comment #2
effulgentsia commentedwith the status change this time.
Comment #3
chx commentedThere is a simpler solution: don't print anything in hook_exit that's not what it is for and document so. If you are so bent on adding something to the absolute end of a page, just alter the page array or use hook_page_build and slap a huge #weight on it.
Comment #4
effulgentsia commentedhook_exit() is the only hook that runs for cached pages. Is there any legitimate reason for something to append html at the end of cached pages (e.g., performance numbers)?
Comment #5
effulgentsia commentedActually, anything hook_exit() prints is placed AFTER the closing html tag. Is that even valid? Anyone else agree with chx that we should consider hook_exit() exclusively non-printing? I do agree, but I don't usually write modules that implement hook_exit(), so maybe I'm missing something.
Comment #6
effulgentsia commentedOr, how about we side-step the question and just use ob_start()/ob_end_clean()? If this makes sense,
1) is it still subject to the 10/15 deadline?
2) should i remove the "API cleanup" tag?
Comment #7
moshe weitzman commentedi think it is cheating to print after end of HTML tag. devel does that in order to show the session close queries but i will be forego that feature in d7 and putting self before in before [/BODY].
Comment #8
effulgentsia commentedMoshe, chx, and I agree that we should require module developers to NEVER print in a hook_exit(). This patch is updated accordingly. If community sentiment moves the other way on this, I can revert to #6. I'm not at all clear on whether a decision to change the rules for whether hook_exit() may print or not qualifies as an API change.
Comment #9
moshe weitzman commentedPlease wait for green before commit.
Comment #10
dries commentedI support this patch.
There are a lot of places in Drupal where we call
exit(). Is it time for adrupal_exit()that invokes the exit hook?Comment #11
effulgentsia commented@Dries: I'm hoping we start having fewer places that call
exit(). Can you please look at #599804: Unify page, AJAX 'path', and AJAX 'callback' callbacks which will move us in that direction?Comment #12
effulgentsia commentedAt Dries's request, this patch implements drupal_exit(). I did not convert some calls from exit to drupal_exit(), specifically the ones I wasn't as sure about, especially within bootstrap.inc and tests. This patch needs to be re-reviewed. If there's no time to properly review this and you need me to re-post #8, please let me know.
Comment #13
effulgentsia commentedFixed a bug when called from drupal_goto().
Comment #14
moshe weitzman commentedvery nice cleanup. i'm satisfied when the bot goes green
Comment #15
dries commentedAlright, looks good. Committed to CVS HEAD. Thanks!