I've been getting random anonymous requests to the path user/validate (without any additional parameters), and it causes PHP errors because of missing arguments to logintoboggan_validate_email(). We can use menu loaders to help clean up some code as well as reduce some user account loading code by using the %user menu load argument.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Status: Active » Needs review
FileSize
9.03 KB
9.15 KB

Don't be scared with the big chunk of changed code in logintoboggan_validate_email(). All I did was remove the "sanity" checks to it's own menu access callback and then un-indented the function's code. Looks worse than the change really is.

hunmonk’s picture

Status: Needs review » Postponed (maintainer needs more info)

shouldn't the access check be:

$timestamp < REQUEST_TIME && $account->uid

Dave Reid’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
9.17 KB
9.05 KB

Oh that's true; it would successfully load the anonymous user. Moved the $account->uid check before the timestamp just incase we can skip running time() in D6. Revised patches for review.

hunmonk’s picture

Status: Needs review » Postponed (maintainer needs more info)

one more question related to the access functions:

isn't FALSE returned for %user if no user object can be loaded for that placeholder? if that's the case, we should be checking for !empty($account->uid) instead of $account->uid, in order to avoid PHP notices.

Dave Reid’s picture

Yes it does return FALSE, but the way menu loaders work, if any of the loaders return FALSE it assumes that the path is denied or invalid and doesn't even check the access callback and returns a 404.

See function stack menu_execute_active_handler() -> menu_get_item() -> _menu_translate() -> _menu_load_objects().

hunmonk’s picture

Status: Postponed (maintainer needs more info) » Fixed

committed to 6.x-1.x-dev and 7.x-1.x-dev, thanks for your work on this!

Dave Reid’s picture

Thanks to you too! :)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.