Migrate policies from Drupal 7 to Drupal 9.

This is looking great! 👍

Marking Needs work for the first two points of feedback in particular; everything else is pure nitpickery 🤓

1. +++ b/src/Plugin/migrate/process/PolicyConfiguration.php
   @@ -0,0 +1,103 @@
   +    unset($value['role']['roles']['1']);
   +
   +    if ($value['role']['roles']['2']) {
   +      $value['roles']["authenticated"] = "authenticated";
   +      unset($value['role']['roles']['2']);
   +    }
   +
   +    if ($value['role']['roles']['3']) {
   +      $value['roles']["administrator"] = "administrator";
   +      unset($value['role']['roles']['3']);
   +    }
   

   This should use a migration lookup; different Drupal 7 sites could have different role IDs.

   Example:

   $lookup_result = $this->migrateLookup->lookup('d7_user_role', [$role['rid']]);
   

   in \Drupal\field\Plugin\migrate\process\d7\FieldInstanceSettings::transform().

2. +++ b/src/Plugin/migrate/process/PolicyConfiguration.php
   @@ -0,0 +1,103 @@
   +      $value['days'] = explode(", ", $value['expire']['expire_warning_email_sent']);
   +      $value['days'] = str_replace(" days", "", $value['days']);
   +      $value['days'] = str_replace("-", "", $value['days']);
   +      $value['days'] = array_map('intval', $value['days']);
   

   Woah, this is an interesting piece of code! 😄

   These are 4 step-by-step transformations. In this case, what would be really helpful is if there was a comment at the top showing the input at the beginning and the output at the end; that allows somebody reading this code to make sense of these 4 transformation steps 😊 … without being overwhelmed 🙈😅

3. +++ b/src/Plugin/migrate/process/PolicyConfiguration.php
   @@ -0,0 +1,103 @@
   +  public function convert(string $value): int {
   

   1. Could be static because uses no data in the object; that way it's clearly enforced that this method cannot modify the object.
   2. AFAICT this could easily be protected? Never make public what you don't have to, because that de facto becomes an API you have to support! (I learned this the hard way 😬)

4. +++ b/src/Plugin/migrate/process/PolicyConfiguration.php
   @@ -0,0 +1,103 @@
   +    $arr = explode(" ", $value);
   +    $days = (int) $arr[0];
   +    if ($arr[1] == "months") {
   +      $days = $days * 30;
   +    }
   +    elseif ($arr[1] == "weeks") {
   +      $days = $days * 7;
   +    }
   +    elseif ($arr[1] == "hours") {
   +      $days = round($days / 24);
   +    }
   

   This … sort of feels like the kind of code that Drupal core should already have something for. So I grepped for "months". Didn't find anything useful 😬

   Then I realized there should be something for this even in PHP, right?

   So I searched php date months weeks hours: https://duckduckgo.com/?q=php+date+months+weeks+hours&ia=web and found https://www.php.net/manual/en/function.date-diff.php as the 4th search result.

   The first example at https://www.php.net/manual/en/datetime.diff.php is:

   $origin = new DateTime('2009-10-11');
   $target = new DateTime('2009-10-13');
   $interval = $origin->diff($target);
   echo $interval->format('%R%a days');
   

   And if I then enter now and now + 2 weeks in there, I get: +14 days.

   See https://3v4l.org/M5bZi

   But … that's not fundamentally better either.

   So +1 to keeping what you have here: it's the least amount of magic for sure!

5. +++ b/src/Plugin/migrate/source/PasswordPolicy.php
   @@ -0,0 +1,48 @@
   +      ->fields(
   +        'password_policy',
   

   This is formatted a bit weirdly to my Drupal-y eyes 🤓

   I'd have expected either:

      return $this->select('password_policy', 'password_policy')
        ->fields(
          'password_policy',
          ['name', 'config']
        );
   

   (when using multi-line formatting)

   or

      return $this->select('password_policy', 'password_policy')
        ->fields('password_policy', ['name', 'config']);
   

   (single-line formatting is possible because it's still less than 80 characters on this line).

6. +++ b/tests/src/Kernel/PasswordPolicyMigrateTest.php
   @@ -0,0 +1,106 @@
   +    $this->assertEquals(1, count($policies), 'Found one policy.');
   

   Can be simplified to:

   $this->assertCount(1, $policies);
   

I have one last round of feedback 🤓

1. +++ b/src/Plugin/migrate/process/PolicyConfiguration.php
   @@ -13,31 +17,83 @@ use Drupal\migrate\Row;
   +   * Constructs a MenuLinkParent object.
   

   Nit: s/MenuLinkParent/PolicyConfiguration/

2. +++ b/migrations/password_policy_settings.yml
   @@ -28,3 +28,6 @@ process:
   +migration_dependencies:
   +  required:
   +    - d7_user_role
   

   👏

3. +++ b/src/Plugin/migrate/process/PolicyConfiguration.php
   @@ -13,31 +17,83 @@ use Drupal\migrate\Row;
   +  // @codingStandardsIgnoreLine
   

   Interesting! Why is this necessary? 🤔

4. +++ b/src/Plugin/migrate/process/PolicyConfiguration.php
   @@ -13,31 +17,83 @@ use Drupal\migrate\Row;
   +      if ($role != 0) {
   

   🤔 What is special about role 0?

   Doesn't that correspond to the anonymous role for which we have a more explicit check 2 lines later?

5. +++ b/src/Plugin/migrate/process/PolicyConfiguration.php
   @@ -13,31 +17,83 @@ use Drupal\migrate\Row;
   +      // The initial format of no of days before a password expire warning email is
   

   🤓 Nitpick: the is at the end of this lines makes this line exceed the "80 characters per line" rule.

   Can you re-flow the comment so that it never exceeds 80 characters?

6. +++ b/src/Plugin/migrate/process/PolicyConfiguration.php
   @@ -13,31 +17,83 @@ use Drupal\migrate\Row;
   +      // sent is a comma separated string. A minus sign before the no. indicates
   +      // that the email is to be sent BEFORE expiration.
   ...
   +      // After transformation, we get an array of positive integers(no. of days).
   +      // [14,7,2]
   

   … that being said, this does explain what's happening very clearly! 👏

Looking great! 👏😊

I tried to reproduce the migration process between a drupal 7 and a drupal 9 fresh installation with one password policy created and I was not able to migrate it via the interface.
How can I manually test it?

My steps were:
1) Create e fresh Drupal 7 project and install password_policy module
2) Create a password_policy to set a minimum of uppercase characters to the password
3) Create e fresh Drupal 9 project and install password_policy module
4) Install password_policy_characters
5) Create the same password policy
6) Go to /upgrade

