Replace drupal_not_found() and drupal_access_denied() with constants [#122130]

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.

Comment	File	Size	Author
#3	drupal6-task_error_pages_constant.patch	15.52 KB	Gurpartap Singh
#2	drupal-access-denied-not-found.patch	14.21 KB	nedjo
	drupal_not_found.patch	12.24 KB	nedjo

Comments

Comment #1

chx commented 23 February 2007 at 23:58

I like the patch (and would like it even more if -p would have been used for diffing).

Comment #2

nedjo

he/him/his

English

commented 19 May 2007 at 17:36

Status	File	Size
new	drupal-access-denied-not-found.patch	14.21 KB

Refreshing the patch. This simple change is essential to making Drupal AJAX compliant.

chx, for you, this time with -p.

Comment #3

Gurpartap Singh commented 20 May 2007 at 10:58

Status	File	Size
new	drupal6-task_error_pages_constant.patch	15.52 KB

You forgot to make changes to includes/file.inc file. Highlighted: http://pastebin.mozilla.org/66990

Comment #4

Gurpartap Singh commented 20 May 2007 at 17:07

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.

Comment #5

nedjo

he/him/his

English

commented 20 May 2007 at 17:12

Thanks for the notes.

You forgot to make changes to includes/file.inc file

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?

Comment #6

Gurpartap Singh commented 20 May 2007 at 18:02

Hmm it occurs on even clean install without any applied patches.. So probably not related to your issue.

Comment #7

nedjo

he/him/his

English

commented 20 May 2007 at 18:03

Okay, thanks for following up.

Comment #8

dries commented 21 May 2007 at 07:52

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.