user_load in hook_init is not performant

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?

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

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.

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.

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.

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.

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.

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

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];

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

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

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 :(

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

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.

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.

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

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.