[PP-1] Derive pathauto pattern migrations to solve inaccurate pattern migration dependencies

1. +++ b/src/Plugin/migrate/PathautoPatternDeriver.php
   @@ -0,0 +1,86 @@
   +        $derivative_id = $bundle
   +          ? implode(PluginBase::DERIVATIVE_SEPARATOR, [
   +            $entity_type,
   +            $bundle,
   +          ])
   +          : "{$entity_type}_default";
   

   🤔 So when no *_default as a derivative ID. Sounds reasonable!

   OTOH … why even rename it in this case? Why not keep the original plugin ID? Isn't there only ever going to be one?

2. +++ b/src/Plugin/migrate/PathautoPatternDeriver.php
   @@ -0,0 +1,86 @@
   +        if ($bundle) {
   +          switch ($entity_type) {
   +            case 'node':
   +              $migration_requirements = ['d7_node_type'];
   +              break;
   +
   +            case 'taxonomy_term':
   +              $migration_requirements = ['d7_taxonomy_vocabulary'];
   +              break;
   +          }
   +        }
   

   Let's add default: and add explicit logging?

   That way the person doing the migration would be warned about missing dependencies (for bundle definitions for entity types whose bundle creation migration is not known). It could even check \Drupal\Core\Entity\EntityTypeInterface::getBundleEntityType() to not show this message when bundles are not configurable but hardcoded.

3. +++ b/src/Plugin/migrate/source/PathautoPattern.php
   @@ -86,6 +93,38 @@ class PathautoPattern extends DrupalSqlBase implements ContainerFactoryPluginInt
   +      // Fetch all pattern variables whose value is not a serialized empty
   +      // string.
   +      $query->condition('name', 'pathauto_%_pattern', 'LIKE');
   

   I don't think this comment matches the query? 😅

4. +++ b/src/Plugin/migrate/source/PathautoPattern.php
   @@ -86,6 +93,38 @@ class PathautoPattern extends DrupalSqlBase implements ContainerFactoryPluginInt
   +      // This is the default condition.
   +      $condition = $query->orConditionGroup();
   

   🤔 What does this mean? You're OR'ing with the default condition I think?

5. +++ b/src/Plugin/migrate/source/PathautoPattern.php
   @@ -86,6 +93,38 @@ class PathautoPattern extends DrupalSqlBase implements ContainerFactoryPluginInt
   +      // Source variable IDs can be mapped to the same "entity_type" and
   

   🤔 What is a "source variable ID"?

6. +++ b/src/Plugin/migrate/source/PathautoPattern.php
   @@ -86,6 +93,38 @@ class PathautoPattern extends DrupalSqlBase implements ContainerFactoryPluginInt
   +      // "bundle". At the end, this can cause that there are multiple patterns
   +      // for the same entity type and bundle.
   

   🤯 Woah! How? Can you include an example in this comment? 🙏

7. +++ b/src/Plugin/migrate/source/PathautoPattern.php
   @@ -123,9 +162,13 @@ class PathautoPattern extends DrupalSqlBase implements ContainerFactoryPluginInt
   +        // If entity type was undeterminable, skip this row.
   +        if (empty($row['entity_type'])) {
   +          continue;
   +        }
   

   When/how could this happen? Including an example here might be valuable 🙏