After adding the information of the Drupal 7 database, in /upgrade/review says that the password_policy module won't be migrated.
See image attached.

@paulocs: that's just because there's no password_policy.migrate_drupal.yml file defined added in the patch. If you use drush migrate:import password_policy_settings it will work just fine.

(Not a whole lot of people use migrate_drupal_ui AFAIK.)

We can easily add that though. Create password_policy.migrate_drupal.yml with these contents, and you'll be able to use that UI:

finished:
  7:
    password_policy: password_policy

Thank you @Wim Leers.
I'll try it later. I'm not very familiar with migration so I'm still learning about it.

Password policy in drupal 7 has no config column. When I run the migration I got the error:
Migration failed with source plugin exception: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'password_policy.config' in 'field list': SELECT "password_policy"."name" AS "name", "password_policy"."config" AS "config", "map"."sourceid1" AS "migrate_map_sourceid1", "map"."source_row_status" AS "migrate_map_source_row_status"

I suppose its because in password_policy_settings.yml has a reference to this field in:

      plugin: default_value
      source: config
      default_value: "a:0:{}"

In Drupal\password_policy\Plugin\migrate\source\PasswordPolicy as well.
The tests should be updated too so moving to NW.

Sorry, only seeing this now, @paulocs!

This migration was developed agains the 7.x-2.x version of the password_policy module, which does have that column: https://git.drupalcode.org/project/password_policy/-/blob/7.x-2.x/passwo... — but indeed the 7.x-1.x version does not: https://git.drupalcode.org/project/password_policy/-/blob/7.x-1.x/passwo....

Per https://www.drupal.org/project/usage/password_policy, ~2500 D7 sites use the 7.x-2.x branch, while ~22000 D7 sites use the 7.x-1.x branch.

We should update this patch to support both branches.

Until we do #14, here's an updated patch that uses #3151732: DrupalSqlBase::checkRequirements should test version with $minimum_version to ensure this migration only runs on sites that were using 7.x-2.x.

Actually, safer to require 7208, which corresponds to the latest update hook there.

Some thoughts on how to proceed here in regard of the data source:

The source plugin could be made compatible with 7.x-1.x if we don't explicitly specify the columns in the query:

/**
 * {@inheritdoc}
 */
public function query() {
  return $this->select('password_policy', 'password_policy')
    ->fields('password_policy');
}

And if we implement the MigrateSourceInterface::prepareRow() method, then we might be able to standardize the two different source data structure, either based on the columns we have, or based on the actual password_policy schema version:

/**
 * {@inheritdoc}
 */
public function prepareRow(Row $row) {
  $system_data = $this->getSystemData();
  $password_policy_schema = $system_data['module']['password_policy']['schema_version'] ?? 0;
  if ((int) $password_policy_schema >= 7200) {
    // 7.x-2.x version stuff.
  }
  else {
    // 7.x-1.x version stuff.
  }
  return parent::prepareRow($row);
}

