Add hooks for post-authentication actions

[alxbridge]

This PR creates two hooks hook_saml_idp_login_completed and hook_saml_idp_reauthenticated, so Drupal modules can perform custom operations after successful authentication or reauthentication. These are fired from new functions for reauthenticate and completeAuth, overriding those from the parent SimpleSAML_Auth_Source class.

(There are also some code style changes as suggested by my IDE's linter.)

[bradjones1]

@alxbridge Thanks for working on this and submitting the PR. A few items:

It seems like you've also linted/done code style changes here as well; I appreciate that, however I'm likely to commit that separately to make the history more atomic.

More importantly, I am curious if we shouldn't fire events rather than hooks; also, hooks would need to be documented in an api.php file, the events require some new classes and using the dispatcher, however I am partial to events for a number of reasons, not least of which that they are object-oriented and go in line more with the direction of development for D8. I do concede there are some other hooks already in this module, e.g. hooks_saml_idp_attributes_alter, but we could deprecate and replace with events.

Regarding the post-auth hook/event, I think we could simplify that into something like:

// Fire event here.
parent::completeAuth(&$state);

[alxbridge]

@bradjones1 Thanks for reviewing this and for the feedback.

• I've now removed the commit with the linting/style changes, as you're right that that would be better handled separately.
• I agree with your point regarding firing events rather than hooks. However, I'm finishing up on a project (which these additions originated from) at the moment, and unfortunately it's not feasible for me right now to make that change & the subsequent changes to my dependent modules.
• Given the above, I've added an api.php file to this PR to cover the hooks as they're currently implemented, so that documentation is now present at least.
• Sorry if I'm being slow, but I don't quite follow your point about the post-auth hook/event: is that a suggestion for if the hook/event changes were made, or something I can act on with the code as-is?

[bradjones1]

I agree with your point regarding firing events rather than hooks. However, I'm finishing up on a project (which these additions originated from) at the moment, and unfortunately it's not feasible for me right now to make that change & the subsequent changes to my dependent modules.

That's partly an inherent risk of using in-dev software in production, so I know the feeling though I'm also a bit unconvinced. You could implement a shim on your side to react to the event by calling your hook. Since this is the core of the PR, I'm not trying to trip you up, but I don't think adding more technical debt to the module is necessarily the way to go, either. Thoughts?

Sorry if I'm being slow, but I don't quite follow your point about the post-auth hook/event: is that a suggestion for if the hook/event changes were made, or something I can act on with the code as-is?

No apology needed, I might not have been as clear as I could have been. I think my point is that you have a comment in your code. // (This function is otherwise identical to the parent) - So instead of wholesale copying the method from the parent for the sake of just firing off a hook, do your hook and then just call the parent method, so we are not maintaining our own copy of the upstream code.

I think for me to want to merge this, given our conversation thus far, I'd like to just do events. If that doesn't work for you, you're of course welcome to continue patching your local copy against your PR as it stands, or shim as above. What do you think?

[alxbridge]

I think my point is that you have a comment in your code. // (This function is otherwise identical to the parent) - So instead of wholesale copying the method from the parent for the sake of just firing off a hook, do your hook and then just call the parent method, so we are not maintaining our own copy of the upstream code.

I think I did try this approach, but the function needed the code which is copied from the parent method, to be sure that the value in $state is handled & passed correctly.

Since this is the core of the PR, I'm not trying to trip you up, but I don't think adding more technical debt to the module is necessarily the way to go, either. Thoughts?
I think for me to want to merge this, given our conversation thus far, I'd like to just do events. If that doesn't work for you, you're of course welcome to continue patching your local copy against your PR as it stands, or shim as above. What do you think?

I totally understand your position here, so I'm not going to push to get this merged. If I have the opportunity a bit further down the line then I'll revisit this, and I'll continue using my branch in the meantime.