Posted by sun on May 8, 2008 at 2:34am
| Project: | Shared Sign-On |
| Version: | 5.x-1.3 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
This is a no-brainer. SSO's menu item is parsed once on each cached page request, twice on each uncached page request.
| Attachment | Size |
|---|---|
| singlesignon-DRUPAL-5.menu_.patch | 1.08 KB |
Comments
#1
Further code clean-up.
#2
Re-rolled.
#3
I just noticed this, and have a quick question; why have you removed the access check in singlesignon_admin_settings -- is it because this check is already done elsewhere?
Other than that, it looks pretty good, although there's one or two spots where I prefer it the way it was (for example, the if statement with $master_url and the bots, I prefer the original comments as being more informative). Don't re-roll, though; if you answer the question above, I'll apply the patch, and make the small changes I prefer afterwards. After all, I'll have to roll this patch for 6.x as well :).
#4
Oh, and BTW, you're not supposed to set your own patches to "reviewed and tested by the community", but to "patch (code needs review)". But now that I've reviewed it myself, I'll leave it.
#5
...and if I haven't said, it's a pleasure to have a guy of your obvious level of Drupal familiarity doing this.
#6
1) singlesignon_admin_settings() is a menu callback and not invoked elsewhere. Thus, it's sufficient to define 'access' in hook_menu().
2) Please reconsider changing this comment -- it's very much cleaner and absolutely easy to understand:
<?php+ // If no master URL is set or we are serving a bot, do nothing.
+ if (!$master_url || _singlesignon_is_bot()) {
+ return;
}
?>
#7
The problem with the comment is it says what we're doing (which should already be obvious from the code), but doesn't say why
#8
Ok, it's now in the DRUPAL-5 branch
#9
...and I've also committed something similar on the Drupal 6 branch. However, I'm planning to wait until the 25th of May 2008 before I release a new version.
#10
#11
Automatically closed -- issue fixed for two weeks with no activity.