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.Problem/Motivation
The template looks like this as of this writing:
register:
plugin: static_map
source: user_register
default_value: visitors_admin_approval
map:
2: visitors_admin_approval
1: user_register
0: admin_onlyBut the constants used in the admin form are
USER_REGISTER_ADMINISTRATORS_ONLY => $this->t('Administrators only'),
USER_REGISTER_VISITORS => $this->t('Visitors'),
USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL => $this->t('Visitors, but administrator approval is required'),
Which map to:
const USER_REGISTER_ADMINISTRATORS_ONLY = 'admin_only';
const USER_REGISTER_VISITORS = 'visitors';
const USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL = 'visitors_admin_approval';
'user_register' is only in the map and 'visitors' is only in the constants so I assume that's the value that needs to be replaced.
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 2598376-12.patch | 2.77 KB | quietone |
| #12 | interdiff-2598376-8-12.txt | 3.38 KB | quietone |
| #8 | 2598376-8.patch | 3.54 KB | quietone |
| #8 | interdiff-2598376-2-8.txt | 2.9 KB | quietone |
| #2 | 2598376-2.patch | 510 bytes | quietone |
Comments
Comment #2
quietone CreditAttribution: quietone commentedI agree. A grep for 'user_register' in D8 shows that it only appears in the D6 and D7 dumps and d6_user_settings.yml.
Comment #3
ultimikeDo we need to write a new or update an existing test to go along with this?
-mike
Comment #4
chx CreditAttribution: chx commentedAs requested in the meta issue, here's how you test this and here's how you find out how to test it: you need to search the codebase. Searching for visitors_admin_approval finds
const USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL = 'visitors_admin_approval';so then we search for the constant and findAccountSettingsFormwhich uses this constants. A quick search ofUSER_REGISTER_in this file shows that the comparisons are used for#open. So: request the form andassertTrue($form['email_admin_created']['#open'])for example. You will need to run the migration as many times as the map to properly cover it and assert the right form key being open every time.Comment #5
quietone CreditAttribution: quietone commentedComment #6
quietone CreditAttribution: quietone commentedTalked to chx on IRC about testing this. He said
Therefore, marking as needs review.
Comment #7
chx CreditAttribution: chx commentedThat IRC discussion was a complete comedy of errors and misunderstanding!
Here's what happened (factor in me being jetlagged to hell): we were talking of implementing #4, I finally found it's
AccountSettingsForm::create($this->container);inUserAdminSettingsFormTestand said , this is what we need to use as well. Then I went to sleep. Then you asked what's the significance ofUserAdminSettingsFormTest. Nothing, nothing, nothing! It's just an example to show how to create that form. What I was (and now much more) frustrated that my well-meaning mention ofUserAdminSettingsFormTestcompletely threw you off track. So. The test in #4 is necessary, mentioningUserAdminSettingsFormTestwas not necessary. It is just a place to copy code from.Comment #8
quietone CreditAttribution: quietone commentedchx, lol. what else can I say? You were jetlagged and I'm suffering from a sore-throat. Neither of us thinking well.
But, I'm very glad we revisited this.
And thanks to your help I've put something together that works. It tests all three possible values of the user_register variable by building the AccountSettings form. For now, I left the all the existing tests. But probably should have removed the assertion on $config->get('register')?
Comment #9
chx CreditAttribution: chx commentedThis is incredible just as I imagined, thanks much! What about creating a loop?
Comment #10
chx CreditAttribution: chx commentedLet's see what the bot thinks. Finally, what about using the relevant constants?
Comment #12
quietone CreditAttribution: quietone commentedYes, I was thinking the same thing. Here it is.
I'm liking this approach.
Comment #13
chx CreditAttribution: chx at Smartsheet commentedThat's great. I would've used
[USER_REGISTER_ADMINISTRATORS_ONLY, USER_REGISTER_VISITORS, USER_REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL]as the map because the keys happen to be 0,1,2 but that's perhaps the wrong kind of clever. This is explicit and nice.Comment #14
catchFixed some whitespace and a trailing period on commit.
Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!
Failed to add chx to the commit message, but adding commit credit to issue.