index.php switch statement matches results beginning with number [#101486]

Take a look at the switch statement in index.php. We have three special cases before the default case.

$return = menu_execute_active_handler();
switch ($return) {
  case MENU_NOT_FOUND: // integer: 2
    drupal_not_found();
    break;
  case MENU_ACCESS_DENIED: // integer: 3
    drupal_access_denied();
    break;
  case MENU_SITE_OFFLINE: // integer: 4
    drupal_site_offline();
    break;
  default:
    // Print any value (including an empty string) except NULL or undefined:
    if (isset($return)) {
      print theme('page', $return);
    }
    break;
}

The bug is that the case statements match not only an integer but any string that begins with said integer. Thus, the following module will give Access Denied and Site Offline errors when you would not expect it to:

// musketeers.info file
; $Id$
name = Musketeers
description = Demonstration of flaky case statements.
package = Testing
version = VERSION

// musketeers.module
 
function musketeers_menu($may_cache) {
  $items = array();
  if (!$may_cache) {
    $items[] = array(
        'title' => 'Three musketeers',
        'path' => '3_musketeers',
        'description' => 'Your site is inaccessible',
        'access' => TRUE,
        'callback' => 'musketeers_noaccess'
      );
    $items[] = array(
        'title' => 'Four musketeers',
        'path' => '4_musketeers',
        'description' => 'Your site is closed',
        'access' => TRUE,
        'callback' => 'musketeers_closed'
      );
  }
  return $items;
}
 
function musketeers_noaccess() {
  $output = '3 musketeers!';
  return $output;
}
 
function musketeers_closed() {
  $output = '4 musketeers!';
  return $output;
}

The solution is to do an explicit check with === to make sure that both the value and the type match.

Comment	File	Size	Author
#15	index_is_int.patch	1.32 KB	jvandyk
#15
#7	index_is_int_switch.patch	1.06 KB	chx
#7
#6	index_cleanup.patch	1.78 KB	chx
#6
#3	musket2jv.patch	717 bytes	jvandyk
#3
#1	musketjv.patch	716 bytes	jvandyk
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jvandyk CreditAttribution: jvandyk commented 5 December 2006 at 15:54

File	Size
musketjv.patch	716 bytes

And now, the patch.

Comment #2

webernet CreditAttribution: webernet commented 5 December 2006 at 15:58

Status:

Needs review

» Needs work

Typo: "retun"

Comment #3

jvandyk CreditAttribution: jvandyk commented 5 December 2006 at 16:00

File	Size
musket2jv.patch	717 bytes

Now demonstrating ALL my typing skills.

Comment #4

jvandyk CreditAttribution: jvandyk commented 5 December 2006 at 16:01

Status:

Needs work

» Needs review

Comment #5

chx CreditAttribution: chx commented 5 December 2006 at 16:06

Status:

Needs review

» Needs work

I remove from the review code and prey nor Steven nor Dries ever sees this. You got to be kidding with case $foo === . What's next, we will use a Duff device in Drupal?

Comment #6

chx CreditAttribution: chx commented 5 December 2006 at 16:36

File	Size
index_cleanup.patch	1.78 KB

What's the point of this switch statement?

Comment #7

chx CreditAttribution: chx commented 5 December 2006 at 16:47

File	Size
index_is_int_switch.patch	1.06 KB

grugnog suggested is_int .

Comment #8

Owen Barton CreditAttribution: Owen Barton commented 5 December 2006 at 17:03

Status:

Needs work

» Needs review

I actually prefer #6 for elegance (tested - it worked great!).
As far as I can see the switch is basically not doing anything that we can't do directly (and in a less obtuse and brittle) way anyway.

I could only find 1 obsolete contrib module (administration) which would be affected by the loss of the constants (and even then would be a no-brainer fix).

Comment #9

Arto CreditAttribution: Arto commented 5 December 2006 at 17:50

With patch #6, wouldn't we be losing the ability to perform multiple calls to menu_execute_active_handler()? (Also, that function is called in a nested context a couple of times from includes/common.inc - has that been taken into account in #6?)

TBH, I don't know how re-entrant menu_execute_active_handler() is or if anyone's actually using it via multiple consecutive calls to render multiple different pages (changing $_GET['q'] in-between), but if that actually works it's a feature I could imagine making use of in the Boost module and wouldn't want to lose.

+1 for patch #7.

Comment #10

chx CreditAttribution: chx commented 5 December 2006 at 19:12

while drupal_not_found calls back the execute handler function, if said page exists then it runs and returns. If that page does not exist then we have some trouble indeed... #7 could be less problematic.

Comment #11

Dries CreditAttribution: Dries commented 6 December 2006 at 16:17

#6 looks a lot cleaner to me, but requires significant testing. I'd prefer to explore #6 more in favor of #7.

Comment #12

Steven CreditAttribution: Steven commented 6 December 2006 at 20:41

I prefer #7... having the menu system be able to execute multiple handlers on the same page is very useful. What if you want to access a certain menu hook through an alternative protocol... you'll want to return the error or not found status itself in any form, not as a prebaked 404/403 error HTML page.

Comment #13

Frando CreditAttribution: Frando commented 7 December 2006 at 12:41

I'm in favor of #7 or something similiar.

In a project that requires quite a lot of AJAX calls, I'm catching all incoming requests in a hook_init, and if a certain $_GET is set, I call menu_execute_active_handler() myself to be able to put all outgoing pages through some more theme and processing functions and exit() then (so that theme_page never gets called, because I want to return only the main content and some metadata (title, ..) in a xml file).

This would break for ACCESS_DENIED and NOT_FOUND if #6 gets applied (if I understand it correctly), because menu_execute_active_handler would not return the error code anymore but call drupal_not_found() directly, which then calls theme_page(), which I don't want to be called in my scenario

Comment #14

Dries CreditAttribution: Dries commented 7 December 2006 at 17:27

Alright, I'm convinced. Patch #7 appears to be favorable.

I think we should document this though. Any programmer looking at index.php will say: 'hey, why don't we do that in one switch-statement?'. (Not required for the patch to get committed.)