Migrate Drupal 7 user settings

@huzooka, Thanks for noticing this!

This will be a BC break for anyone expecting d6_user_settings to exist. See 3096676#comment-13416893 and #3039240: Create a way to declare a plugin as deprecated. It will probably be quicker to make a new d7 migration and a test.

Drupal\Tests\user\Kernel\Migrate\d6\MigrateUserConfigsTest::testUserMail() needs its assertions updated for tests to pass.

#4:

It will probably be quicker to make a new d7 migration and a test.

So you're proposing to take a different approach, and leave d6_user_settings untouched and instead just add d7_user_settings and make it identical to d6_user_settings? That makes sense and is super pragmatic. I like it! 😄

I started out with copying \Drupal\Tests\user\Kernel\Migrate\d6\MigrateUserConfigsTest as-is. It tests both d6_user_mail and d6_user_settings.

But then noticed that d7_user_mail is already tested in \Drupal\Tests\user\Kernel\Migrate\d7\MigrateUserMailTest. So, had to remove that test coverage.

Then I modified the sole remaining test method to match the pattern in MigrateUserMailTest. And at that point, \Drupal\Tests\user\Kernel\Migrate\d7\MigrateUserConfigsTest no longer makes sense — it should be \Drupal\Tests\user\Kernel\Migrate\d7\MigrateUserSettingsTest for it to make sense.

Also assertSame() instead of assertIdentical() — pointless to retain already deprecated function calls oobviously.

All of that in this interdiff/patch!

In the #8 patch, it's not clear what the differences are between the D6 and D7 migration definition and their corresponding test.

Using git diff -M1% didn't record it as a rename. But fortunately, --find-copies-harder saved the day! Attached is a patch that is identical to #8 but makes the changes made after copy/pasting existing files much more obvious:

+++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserSettingsTest.php
@@ -22,46 +22,23 @@ class MigrateUserConfigsTest extends MigrateDrupal6TestBase {
-    $this->assertFalse($config->get('notify.status_activated'));
-    $this->assertFalse($config->get('verify_mail'));
-    $this->assertIdentical('admin_only', $config->get('register'));
-    $this->assertIdentical('Guest', $config->get('anonymous'));
+    $this->assertTrue($config->get('notify.status_activated'));
+    $this->assertTrue($config->get('verify_mail'));
+    $this->assertSame(UserInterface::REGISTER_VISITORS_ADMINISTRATIVE_APPROVAL, $config->get('register'));
+    $this->assertSame('Anonymous', $config->get('anonymous'));

All four expectations here are different in the D7 DB fixture compared to the D6 one. Hence the changes.

These are the only functional changes in the test coverage, compared to the D6 test coverage. This is why I wanted to create a patch like #9: because it makes that much more obvious 🤓

I compared the new d7_user_settings migration to d6_user_settings and the only changes are to the name and the migration tags. That is correct.

Then I reviewed the new test file, using the information in #8 -#10 which explains the differences. I agree with the changes and new test is testing what it should be.

There are just a few changes to do.

1. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserSettingsTest.php
   @@ -1,19 +1,19 @@
   + * Migrates user settings.
   

   This tests the migration, 'Tests migration of user settings.'

2. +++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserSettingsTest.php
   @@ -22,46 +22,23 @@ class MigrateUserConfigsTest extends MigrateDrupal6TestBase {
   +    // Map D7 value to D8 value
   

   How to refer to D8/D9?. Just avoid it and comment 'Map source values to destination values.'

+++ b/core/modules/user/tests/src/Kernel/Migrate/d7/MigrateUserSettingsTest.php
@@ -22,46 +22,23 @@ class MigrateUserConfigsTest extends MigrateDrupal6TestBase {
     // Tests migration of user_register using the AccountSettingsForm.

Remove blank line following this standalone comment.

In the patch the new test MigrateUserSettingsTest is a copy and as such doesn't show all lines. There is a comment that does not end in a full stop that needs to be fixed. The line is // Tests migration of user_register = 1.

Alright, so that looks like 100% nitpicks! 🥳 Yay, thank you for the super fast review, @quietone!

I think this would make an excellent DrupalCon sprint task 🤓

I've done the changes asked in #11.
Please review the patch.

@shreyakaushik11 I'm afraid the interdiff is wrong. Can you please regenerate it? 🙏

@Wim Leers, sorry I got confused and created interdiff for #9. Now, I've created an interdiff for #8 and #13.
Please review.

Adding to the meta for D7 sources and simplifying title. Changing to 'task' because even though there was an oversight in for the D7 user settings it is still a task as part of the migration of D7 sites.

Lovely, thank you so much @shreyakaushik11! 😊🙏

AFAICT #13 then addresses @quietone's review in #11.

Committed and pushed 5e8e2edb22 to 9.2.x and c016a7dcb0 to 9.1.x. Thanks!

As this is a new migration I've backported it to 9.1.x so it is available for sites migrating from D7 asap.