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 is a sub-task of #1775842: [meta] Convert all variables to state and/or config systems
Comment | File | Size | Author |
---|---|---|---|
#13 | 1807266-cmi_user-cancel-method-13.patch | 10.8 KB | Cameron Tod |
#13 | interdiff.txt | 1.61 KB | Cameron Tod |
#10 | 1807266-cmi_user-cancel-method-9.patch | 11.05 KB | Cameron Tod |
#10 | interdiff.txt | 3.01 KB | Cameron Tod |
#8 | 1807266-cmi_user-cancel-method-8.patch | 8.64 KB | Cameron Tod |
Comments
Comment #1
barbun CreditAttribution: barbun commentedAnd here's the patch.
Comment #3
barbun CreditAttribution: barbun commentedOk, added small tweak for default value.
Btw, I built my patch over this:
http://git.drupal.org/sandbox/heyrocker/1145636.git
is that ok?
Comment #4
barbun CreditAttribution: barbun commentedComment #6
barbun CreditAttribution: barbun commented#3: 1807266-user-cancel-method-3.patch queued for re-testing.
Comment #7
Cameron Tod CreditAttribution: Cameron Tod commentedThis patch looks basically fine, just a few whitespace issues:
There's a few leading whitespace issues like this.
Trailing whitespace here.
I will post a patch shortly...
Comment #8
Cameron Tod CreditAttribution: Cameron Tod commentedCleaned up the whitespace stuff.
Comment #9
BerdirIt probably wouldn't hurt to add some basic test coverage for this, see SystemUpgradePathTest.php (we should rename that to ConfigUpgradePathTest in another issue). Then we don't have to test it manually to RTBC this.
Comment #10
Cameron Tod CreditAttribution: Cameron Tod commentedOk so I have added an upgrade path, and set the default for the value in user.settings.yml instead of using a ternary.
There's an unused variable, $default_method, in user_cancel_confirm_form(), which is in D7 as well, so I wil create an issue for that once this gets committed.
Comment #11
Cameron Tod CreditAttribution: Cameron Tod commentedIssue created to rename the test class: #1825648: Rename SystemUpgradePathTest to ConfigUpgradePathTest, so we can have tests for the CMI upgrade path
Comment #12
BerdirNot sure if we need a separate test method for this, because it's the same database dump anyway. Other than that, this looks good to me.
Comment #13
Cameron Tod CreditAttribution: Cameron Tod commentedMoved tests into same method, renamed, and updated comment.
Comment #14
BerdirYes, I think this is fine like this.
Comment #15
catchLooks good. Committed/pushed to 8.x.
Comment #16
Cameron Tod CreditAttribution: Cameron Tod commentedGreat stuff. I have posted a patch over in #1825648: Rename SystemUpgradePathTest to ConfigUpgradePathTest, so we can have tests for the CMI upgrade path, if we can get that or something similar in it will be a good conduit to increasing test coverage of the conversion effort.
Comment #17.0
(not verified) CreditAttribution: commentedUpdated issue summary.