On the user edit page (and presumably on other pages with password_confirm fields), if a user enters a password only in the "Confirm password" field (ie, the 2nd field), the form validates and returns "The changes have been saved." However, the password hasn't been changed. The validation in the form.inc password_confirm_validate function only runs if the first password isn't blank.
I've done some testing and the same issue exists in D5, D6, and D7.
I'm attaching a small patch that runs the validation if either the first or second password field are not blank.
Comment | File | Size | Author |
---|---|---|---|
#12 | password.patch | 2.71 KB | Jody Lynn |
#7 | password.patch | 2.8 KB | Jody Lynn |
#4 | form.inc_.patch | 2.32 KB | ugerhard |
#3 | form.inc_.patch | 910 bytes | ugerhard |
password_confirm.patch | 590 bytes | Matt V. | |
Comments
Comment #1
c960657 CreditAttribution: c960657 commentedLooks good.
Comment #2
webchickThis is a stupid bug to have. Let's get some tests for this.
Comment #3
ugerhard CreditAttribution: ugerhard commentedFirst a quick reroll. We will try to write a test for this at the Paris code sprint today
Comment #4
ugerhard CreditAttribution: ugerhard commentedNow with test
Comment #5
attiks CreditAttribution: attiks commentedPatch applies fine and fixed the problem
+1
Comment #7
Jody LynnI adjusted the tests a bit and rerolled.
Comment #8
Matt V. CreditAttribution: Matt V. commentedThe tests look good to me, with the exception of the description for the last test. Does "Save user password with mismatched type in password confirm" really describe what is happening there? It looks to me like it's setting matching passwords, but it says "mismatched" in the description.
Comment #9
lcool CreditAttribution: lcool commented#7: password.patch queued for re-testing.
Comment #11
Jody LynnI'm rerolling
Comment #12
Jody LynnPatch rerolled. I removed the part of the test with the wording criticized in #8 (the part showing that matching passwords do work) because it now seemed to be redundant with existing tests.
Comment #13
domthinks CreditAttribution: domthinks commentedReviewed and testing with drupal 7.x-dev and worked as expected. The patch checks both password fields and if the 'confirm password' field is empty it returns "The specified passwords do not match."
Comment #14
stupiddingo CreditAttribution: stupiddingo commentedWorks when one of the two is null, but exhibits some odd behavior from a usability standpoint when both are null.
Three possibilities:
If "confirm password" is null:
Error Message
----------------
The specified passwords do not match.
Your current password is missing or incorrect; it's required to change the Password.
If "password" is null:
Error Message
----------------
The specified passwords do not match.
Both Null:
Error Message
----------------
The changes have been saved.
>>> This makes sense in a way since one could change the their timezone and would not need to enter their passwords. But oddly, (is this is separate issue?) it returns The changes have been saved. when no changes are made and the form is posted (technically this may be true, but it's a little odd from a usability standpoint).
Comment #15
Dries CreditAttribution: Dries commentedCommitted #12 to CVS HEAD, but we might want to tweak things some more based on #14.
Comment #16
chriso CreditAttribution: chriso commentedSubscribing
Comment #17
Tsalop CreditAttribution: Tsalop commentedHere is a quick fix for #14 issue based on Jody Lynns patch:
I'll post patch after finding a good solution for that "The changes have been saved" - message
even if no changes have been...
Comment #18
Tsalop CreditAttribution: Tsalop commentedThe solution mentioned in my previous post can be found from this patch.
Comment #19
Gaelan CreditAttribution: Gaelan commentedNeeds a reroll because everything moved to core/* in D8.
Comment #20
Gaelan CreditAttribution: Gaelan commentedOh. This is 7. Never mind.
Comment #21
rkjha CreditAttribution: rkjha commented#12: password.patch queued for re-testing.
Comment #23
rkjha CreditAttribution: rkjha commentedTo the best of my understanding, the test cases for this issue and for changing password or other user details are complete. I can't think of anything else that should be included except for the case when the message "The changes have been saved" is displayed even when there have been no changes. Any suggestion is welcome.
Comment #24
rkjha CreditAttribution: rkjha commentedAll the changes suggested in #14 have already been committed(commit no 66d75d16fcc1e531811fb3bcf32a2f4318c01588). And the tests for the same already exist. The only thing that seems to remain is finding a solution for "The changes have been saved" message as mentioned in #14. But that appears more to be a feature request than a bug an this is apparently "impossible" to fix. I therefore suggest to close this issue as it has been open for quite a long without really making much sense.
Comment #25
star-szr@rkjha and I talked about this issue in IRC today.
As far as we can tell from looking at this issue and performing manual tests of the user edit form, the only change (at least for D7/D8) that is mentioned in #14 is that when no changes are made to the user profile, the message "The changes have been saved." should not be displayed.
We agreed that #14 seems like more of a feature request, and that it would need to apply on a higher level so that for example updating a node without changing any fields would not say "[node:title] has been updated."
So unless we're both missing something, I think we can close this issue as fixed since the main issue was addressed with the commit in #12 and the response to the request in #14 would likely be "closed (won't fix)" or maybe even "closed (works as designed)".
Comment #26
dman CreditAttribution: dman commentedI can't see any reason to keep this open now, no.
I'll bump it to 'as designed' as this original issue has been fixed enough now.