| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | menu system |
| Category: | task |
| Priority: | normal |
| Assigned: | nedjo |
| Status: | needs work |
| Issue tags: | API clean-up |
Issue Summary
Currently we call drupal_not_found() and drupal_access_denied() directly in many places, leading to full page renders. For menu callbacks, these calls are unneeded. We can simply return the constants MENU_NOT_FOUND and MENU_ACCESS_DENIED instead; index.php tests for these return values and calls drupal_not_found() and drupal_access_denied() as needed.
The main problem with our current direct calls to these functions is that it breaks any attempt to return just the menu item callback content. For example, if we wish to return the content of a page for use in AJAX, we get the page fine if it's found, but if not we get the full rendered output of theme_page().
Attached is a patch. I've left in the drupal_not_found() and drupal_access_denied() calls where they are not regular menu callbacks but special uses, e.g., for file piping.
By returning menu constants, we make the output system a lot more flexible. AJAX and other modules can decide what do return based on the returned constant, rather than being stuck with a full page render. It's more consistent with our general approach of returning results from menu callbacks rather than directly rendering. And it saves a few extra lines.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal_not_found.patch | 12.24 KB | Ignored: Check issue status. | None | None |
Comments
#1
I like the patch (and would like it even more if -p would have been used for diffing).
#2
Refreshing the patch. This simple change is essential to making Drupal AJAX compliant.
chx, for you, this time with -p.
#3
You forgot to make changes to includes/file.inc file. Highlighted: http://pastebin.mozilla.org/66990
#4
I have started getting these errors:
* warning: __clone method called on non-object in /var/www/drupal-6.x-dev/includes/common.inc on line 1436.* warning: __clone method called on non-object in /var/www/drupal-6.x-dev/includes/common.inc on line 1436.
* warning: __clone method called on non-object in /var/www/drupal-6.x-dev/includes/common.inc on line 1436.
* warning: __clone method called on non-object in /var/www/drupal-6.x-dev/includes/common.inc on line 1436.
Maybe after this patch? (or maybe because of my experimental code... which i fear not..). These error occur everytime on a 'not found' page. and not on normal pages.
#5
Thanks for the notes.
I left file piping out of the patch because it is a not a standard page request, see my note in the original issue post.
Regarding the clone method errors, would you be so kind as to test a clean patch (without your added experimental code) and report back?
#6
Hmm it occurs on even clean install without any applied patches.. So probably not related to your issue.
#7
Okay, thanks for following up.
#8
Wouldn't AJAX calls be able to look at the HTTP error code?
That said, I agree that this patch makes things more flexible so I'm in favor.
What would happen to JSON/AJAX calls after this patch lands? Is there any core functionality that could be simplified as the result of this? What are the logical next steps?
My only gripe with this patch is that is still allows module developers to call drupal_not_found() directly from their code. That would defeat the purpose of making Drupal more JSON/AJAX-friendly. Maybe it would be good to rename drupal_not_found() to _drupal_not_found()? I'd like to _enforce_ good practice so let's try to make it impossible to do otherwise.
#9
No longer applies, and moving to D7.
#10
Tagging.
I've mentioned a similar proposal over in #415474: Theme printed twice when access denied -- we should figure out which one to mark as duplicate.
#11
In D7 it seems like thrown Exceptions with a specific sub-class would be more appropriate than simply returning.