Make session_restore.module compatible with persistent_login.module
| Project: | Session Restore |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
Jump to:
First off, Persistent_login is a great module - very useful! I think it should be included into Drupal core.
However, I bumped into a problem when both persistent_login and session_restore modules are enabled. They in theory should complement each other, however in practice session_restore makes persistent_login to hickup on protected pages - login page is presented as expected, but it does not allow to get to the destination. It happens when login is automatic and user selects one of protected pages. I figured session_restore plainly restores all session variables, screwing up persistent_login logic.
I resolved the problem when I've added this function to the end of persistent_login.module:
/*
* Implementation of hook_session_restore().
* It is used by session_restore.module to exclude variables that should not be restored.
*/
function persistent_login_session_restore() {
return array(
'persistent_login_login',
'persistent_login_default_user',
'persistent_login_check',
'persistent_login_login',
'persistent_login_reauth',
'persistent_login_welcomed',
);
}Please review for committing to 6.x-1.x-dev. I can roll a patch, but feel like there's no need, since there is no change of any existing code. It's only a new hook that kicks in when session_restore.module is enabled, and can be copy-pasted.
#1
I could commit this, but I don't like the idea to add stuff related to other modules here, unless strictly necessary.
On the other hand, session_restore could do this conditionally by putting this into a separate .inc file included after checking for
module_exists('persistent_login'). Many modules do this. For example, CCK includes support for modules such as Views, Token, etc. using this method.#2
@markus_petrux
I understand you point.
Moving to session_restore queue. I'm curious what its maintainer will say.
My take is that session_restore.module provided a simple hook mechanism for other modules to be friends. It does not burden persistent_login with too much code (just 9 lines w/o comments). Also, it is more clean in my opinion. For example, if during development of persistent_login there is another variable 'persistent_login_***' that gets introduced and needs special session treatment, it will be included in the above function in a single commit. If the burden is on the session_restore, then maintainer of persistent_login will have to notify the maintainer of session_restore of such a variable and then wait for it to percolate into a release, which will break sites for all the duration. If the above function was in persistent_login, then such module update will not break anyone's site and will work immediately.
And most importantly, both persistent_login and session_restore modules make so much sense together due to perfect complementary functions, that having a conflict between them just feels wrong.
#3
nag nag
Maintainer of persistent login.module commented...
Can maintainer of session_restore.module comment on the request?