Support from Acquia helps fund testing for Drupal Acquia logo

Comments

erikwebb’s picture

This is a forced redirect for users with an expired password. We could change this behavior to only trigger at login, then print a message on each page load instead. I'm worried about changing behavior that significantly for 6.x-1.x at this point. Any other suggested flows?

killes@www.drop.org’s picture

I'd be ok if you'd make a 6.x-2 release for us. ;)

I am not sure, but that user_load must go.

erikwebb’s picture

So if we can remove the user_load(), but leave the remainder of the hook_init() code, is that fair?

killes@www.drop.org’s picture

I am not sure what would remain. The rest seems to depend on the user_load.

erikwebb’s picture

The rest actually would work without the explicit user_load. From what I remember, it was there to solve some kind of race condition that resulted in not all user properties being loaded. It may actually run correctly without this code anyway. I don't have a D6 site setup to test right now, but maybe I can verify this weekend.

vijaycs85’s picture

Status: Active » Needs review
FileSize
1.09 KB

It might be possible that $user might not have 'force_password_change', for which we need to find a way. otherwise, this code is good to go.

Status: Needs review » Needs work

The last submitted patch, 2063599-remove-password_policy_init-1.patch, failed testing.

erikwebb’s picture

From my perspective, I think we should focus on removing the user_load(), but the rest of the code should remain so that current functionality isn't affected.

hefox’s picture

What's password policy's main branch as far as features go? Currently this issue is set for 6.x-1.x. A version of this issue for 7.x was marked as duplicate.

travist’s picture

You could just create a direct load method that uses static caching and therefore offer reverse compatibility while still maintaining the performance increase. Here is the patch I came up with.

travist’s picture

Status: Needs work » Needs review
ablommers’s picture

Wouldn't it be better removing this peace of code from the hook_init and move it to the hook_user ($op: login ) instead. Currently when a user is logged in and an admin would set the option "force password reset at next login" the user is redirected to the account page to reset his password, regardless of what he is doing.
Similar problems occur when using the masquerade module. At the moment you try to masquerade as a user for which the password has expired you are forced to change the password, despite the fact that you are not that user at all and should not be able to change his password.

Forcing password change only at login would reflect the option on the account page that says the user needs to reset his password at next login (and not at next page request).

vijaycs85’s picture

Fix looks great, here are some review comments:

+++ b/password_policy.module
@@ -52,6 +48,25 @@ function password_policy_init() {
+  if (isset($force_change[$account->uid])) {
+    return $force_change[$account->uid];
+  }
+
+  $force_change[$account->uid] = db_result(db_query('SELECT force_change FROM {password_policy_force_change} WHERE uid=%d', $account->uid));
+  return $force_change[$account->uid];

1. if the uid doesn't have an entry in {password_policy_force_change} table, we get NULL? if we get null, isset() always fails as it checks if a variable is set and is *not* NULL?

2. Can we have it like below so that we have one exist point from the function?

+++ b/password_policy.module
@@ -52,6 +48,25 @@ function password_policy_init() {
+  if (!isset($force_change[$account->uid])) {
+    $force_change[$account->uid] = db_result(db_query('SELECT force_change FROM {password_policy_force_change} WHERE uid=%d', $account->uid));
+  }
+
+  return $force_change[$account->uid];
bdlangton’s picture

This updates #10 to apply cleanly on 6.x-1.6.

vijaycs85’s picture

@bdlangton, the patch should work against 6.x-1.x-dev. We may need to create a new tag (i.e. 6.x-1.7) to make this update.

vijaycs85’s picture

Answering my questions on #13:
#13.1 No,db_result returns FALSE (Not NULL), if there is no row.
#13.2. Fixed in below patch..

+1 to RTBC from my side.

We may need to consider porting to 7.x-1.x as #2063599: hook_init causing performance issue is 7.x-1.x and closed as duplicate of this issue :(

AohRveTPV’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
FileSize
4.06 KB

Patch for 7.x-1.x. Didn't use a static variable because I don't think it's necessary. Please review.

AohRveTPV’s picture

D6 patch in #16 looks pretty good on second viewing. I will try to implement that way for D7, using a static variable, to see if the code comes out simpler.

Two comments on #16:
1. Would improve performance for sites with many anonymous users to skip database lookups for uid 0, I think. Agree?
2. The function name password_policy_force_change() by itself sounds like it is forcing a password change to me. Maybe password_policy_get_force_change() or something else would be clearer?

Hopefully commenting on this old issue is not beating a dead horse. It is a significant performance problem in the only stable/mature releases of Password Policy.

AohRveTPV’s picture

Status: Needs review » Needs work
AohRveTPV’s picture

Status: Needs work » Needs review
FileSize
2.75 KB

Reimplemented patch from #18 using static variable approach of #16. I think my method that intentionally avoided using a static was no more understandable and perhaps more complex. Please review and test.

  • Commit 36a84cb on 7.x-1.x by AohRveTPV:
    Issue #2008282 by travist, vijaycs85, bdlangton, AohRveTPV: user_load in...
AohRveTPV’s picture

Status: Needs review » Patch (to be ported)

Should be fixed in 7.x-1.x. Needs porting to 6.x and 7.x-2.x.

  • Commit a888eb6 on 7.x-1.x by AohRveTPV:
    Issue #2008282 by travist, vijaycs85, bdlangton, AohRveTPV: user_load in...
  • Commit b734389 on 7.x-1.x by AohRveTPV:
    Revert "Issue #2008282 by travist, vijaycs85, bdlangton, AohRveTPV:...

  • Commit 284c5a1 on 6.x-1.x by AohRveTPV:
    Issue #2008282 by travist, vijaycs85, bdlangton, AohRveTPV: user_load in...
AohRveTPV’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev
AohRveTPV’s picture

Status: Patch (to be ported) » Postponed

Confirmed that in 7.x-2.x there are user_load() calls which will happen in hook_init() if the history or delay constraints are enabled. This inefficiency is a relatively lesser problem in 7.x-2.x, so postponing.

  • AohRveTPV committed 36a84cb on 8.x-1.x
    Issue #2008282 by travist, vijaycs85, bdlangton, AohRveTPV: user_load in...
  • AohRveTPV committed b734389 on 8.x-1.x
    Revert "Issue #2008282 by travist, vijaycs85, bdlangton, AohRveTPV:...
  • AohRveTPV committed a888eb6 on 8.x-1.x
    Issue #2008282 by travist, vijaycs85, bdlangton, AohRveTPV: user_load in...