Missing 'file' parameter in password_policy_menu() causes PHP fatal error when loading admin/people/expired [#1866720]

Hiya,

I've spotted what I think is a bug with the menu callback definition for admin/people/expired - the definition looks like this in the latest -dev and 1.3 code:

  $items['admin/people/expired'] = array(
    'title' => 'Expired accounts',
    'type' => MENU_LOCAL_TASK,
    'description' => 'Lists all expired accounts.',
    'page callback' => 'password_policy_expired_list',
    'page arguments' => array('password_policy_list_expired'),
    'access arguments' => array('unblock expired accounts'),
  );

... which causes the following PHP error:

Fatal error: require_once(): Failed opening required '.../sites/all/modules/contrib/password_policy/user.admin.inc' (include_path='...') in .../includes/menu.inc on line 515

Adding a 'file' key to the array fixes it:

  $items['admin/people/expired'] = array(
    'title' => 'Expired accounts',
    'type' => MENU_LOCAL_TASK,
    'description' => 'Lists all expired accounts.',
    'page callback' => 'password_policy_expired_list',
    'page arguments' => array('password_policy_list_expired'),
    'access arguments' => array('unblock expired accounts'),
    'file' => 'password_policy.admin.inc',
  );

I will attach this as a patch to the first comment - brb :)

Comment	File	Size	Author
#7	password_policy-1866720-missing_file_parameter_expired-7.patch	858 bytes	torpy
#7
#6	password_policy-1866720-missing_file_parameter_expired-6.patch	521 bytes	erikwebb
#6
#3	ExpiredPasswordsMenuCallbackFix-1866720-3.patch	3.94 KB	alexharries
#3
#2	ExpiredPasswordsMenuCallbackFix-1866720-1.patch	3.94 KB	alexharries
#2
#1	ExpiredPasswordsMenuCallbackFix-1866720-1.patch	3.94 KB	alexharries
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

alexharries CreditAttribution: alexharries commented 14 December 2012 at 13:24

File	Size
ExpiredPasswordsMenuCallbackFix-1866720-1.patch	3.94 KB

Patch attached against latest 1.x-dev version - one of my first patches, so hopefully it will all be ok!

Comment #2

alexharries CreditAttribution: alexharries commented 14 December 2012 at 13:25

Status:

Active

» Patch (to be ported)

File	Size
ExpiredPasswordsMenuCallbackFix-1866720-1.patch	3.94 KB

Ack, patch ignored due to issue status - I'll trying to change it to "Patch (to be ported)" and re-attach.

Comment #3

alexharries CreditAttribution: alexharries commented 14 December 2012 at 13:28

Status:

Patch (to be ported)

» Needs review

File	Size
ExpiredPasswordsMenuCallbackFix-1866720-3.patch	3.94 KB

Right, third time lucky? According to this bug report about vague "patch ignored" messages, I need to set it to "Needs review"...? http://drupal.org/node/1162082

Comment #4

alexharries CreditAttribution: alexharries commented 14 December 2012 at 13:28

Huzzah, that got it :)

Comment #5

erikwebb CreditAttribution: erikwebb commented 14 December 2012 at 14:10

Status:

Needs review

» Postponed (maintainer needs more info)

The password_policy_expired_list() function is actually in password_policy.module. The error you would see is something like "Call to undefined function password_policy_expired_list()", if that were the problem. (I agree it's not the right place, but that's a separate issue.)

In your error message, I'm not sure why it would be asking for '.../sites/all/modules/contrib/password_policy/user.admin.inc'. That's really odd. I honestly have no idea what this error refers to.

Comment #6

erikwebb CreditAttribution: erikwebb commented 26 May 2013 at 20:40

Status:

Postponed (maintainer needs more info)

» Needs review

File	Size
password_policy-1866720-missing_file_parameter_expired-6.patch	521 bytes

I've reformatted your patch without all the whitespace changes. Please confirm it still fixes the problem for you.

Comment #7

torpy CreditAttribution: torpy commented 24 June 2013 at 06:35

File	Size
password_policy-1866720-missing_file_parameter_expired-7.patch	858 bytes

The issue is that all paths under 'admin/people' fall under the 'user.admin.inc' pages file because that's where the menu callback for the 'admin/people' path is defined.

Due to the Drupal menu system, it considers all subpaths to really be under the main path. i.e. it seems to be doing some weird inheritance of properties, where if you define a menu path such as 'admin/people/blah', it will look at the definition for 'admin/people' for properties. The fix is to manually define a file in hook_menu.

I've change the file definition in hook_menu to point to 'password_policy.module' because thats where the menu callback function 'password_policy_expired_list' is defined.

Updated patch attached against 1.x-dev. :)

Comment #8

erikwebb CreditAttribution: erikwebb commented 24 June 2013 at 21:50

I wonder if this is really a core bug that should be raised (or noted) instead of something to workaround here.

We have tests that check these pages and don't seem to have issues. I wonder why SimpleTest would pass, but your normal Drupal installation would fail.

Comment #9

torpy CreditAttribution: torpy commented 25 June 2013 at 00:04

It could very well be a core bug, just reading over the inheritance rules (https://drupal.org/node/146172) and _menu_router_build() (https://api.drupal.org/api/drupal/includes%21menu.inc/function/_menu_rou...) seem to suggest that if you provide a 'page callback' function in your menu definition, the 'file' property should not be inherited.