Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
This would cause a lot of extra-queries on a largish website. Is there a way to avoid that? Possibly by not using the password history function?
Comment | File | Size | Author |
---|---|---|---|
#20 | remove_user_load_from_hook_init-2008282-20.patch | 2.75 KB | AohRveTPV |
#17 | remove_user_load_from_hook_init-2008282-17.patch | 4.06 KB | AohRveTPV |
#16 | 2008282-diff-10-16.txt | 767 bytes | vijaycs85 |
#16 | 2008282-hook_init-user_load-16.patch | 2.05 KB | vijaycs85 |
#14 | password_policy-improve_init_perf-2008282-14.patch | 2.08 KB | bdlangton |
Comments
Comment #1
erikwebb CreditAttribution: erikwebb commentedThis 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?
Comment #2
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedI'd be ok if you'd make a 6.x-2 release for us. ;)
I am not sure, but that user_load must go.
Comment #3
erikwebb CreditAttribution: erikwebb commentedSo if we can remove the user_load(), but leave the remainder of the hook_init() code, is that fair?
Comment #4
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedI am not sure what would remain. The rest seems to depend on the user_load.
Comment #5
erikwebb CreditAttribution: erikwebb commentedThe 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.
Comment #6
vijaycs85It 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.
Comment #8
erikwebb CreditAttribution: erikwebb commentedFrom 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.
Comment #9
hefox CreditAttribution: hefox commentedWhat'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.
Comment #10
travist CreditAttribution: travist commentedYou 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.
Comment #11
travist CreditAttribution: travist commentedComment #12
ablommers CreditAttribution: ablommers commentedWouldn'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).
Comment #13
vijaycs85Fix looks great, here are some review comments:
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?
Comment #14
bdlangton CreditAttribution: bdlangton commentedThis updates #10 to apply cleanly on 6.x-1.6.
Comment #15
vijaycs85@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.
Comment #16
vijaycs85Answering 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 :(
Comment #17
AohRveTPV CreditAttribution: AohRveTPV commentedPatch for 7.x-1.x. Didn't use a static variable because I don't think it's necessary. Please review.
Comment #18
AohRveTPV CreditAttribution: AohRveTPV commentedD6 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. Maybepassword_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.
Comment #19
AohRveTPV CreditAttribution: AohRveTPV commentedComment #20
AohRveTPV CreditAttribution: AohRveTPV commentedReimplemented 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.
Comment #22
AohRveTPV CreditAttribution: AohRveTPV commentedShould be fixed in 7.x-1.x. Needs porting to 6.x and 7.x-2.x.
Comment #25
AohRveTPV CreditAttribution: AohRveTPV commentedComment #26
AohRveTPV CreditAttribution: AohRveTPV commentedConfirmed 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.