CAS implemented hooks do not follow the standard hook naming convention. Need to rename the auth_filter hook to use a cas namespace. Warning the 6.x-3 and 7.x code will change here.

CommentFileSizeAuthor
#4 1059204-rename-hooks-and-add-documentation.patch6.22 KBbfroehle
#3 1059204_rename_hooks_revised.patch1.18 KBbfroehle
#2 1059204_rename_hooks.patch1.89 KBmetzlerd
#1 1059204_rename_auth_filter_hook.patch1.37 KBmetzlerd

Comments

metzlerd’s picture

metzlerd commented
Assigned: Unassigned » metzlerd
Status: Active » Needs review
StatusFileSize
new 1059204_rename_auth_filter_hook.patch1.37 KB

I also took away the 'cas' constant, as it seems to add no value.

metzlerd’s picture

metzlerd commented
Title: Rename auth filter hook » Rename hooks
StatusFileSize
new 1059204_rename_hooks.patch1.89 KB

On second thought, renaming both hooks, because hook_auth_transform should go away when we remove the dependency on external authmap tables.

bfroehle’s picture

bfroehle commented
StatusFileSize
new 1059204_rename_hooks_revised.patch1.18 KB 
+++ cas.module	11 Feb 2011 23:19:32 -0000
@@ -34,10 +34,10 @@
+    $function = $module . 'cas_auth_transform';

Missing an underscore.

+++ cas.module	11 Feb 2011 23:19:32 -0000
@@ -56,11 +56,11 @@
+    $function = $module . 'cas_auth_filter';

Missing an underscore.

+++ cas.module	11 Feb 2011 23:19:32 -0000
@@ -56,11 +56,11 @@
+      if (($return = $function($cas_name)) === FALSE) {

We never use $return, so we can get rid of that.

Revised patch attached.

bfroehle’s picture

bfroehle commented
Issue tags: +Needs documentation
StatusFileSize
new 1059204-rename-hooks-and-add-documentation.patch6.22 KB

Since we are renaming the hooks already, I suggest we choose more intuitive names.

For transforming the user names (previously hook_auth_transform()), I suggest hook_cas_name_alter(). This then allows us to run the hook by simply calling drupal_alter('cas_name', $cas_name).

For allowing/denying access based upon user name, I suggest hook_cas_name_allowed(). (This name I'm not too thrilled with .. hook_cas_name_filter() didn't seem much better.... definitely open to suggestions here).

Also I've added a cas.api.php file which demonstrates the hooks, and added a note in the README about how the hooks were renamed.

metzlerd’s picture

metzlerd commented

I like the first, but not the auth_filter rename. Ultimately this filter is there to filter the user. As we move forward in implementation, it might be better to change this one to create a user object that we add some kind of way to say---- nope don't allow this person and wrap it up with the altering of user identitities. If we're going to get to removing authmap, we need to rethink this. Now we have at least 3 different issues that talk about refactoring hooks. I think we need to pick one and have the discussion there.

bfroehle’s picture

bfroehle commented
Status: Needs review » Closed (duplicate)

Let's move the discussion to #1059942: Refactor Hooks .