(Note that the available password_policy columns in 7.x-1.x are pid, name, description, enabled, constraints (as a serialized array), created, expiration, warning (a positive number!); while in 7.x-2.x: name, config (serialized array of every single config).

A very rough review:

In addition to the above, we will need to refactor the migration and process plugin as well, because the only process plugin we have now tries to convert all of the destination policy's props, which will be the bottleneck of making the migration path compatible with both 7.x-1.x and 7.x-2.x.

We also should consider migrating other data (or at least triage how important they are), e.g. the previous password (hashes) also could be important imho.

But since this module had also a Drupal 4 version, declaring the minimum_schema_version is a requirement here! And I'd also change the ID of migration by adding a d7_ suffix.

follow up for #17.

1. +++ b/src/Plugin/migrate/process/PolicyConfiguration.php
   @@ -81,55 +82,147 @@ class PolicyConfiguration extends ProcessPluginBase implements ContainerFactoryP
   +    // Check if the source site's Password Policy version is 7.x-1.x or 7.x-2.x.
   +    if ($source_version === 2) {
   

   🤓 All logic in this if-branch should move into a private function transformV2() helper function.

2. +++ b/src/Plugin/migrate/process/PolicyConfiguration.php
   @@ -81,55 +82,147 @@ class PolicyConfiguration extends ProcessPluginBase implements ContainerFactoryP
   +    else {
   

   🤓 All logic in this else-branch should move into a private function transformV1() helper function.

3. +++ b/src/Plugin/migrate/source/PasswordPolicy.php
   @@ -10,7 +11,6 @@ use Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase;
   - *   minimum_version = "7208",
   

   👍 This means that it will now work for both 7.x-1.x and 7.x-2.x.

4. +++ b/src/Plugin/migrate/source/PasswordPolicy.php
   @@ -41,7 +41,42 @@ class PasswordPolicy extends DrupalSqlBase {
   +      $config['pid'] = $source_data['pid'];
   +      $config['name'] = $source_data['name'];
   +      $config['description'] = $source_data['description'];
   +      $config['enabled'] = $source_data['enabled'];
   +      $config['constraints'] = $source_data['constraints'];
   +      $config['created'] = $source_data['created'];
   +      $config['expiration'] = $source_data['expiration'];
   +      $config['warning'] = $source_data['warning'];
   +      $config['weight'] = $source_data['weight'];
   +      foreach ($password_policy_role as $role) {
   +        $config['role']['roles'][$role['rid']] = $role['rid'];
   +      }
   +      $row->setSourceProperty('config', serialize($config));
   

   This looks simple enough 👍 And the passing test proves that it works … but only kinda. I'd like to see:

   1. before-and-after screenshot of 7.x-1.x configuration and the equivalent in D9 (8.x-3.x version of the module)
   2. before-and-after screenshot of 7.x-2.x configuration and the equivalent in D9 (8.x-3.x version of the module)

5. +++ b/tests/fixtures/drupal7.1/password_policy.php
   @@ -0,0 +1,103 @@
   +  'enabled' => '0',
   

   👎 This is testing with a disabled password policy. I think we should test with an enabled one.

6. +++ b/tests/fixtures/drupal7.1/password_policy_role.php
   @@ -0,0 +1,53 @@
   +->values(array(
   +  'pid' => '1',
   +  'rid' => '2',
   +))
   +->values(array(
   +  'pid' => '1',
   +  'rid' => '3',
   +))
   +->values(array(
   +  'pid' => '1',
   +  'rid' => '4',
   +))
   

   This means the same password policy applies to all 3 roles. It'd be better if we had at least one other policy applied to one of the roles.

follow up for #19.

Added parameter doc comments.

1. +++ b/src/Plugin/migrate/process/PolicyConfiguration.php
   @@ -251,4 +116,153 @@ class PolicyConfiguration extends ProcessPluginBase implements ContainerFactoryP
   +   * Helper function for version2.
   ...
   +      // After transformation, we get an array of positive integers(no. of
   +      // days).
   +      // [14,7,2].
   ...
   +   * Helper function for version1.
   

   Could use a cleanup in formatting 🙏

2. +++ b/src/Plugin/migrate/process/PolicyConfiguration.php
   @@ -251,4 +116,153 @@ class PolicyConfiguration extends ProcessPluginBase implements ContainerFactoryP
   +    if ($value['expire']['expire_enabled'] === 1) {
   

   🤔 Do we really want strict equality checking here?

3. +++ b/src/Plugin/migrate/process/PolicyConfiguration.php
   @@ -251,4 +116,153 @@ class PolicyConfiguration extends ProcessPluginBase implements ContainerFactoryP
   +      // The initial format of no of days before a password expire warning email
   +      // is sent is a comma separated string. A minus sign before the no.
   

   I know it's a common abbreviation in everyday English, but it's a bit confusing in a code comment I think 🤓

   s/no/number/
   s/no./number/

4. +++ b/src/Plugin/migrate/process/PolicyConfiguration.php
   @@ -251,4 +116,153 @@ class PolicyConfiguration extends ProcessPluginBase implements ContainerFactoryP
   +      // for eg - "-14 days, -7 days, -2 days".
   

   for eg -
   →
   For example:

follow up for #23.

Looking good!

P.S.: could you maybe post the before → after screenshots for both branches here too that you showed me? 🤞

7.x-1.x:
migrates to

.

7.x-2.x:

migrates to

Above screenshots shows, migration for booth 7.x-1.x and 7.x-2.x branches.

Yay, thanks!

Just tested it and it looks good for both versions. Thank you all!

Wonderful! Any chance there will be a new release soon that includes this? 🤓

Yes, I'll create it next week.
#3305839: Create Password Policy 8.x-3.2 release

🥳