This is a no-brainer. SSO's menu item is parsed once on each cached page request, twice on each uncached page request.

CommentFileSizeAuthor
#2 singlesignon-DRUPAL-5.clean-up.patch13.82 KBsun
#1 singlesignon-DRUPAL-5.clean-up.patch13.81 KBsun
singlesignon-DRUPAL-5.menu_.patch1.08 KBsun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new singlesignon-DRUPAL-5.clean-up.patch13.81 KB

Further code clean-up.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Menu item is not cached » Menu item is not cached / Code clean-up
Version: 5.x-1.2 » 5.x-1.3
StatusFileSize
new singlesignon-DRUPAL-5.clean-up.patch13.82 KB

Re-rolled.

wayland76’s picture

wayland76 commented

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 :).

wayland76’s picture

wayland76 commented

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.

wayland76’s picture

wayland76 commented

...and if I haven't said, it's a pleasure to have a guy of your obvious level of Drupal familiarity doing this.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

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:

+  // If no master URL is set or we are serving a bot, do nothing.
+  if (!$master_url || _singlesignon_is_bot()) {
+    return;
   }
wayland76’s picture

wayland76 commented

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

wayland76’s picture

wayland76 commented

Ok, it's now in the DRUPAL-5 branch

wayland76’s picture

wayland76 commented

...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.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Reviewed & tested by the community » Fixed
Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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