Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The user password reset needs a test, quoting dmitrig01 'can use mail.inc to intercept the mail'.
Comment | File | Size | Author |
---|---|---|---|
#29 | 293487-29-password-reset-test.patch | 5.67 KB | dcam |
#23 | reset_tests-293487-23.patch | 5.71 KB | ZenDoodles |
#16 | issue-293487.patch | 4.54 KB | lilou |
#14 | password_reset_test_4.patch | 4.28 KB | mr.baileys |
#12 | password_reset_test_3.patch | 4.31 KB | mr.baileys |
Comments
Comment #1
mr.baileysFirst stab at this. This patch relies on the new assertions and API introduced in #296001: Capture e-mails sent during tests and add e-mail assertions / API.
This tests:
Some of the logic for this test was already present in the existing user registration test, but a work-around was used to create the one-time login link (direct call to user_pass_reset_url instead of capturing the e-mail).
Leaving at NW until #296001: Capture e-mails sent during tests and add e-mail assertions / API is commited, and because the tests sometimes fail for reasons unknown to me.
Comment #2
mr.baileys#296001: Capture e-mails sent during tests and add e-mail assertions / API has been commited, let's take this one for a spin...
Comment #4
mr.baileysThe original code to test the password reset URL had the following code:
When I leave this out, tests succeed on my local computer about 25% of the time. If I put it back in, tests consistently pass, so putting it back in here to see what the testbot says. I browsed the repository but since this was part of the initial commit, I couldn't find any further reasoning as to why it's in there.
Comment #6
mr.baileysComment #7
mr.baileysNote to reviewers: I haven't figured out why, but the test fails 1 out of 5 runs. The other 4 test runs succeed without any problem...
Comment #8
Dries CreditAttribution: Dries commentedThat
sleep(1)
is also confusing me.Comment #9
catchI think there's some validation which checks that the request time for the password request is < the request time for the time it's used, and on fast systems this can cause a failure. But that's half-remembered from something else similar rather than looking at the actual code it tests.
Comment #10
mr.baileys@catch: thanks for pointing me in the right direction. The current implementation for the one-time login link compares timestamps in seconds, and uses < and >. This means it might take up to one second before the link actually becomes active.
Since I think these links should be active immediately, I've created a separate issue for this: #481794: One-time link should be active immediately. Once this lands, the
sleep(1)
hack can be removed.Comment #12
mr.baileysre-rolled.
Comment #13
Dries CreditAttribution: Dries commentedThe sleep(1) is still in patch #12.
Comment #14
mr.baileysOops... sorry about that.
Re-rolled without the sleep(1)...
Comment #15
catch+ // Verify that the user received an e-mail.
missing a period
Not changed by the patch but could add one here too.
// Check password type validation
Comment #16
lilou CreditAttribution: lilou commentedAdd two missing periods.
Comment #17
kathyh CreditAttribution: kathyh commentedVerified that sleep() is not in the patch. Tests installed and reviewed.
User tests pass with no fails - 699 passes, 0 fails, and 0 exceptions
Comment #19
sun.core CreditAttribution: sun.core commentedTests don't qualify as critical.
Comment #22
mr.baileysPatch no longer applies after #46149: Prevent account cancellation for uid 1 was commited.
Comment #23
ZenDoodles CreditAttribution: ZenDoodles commentedFix in D8 first, then backport.
Comment #24
Ivan Zugec CreditAttribution: Ivan Zugec commentedPatch looks good and I was able to apply it. I'll leave this issue in "needs review" for others to test.
Comment #25
tomaszjureczko CreditAttribution: tomaszjureczko commentedI've:
- applied the patch
- run tests few times (php core/scripts/run-tests.sh --file core/modules/user/lib/Drupal/user/Tests/UserPasswordResetTest.php)
- review the code of the test
Everything works fine. I would apply the patch.
Comment #26
lucashodge CreditAttribution: lucashodge commentedI applied the patch, ran the test and got this message:
An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: /batch?id=5&op=do_nojs&op=do StatusText: OK ResponseText: Exception: The configuration directory type <em class="placeholder">staging</em> does not exist. in config_get_config_directory() (line 526 of /Users/lukehodge/Sites/d8/www/core/includes/bootstrap.inc).
Comment #27
adammaloneTest works perfectly for me - marking RTBC
Comment #28
catchOnly took four years :)
Committed/pushed to 8.x, moving to 7.x for backport.
Comment #29
dcam CreditAttribution: dcam commentedBackported #23 to D7.
Comment #30
YesCT CreditAttribution: YesCT commentedun assigning since it had been a while and someone else has posted patch anyway. :)
Comment #31
mgifford29: 293487-29-password-reset-test.patch queued for re-testing.