Some callbacks return junk when calling drupal_not_found(); replace with return MENU_NOT_FOUND instead.

Yep, I think this is a good change to make since I've seen problems before with various caching systems or headers causing this bug if code continues to execute past drupal_not_found(). Looks like we'll need to fix this in Drupal 8 and then backport this to Drupal 7 when it is fixed.

Same here, drupal_not_found() is no more in 8.x.

#5: core-1426122-5562850-return-MENU_NOT_FOUND-in-callbacks-instead-of-calling-drupal_not_found.patch queued for re-testing.

Same does for drupal_access_denied().

http://api.drupal.org/api/drupal/includes!common.inc/function/drupal_acc...

This is getting much bigger, a mindless patch pre-alpha but now at 7.18.... I wonder if this should be switched to multiple issues.

book.module
The book_export() is primarily fixed by this patch, but calling book_export_html() could also trigger this error that directly calls both drupal_not_found() and drupal_access_denied(). I have left this out.

I haven't tested to see what happens with drupal_get_form() page callbacks, but this may effect the following:

• contact_site_form
• contact_personal_form
• locale_languages_edit_form

Also, not sure on system_batch_page(), user_pass_reset() and user_cancel_confirm().

And the untested changes left are in the patch. Since drupal_exit() is also called on locale_languages_delete_form(), I wonder if this suggests that this can be called outside of the main menu system (or that this is just redundant leftovers)

   if (!isset($languages[$langcode])) {
-    drupal_not_found();
-    drupal_exit();
+    return MENU_NOT_FOUND;
   }

Marking as needs review, but this is only for the bot. Each part needs manual testing at least.

And the patch :{

Marked #1233854: Warning: ini_set() [ref.outcontrol]: Cannot change zlib.output_compression - headers already sent in drupal_serve_page_from_cach as a duplicate.

Most of these changes are not necessary (as they contain a return) and/or broken. You can *not* return the MENU constants from form builder functions, helper functions and so on and a lot of these look like such, without seeing much of the context.

Those are only supported from menu callback functions or if they are passed through the menu callback functions as well.

@Sascha were you referring to the ones marked in the comment only?

Re-roll without the return for the locale_languages_delete_form form (the menu callback was drupal_get_form).

+++ b/modules/menu/menu.admin.incundefined
@@ -622,8 +621,7 @@ function menu_item_delete_page($item) {
   if ($item['module'] == 'system' && !$item['updated']) {
-    drupal_access_denied();
-    return;
+    return MENU_ACCESS_DENIED;

+++ b/modules/profile/profile.admin.incundefined
@@ -192,8 +192,7 @@ function profile_field_form($form, &$form_state, $arg = NULL) {
 
       if (!$edit) {
-        drupal_not_found();
-        return;
+        return MENU_NOT_FOUND;
       }
       drupal_set_title(t('Edit %title', array('%title' => $edit['title'])), PASS_THROUGH);
       $form['fid'] = array('#type' => 'value',

What I mean are cases like this, and this patch consists mostly of those.

a) This is not necessary, as the old code is not broken (ugly, but it works)

b) The second example is within a form builder function, just like most others in profile.module and this does not work.

I suggest to *only* change those that are actually broken right now, like the one in comment.module

I just tried http://drupal/admin/structure/menu/item/1/delete (the admin link/router path) and got this warning:

Cannot modify header information - headers already sent by (output started at includes\common.inc:2690) in drupal_send_headers() (line 1216 of includes\bootstrap.inc).

Missed the second. The patch was a bit rush using a code search :?

Played with some tests, but when I try to run the tests for Drupal 7.x-dev, these blow up badly using PHP 5.4 (on a fully clean environment; with fatal errors blocking the batch process from completing)

But maybe something like this can be used if someone wants to follow up (I am bowing out due to time limitations at the moment):

    $this->deleteComment($comment_new_page);

    $this->drupalGet('node/' . $this->node->nid);
    $this->assertFalse($this->commentExists($comment), 'Comment not found.');
    $this->assertFalse($this->commentExists($reply, TRUE), 'Reply not found.');
#### cid or id????
+    $this->drupalGet('comment/' . $comment->id);
+    $this->assertResponse(404, 'Comment page not found.');

What is the problem with #15? Can it be committed?

b) The second example is within a form builder function, just like most others in profile.module and this does not work.

form builders should use drupal_exit()

So the patch can be modified slightly to use drupal_exit() in some places, right?

This patch modifies page callbacks only.

Absolutely, we should fix it.
Documentation for drupal_not_found() and drupal_access_denied() clearly states that page callback functions shouldn't call them directly and should instead return MENU_NOT_FOUND or MENU_ACCESS_DENIED.

In my module I've been using writing user activity info to database on hook_exit(). So all items have been inserted twice! Should fix it definetely!

+++ b/modules/statistics/statistics.pages.inc
@@ -54,9 +54,7 @@ function statistics_node_tracker() {
     return $build;
...
+  return MENU_NOT_FOUND;

@@ -99,7 +97,5 @@ function statistics_user_tracker() {
     return $build;
...
+  return MENU_NOT_FOUND;

wtf?

Checked all places where drupal_access_denied() used
Now all covered but seems needs tests

What about drupal_not_found()? Have you checked it in all places?

Yes, drupal_not_found() is replaced in all proper places except for inline documentation before the functions where it was replaced.

The patch in #26 is used on two production sites without problems.

Approve. No more double-inserts while hook_exit :-)

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/257fac8