8. +++ b/tests/src/Kernel/Migrate/d7/MigratePathautoTest.php
   @@ -83,6 +81,11 @@ class MigratePathautoTest extends MigrateDrupal7TestBase {
   +      'd7_pathauto_patterns:taxonomy_term',
   +      'd7_pathauto_patterns:taxonomy_term_default',
   

   Why do these two happen in this order?

9. +++ b/tests/src/Kernel/Migrate/d7/MigratePathautoTest.php
   @@ -92,6 +95,11 @@ class MigratePathautoTest extends MigrateDrupal7TestBase {
   +    // Execute the rest of the migrations. For now, this is equal with
   

   🤓 s/with/to/

#9

1. 👍 That's clear, thanks!
2. 🙏 Aha! That makes sense. Then let's A) document this explicitly, B) enforce this — and we can do both simultaneously by adding
   

     default:
       throw new \LogicException('pathauto in Drupal 7 only supports bundle-specific patterns for nodes and taxonomy terms.');
   

   Defensive programming FTW 😊

3. 👍

#9.8: it just looks odd to me to first import :foo and then :foo_default. It's still not entirely clear to me what the difference is.

Review

1. 🤓
   

   +++ b/src/Plugin/migrate/source/PathautoPattern.php
   @@ -31,6 +48,26 @@ class PathautoPattern extends DrupalSqlBase implements ContainerFactoryPluginInt
   +        throw new \LogicException(sprintf('If "bundle" configuration is set for %s migration source plugin, "entity_type" configuration has also be defined.', get_class($this)));
   

   s/has also be/must also be/

2. 🤓
   

   +++ b/src/Plugin/migrate/source/PathautoPattern.php
   @@ -52,8 +52,7 @@ class PathautoPattern extends DrupalSqlBase implements ContainerFactoryPluginInt
   +      // Fetch every pattern variables.
   

   s/every pattern variables/every pattern variable/

RE #9.8: @huzooka clarified that d7_pathauto_patterns:taxonomy_term results in all bundle-specific derivatives getting executed, so for example:

• d7_pathauto_patterns:taxonomy_term:tags
• d7_pathauto_patterns:taxonomy_term:sujet_de_discussion

And only after those d7_pathauto_patterns:taxonomy_term_default gets migrated. This is the fallback pattern. So … it totally makes sense 👍

RTBC once the blocker lands 🚢

I'm tempted to mark this a bug instead of a feature request — the issue summary describes pretty clearly why this is a bug.

Problem

+++ b/src/Plugin/migrate/PathautoPatternDeriver.php
@@ -0,0 +1,89 @@
+        if ($bundle) {
+          switch ($entity_type) {
+            case 'node':
+              $migration_requirements = ['d7_node_type'];
+              break;
+
+            case 'taxonomy_term':
+              $migration_requirements = ['d7_taxonomy_vocabulary'];
+              break;
+
+            default:
+              throw new \LogicException('Drupal 7 Pathauto only supports bundle-specific patterns for nodes and taxonomy terms.');
+          }
+        }

When the source site has installed file_entity, they'll also have bundles for the file entity type.

On a particular site I am testing against, I'm seeing the following Pathauto patterns in the variable table:

• pathauto_file_audio_pattern
• pathauto_file_document_pattern
• pathauto_file_image_pattern
• pathauto_file_pattern
• pathauto_file_video_pattern

which results in this error:

LogicException: Drupal 7 Pathauto only supports bundle-specific patterns for nodes and taxonomy terms. in Drupal\pathauto\Plugin\migrate\PathautoPatternDeriver->getDerivativeDefinitions() (line 75 of modules/pathauto/src/Plugin/migrate/PathautoPatternDeriver.php).
…
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
The website encountered an unexpected error. Please try again later.

Proposals

1️⃣ First, to make this easier to debug, this should at the very least be throwing a more helpful exception, like so:

-              throw new \LogicException('Drupal 7 Pathauto only supports bundle-specific patterns for nodes and taxonomy terms.');
+              var_dump($source);
+              throw new \LogicException(sprintf('Drupal 7 Pathauto only supports bundle-specific patterns for nodes and taxonomy terms. Now observed %s bundle for %s entity type.', $bundle, $entity_type));

2️⃣ Second, it should not crash, but instead it should just continue without adding dependencies.

3️⃣ Third, let's add explicit support for file entities?

Manually tested #18. I can confirm it works.

Review

+++ b/src/Plugin/migrate/PathautoPatternDeriver.php
@@ -48,6 +90,13 @@ class PathautoPatternDeriver extends DeriverBase {
+        if (
+          !$pathauto_entity_installed_on_source &&
+          !in_array($entity_type, ['node', 'taxonomy_term', 'user'], TRUE)
+        ) {
+          continue;
+        }

Isn't this returning too early? Doesn't this mean that the new file and comment cases you added later in this file will never get executed?

Looks good 👍

+++ b/src/Plugin/migrate/source/PathautoPattern.php
@@ -164,6 +173,13 @@ class PathautoPattern extends DrupalSqlBase implements ContainerFactoryPluginInt
+      // If the current 'bundle' config is FALSE, then we want to skip results
+      // which have bundle value, because this derivative should only migrate
+      // the entity type's default (fallback) pattern.
+      if ($this->configuration['bundle'] === FALSE && !empty($row['bundle'])) {

🤓 Nit.

So this means we assign different meanings to bundle === NULL (the default value, pre-existing) and bundle === FALSE (introduced here).

We should document that in \Drupal\pathauto\Plugin\migrate\source\PathautoPattern::__construct() IMHO, or better yet, in the class-level docblock.

Could someone please clarify this part:

+      // Add the original simpletest prefix so SQLite can attach its database.
+      // @see \Drupal\Core\Database\Driver\sqlite\Connection::init()
+      $connection_info[$target]['prefix'][$value['prefix']['default']] = $value['prefix']['default'];

That class method doesn't exist and this change causes the tests to fail with the error "Illegal string offset 'default'".

Correction, it fails on this bit:

      // Simpletest uses 7 character prefixes at most so this can't cause
      // collisions.
      $connection_info[$target]['prefix']['default'] = $prefix . '0';

$connection_info[$target]['prefix'] is a string, not an array.

This change lets the tests pass:

diff --git a/tests/src/Kernel/Plugin/migrate/source/PathautoSourceTestBase.php b/tests/src/Kernel/Plugin/migrate/source/PathautoSourceTestBase.php
index 89d4a60..3a8c402 100644
--- a/tests/src/Kernel/Plugin/migrate/source/PathautoSourceTestBase.php
+++ b/tests/src/Kernel/Plugin/migrate/source/PathautoSourceTestBase.php
@@ -85,11 +85,11 @@ private function createMigrationConnection() {
       $prefix = is_array($value['prefix']) ? $value['prefix']['default'] : $value['prefix'];
       // Simpletest uses 7 character prefixes at most so this can't cause
       // collisions.
-      $connection_info[$target]['prefix']['default'] = $prefix . '0';
+      $connection_info[$target]['prefix'] = $prefix . '0';
 
       // Add the original simpletest prefix so SQLite can attach its database.
       // @see \Drupal\Core\Database\Driver\sqlite\Connection::init()
-      $connection_info[$target]['prefix'][$value['prefix']['default']] = $value['prefix']['default'];
+      // $connection_info[$target]['prefix'][$value['prefix']['default']] = $value['prefix']['default'];
     }
     Database::addConnectionInfo('migrate', 'default', $connection_info['default']);
   }

Looking at it again I spotted this:

  /**
   * Changes the database connection to the prefixed one.
   *
   * @see \Drupal\Tests\migrate\Kernel\MigrateTestBase::createMigrationConnection()
   *
   * @todo Refactor when core doesn't use global.
   *   https://www.drupal.org/node/2552791
   */
  private function createMigrationConnection() {

#2552791 was closed as a duplicate of #2681869 which was committed in 2016. Does this mean that other changes are needed in core, or that the code needs to be refactored to account for core changes from 2016? The comment isn't clear.

Putting this back to "needs work" due to my findings above.