See http://groups.drupal.org/node/21357
hook_openid_provider_ax($op, $attributes, $request) and
hook_openid_provider_sreg($op, $sreg_fields, $request)
is too much.
Almost the same signature, almost the same purpose. Let's unify them.
Also in the patchset (depending patches): #488628: Simplify provider-side hooks , #488636: Simplify provider-side hooks in D7 port

CommentFileSizeAuthor
openid_cp_field_union_provider_hooks.patch2.54 KBaron novak

Comments

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented

Patchset also includes #489776: Simplify provider-side hooks at OpenID stack

alex_b’s picture

alex_b commented

I'm actually not convinced whether this consolidation is a good idea, here are my reasons:

- the change has only minor benefits in making the code clearer in openid_cp_field, there may be other ways to achieve this.
- there is a certain clarity in linking the hook to a specific invoker module (i. e. sreg invokes an _sreg hook)
- it comes at the cost of patching 4 projects
- it moves against the trend in Drupal to break out $op style hooks into their own hooks

I may be overlooking some aspect here, is there any other issue this patch would be solving?

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented

There is no additional benefit on the top of decreasing the number of the hooks at openid stack and making the code a little bit simplier in openid_cp_field.
"it comes at the cost of patching 4 projects" - what are under heavy development
"it moves against the trend in Drupal to break out $op style hooks into their own hooks" - $op is not affected by the patch, anyway, $op is not used really, it can be eliminated.
"there is a certain clarity in linking the hook to a specific invoker module" - agree

xamanu’s picture

xamanu commented

I think it would be fine to do this in the 7.x version. But to keep things simple I would not put this in the 6.x branches. Since this module is not going to be available for D7 we can close this case. But similar things have to be done for the OpenID Profile module. And of course the other modules should have it in the 7.x branches: #488628: Simplify provider-side hooks #488636: Simplify provider-side hooks in D7 port

xamanu’s picture

xamanu commented
Status: Needs review » Closed (works as designed)

Closing