Make session_restore.module compatible with persistent_login.module [#542540]

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.

Comment	File	Size	Author
#8	persistent_login+session_restore-542540-7.patch	831 bytes	iva2k
#8

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

markus_petrux CreditAttribution: markus_petrux commented 18 August 2009 at 23:38

Status:

Needs review

» Closed (won't fix)

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.

Comment #2

iva2k CreditAttribution: iva2k commented 1 September 2009 at 06:27

Title:	Make compatible with session_restore.module	» Make compatible with persistent_login.module
Project:	Persistent Login	» Session Restore
Status:	Closed (won't fix)	» Active

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

Comment #3

iva2k CreditAttribution: iva2k commented 18 September 2009 at 15:35

Title:

Make compatible with persistent_login.module

» Make session_restore.module compatible with persistent_login.module

nag nag

Maintainer of persistent login.module commented...

Can maintainer of session_restore.module comment on the request?

Comment #4

iva2k CreditAttribution: iva2k commented 5 March 2010 at 17:03

Project:	Session Restore	» Persistent Login
Category:	feature	» bug

@markus_petrux
No response from maintainers of session_restore.module. Moving back to Persistent Login queue. Please commit. Advantage of this patch in Persistent Login module is that it gives you full control of your variables. If a new variable is ever added, you don't have to work through updating session_restore.module.

I also changed that to bug report because this compatibility problem results in significant loss of functionality and access issues (my earlier post "login page is presented as expected, but it does not allow to get to the destination") - so it looks like a bug.

Comment #5

gapple

he/they

English

CreditAttribution: gapple commented 29 August 2011 at 05:27

Status:

Active

» Needs work

Your code has persistent_login_login twice.

I am unfamiliar with session_restore, and so would like some feedback from other users. I understand how conflicts could arise with some of the session variables, but what happens if persistent_login_welcomed is or isn't included in the list?

Comment #6

iva2k CreditAttribution: iva2k commented 29 August 2011 at 15:53

>Your code has persistent_login_login twice.
Thanks for spotting it. Though that does not make it unusable. Do you want me to roll a patch, or can you edit before commit?

>but what happens if persistent_login_welcomed is or isn't included in the list?
You have to decide as the maintainer of persistent_login which variables should be excluded - those variables that should not be restored.

Comment #7

iva2k CreditAttribution: iva2k commented 29 August 2011 at 15:54

Here's the edited code:

/*
 * 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_reauth',
    'persistent_login_welcomed',
  );
}