[PP-2] Derive path alias migrations per entity type (and bundle)

1. +++ b/core/modules/path/path.module
   @@ -156,3 +157,227 @@ function path_field_widget_form_alter(&$element, FormStateInterface $form_state,
   +  $optional_migration_dependencies = [];
   ...
   +        // If this migration's entity type does not have a corresponding path
   +        // alias migration, it will be used as optional dependency.
   +        if (!isset($optional_migrations_per_entity_type[$entity_type_id])) {
   +          $optional_migration_dependencies[] = $migration_plugin_id;
   +        }
   ...
   +      // If this path alias migration is not entity type specific, add the
   +      // optional migration dependencies.
   +      if (empty($migration_plugin['source']['entity_type_id'])) {
   +        $migration_plugin['migration_dependencies']['optional'] = array_unique(array_merge($migration_plugin['migration_dependencies']['optional'], $optional_migration_dependencies));
   

   🤔 I was really confused by what $optional_migration_dependencies was for.

   But I think I get it now!

   It's about the optional migration dependencies for the d*_url_alias migration's "catch-all" derivative.

   I think a variable name like $other_entity_destination_migrations would have made this a lot clearer.

2. +++ b/core/modules/path/path.module
   @@ -156,3 +157,227 @@ function path_field_widget_form_alter(&$element, FormStateInterface $form_state,
   +        // Track which entity type IDs should be used as required dependencies
   +        // instead of optional. Here we just pre-populate with an empty array.
   

   🤔 This comment talks about required dependencies, but then the code it comments does not do anything with required dependencies?

3. +++ b/core/modules/path/path.module
   @@ -156,3 +157,227 @@ function path_field_widget_form_alter(&$element, FormStateInterface $form_state,
   +      $destination_plugin_parts = explode(':', $migration['destination']['plugin']);
   +      $entity_destination_plugins = ['entity', 'entity_complete'];
   +      $entity_type_id = in_array($destination_plugin_parts[0], $entity_destination_plugins, TRUE) ?
   +        $destination_plugin_parts[1] : NULL;
   

   🤔 I find this pretty difficult to understand.

   Why only entity and entity_complete? Why not node and node_complete? And isn't it d7_node_complete for examle?

4. +++ b/core/modules/path/path.module
   @@ -156,3 +157,227 @@ function path_field_widget_form_alter(&$element, FormStateInterface $form_state,
   +      // Track which migrations have a locatable entity type as their
   +      // destination.
   +      if (!$entity_type_id || !$entity_type_definition = $entity_type_manager->getDefinition($entity_type_id, FALSE)) {
   +        continue;
   +      }
   +
   +      // Skip entity types that are not content entities.
   +      if (!is_subclass_of($entity_type_definition->getClass(), ContentEntityInterface::class)) {
   +        continue;
   +      }
   

   The first comment belongs on the second if-statement.

   The first if-statement needs a comment like // Only modify migrations with an entity destination.

5. +++ b/core/modules/path/path.module
   @@ -156,3 +157,227 @@ function path_field_widget_form_alter(&$element, FormStateInterface $form_state,
   +      // Skip path alias migrations.
   +      if ($entity_type_id === 'path_alias') {
   +        continue;
   +      }
   

   👍🤣 Imagine, aliases for aliases!

6. +++ b/core/modules/path/path.module
   @@ -156,3 +157,227 @@ function path_field_widget_form_alter(&$element, FormStateInterface $form_state,
   +      // Get only those link templates that use the entity type ID as a
   +      // parameter; so we don't care about collection or create templates.
   +      $link_templates_filtered = array_filter($entity_type_definition->getLinkTemplates(), function (string $template) use ($entity_type_id) {
   +        return mb_strpos($template, '{' . $entity_type_id . '}') !== FALSE;
   +      });
   

   👍 I was gonna ask: why not just care about the canonical link template?

   But this is much more robust! 👏

7. +++ b/core/modules/path/path.module
   @@ -156,3 +157,227 @@ function path_field_widget_form_alter(&$element, FormStateInterface $form_state,
   +        // For a not derived path alias migration, we need all of the discovered
   +        // content entity migrations as dependency.
   +        $locatable_entity_type_destination_migration_ids[] = $migration_plugin_id;
   

   Rather than this comment, I think you could write

   // @todo Remove this when the d6_url_alias migration also is being derived.
   

8. +++ b/core/modules/path/path.module
   @@ -156,3 +157,227 @@ function path_field_widget_form_alter(&$element, FormStateInterface $form_state,
   +    // The d6_url_migration is not derived yet.
   

   🐛d6_url_migration → d6_url_alias migration

9. +++ b/core/modules/path/src/Plugin/migrate/D7PathAliasDeriver.php
   @@ -0,0 +1,319 @@
   +      if (!$definition instanceof ContentEntityTypeInterface) {
   +        continue;
   +      }
   

   👍 This matches the logic of \Drupal\jsonapi\ResourceType\ResourceTypeRepository::isLocatableResourceType().

10. +++ b/core/modules/path/src/Plugin/migrate/D7PathAliasDeriver.php
    @@ -0,0 +1,319 @@
    +    if (!$source instanceof DrupalSqlBase) {
    +      return $this->derivatives;
    +    }
    

    When can this happen?

    Why is it okay to return early?

    This needs some documentation :)

11. +++ b/core/modules/path/src/Plugin/migrate/D7PathAliasDeriver.php
    @@ -0,0 +1,319 @@
    +        $supported_entity_types = [
    +          'node',
    +          'taxonomy_term',
    +          'user',
    +        ];
    

    🤓 Let's make this a class constant.

12. +++ b/core/modules/path/src/Plugin/migrate/D7PathAliasDeriver.php
    @@ -0,0 +1,319 @@
    +        // Construct the query.
    

    This comment is pointing out something pretty obvious — this can be removed?

13. +++ b/core/modules/path/src/Plugin/migrate/D7PathAliasDeriver.php
    @@ -0,0 +1,319 @@
    +              $aliases_grouped_by_bundle = array_reduce($base_query->execute()->fetchAllAssoc('pid'), function ($carry, $row) {
    +                $carry[$row->type]['label'] = $row->name;
    +                $carry[$row->type]['pids'][$row->pid] = $row->pid;
    +                return $carry;
    +              });
    

    😍👏 Very elegant!

14. +++ b/core/modules/path/src/Plugin/migrate/D7PathAliasDeriver.php
    @@ -0,0 +1,319 @@
    +              // Get all of the path aliases that's source is a taxonomy term
    

    s/that's/whose/

15. +++ b/core/modules/path/src/Plugin/migrate/D7PathAliasDeriver.php
    @@ -0,0 +1,319 @@
    +          // per-entity-type URL alias migration. Dependency metadata added by path_migration_plugins_alter().
    

    80 cols 🤓

16. +++ b/core/modules/path/src/Plugin/migrate/D7PathAliasDeriver.php
    @@ -0,0 +1,319 @@
    +      // Add the opposite of the links template specific conditions we added for
    +      // the entity_type specific "$base_query": operator is "NOT LIKE", and the
    +      // conjunction is "AND". Since "AND" is the default, we don't need a
    +      // separate condition group in this case.
    +      foreach ($excluded_bundles as $entity_type_id) {
    +        if (!isset($this->linkTemplates[$entity_type_id]) || !is_array($this->linkTemplates[$entity_type_id])) {
    +          continue;
    +        }
    +
    +        foreach ($this->linkTemplates[$entity_type_id] as $link_template) {
    +          $pattern = '/(' . self::ENTITY_TYPE_PARAMETER . '|' . self::EXTRA_PARAMETER . ')/';
    +          $template_raw = preg_replace($pattern, '%', $source->getDatabase()->escapeLike($link_template));
    +          $uncovered_aliases_query->condition('ua.source', $template_raw, 'NOT LIKE');
    +        }
    +      }
    

    😍👏

17. +++ b/core/modules/path/src/Plugin/migrate/source/d7/UrlAlias.php
    @@ -14,6 +14,19 @@
    +    if (!empty($this->configuration['pids']) && is_array($this->configuration['pids'])) {
    +      $query->condition('ua.pid', $this->configuration['pids'], 'IN');
    +    }
    

    🤔 Couldn't this result in hundreds or even tens of thousands of values in that IN expression in the SQL query? That query would fail.

18. Found 6 path_alias entities, expected 8.
    

    I did not expect this would change the number of path_alias entities. This sounds like a bug? 🤔

Everything in #4 is superficial, except for points 17 and 18. I wonder if point 18 is caused by point 3.

+++ b/core/modules/path/src/Plugin/migrate/D7PathAliasDeriver.php
@@ -0,0 +1,319 @@
+      // Only add the "Other URL aliases" migration if it makes sense.
...
+        $this->derivatives['other']['label'] = $this->t('Other and unmapped @label', [

I think a label like Non-entity path aliases would make more sense.

I found and fixed a dependency problem. :)

+++ b/core/modules/path/src/Plugin/migrate/source/d7/UrlAlias.php
@@ -14,14 +15,52 @@
-    if (!empty($this->configuration['pids']) && is_array($this->configuration['pids'])) {
-      $query->condition('ua.pid', $this->configuration['pids'], 'IN');
+    if (!empty($this->configuration['entity_type_id']) && in_array($this->configuration['entity_type_id'], $this->supportedEntityTypes, TRUE)) {
+      $entity_type_id = $this->configuration['entity_type_id'];
+      $bundle_id = $this->configuration['bundle'] ?? NULL;
+
+      $this->addEntityTypeRestrictions($query, $entity_type_id);
+
+      if ($bundle_id) {
+        switch ($entity_type_id) {
+          case 'node':
+            // We want to get a per-node-type URL alias migration. So we
+            // inner join the base query on node table based on the link
+            // templates of nodes.
+            $join_conditions = $this->getJoinConditions('node', 'n.nid');
+            $query->join('node', 'n', implode(' OR ', $join_conditions['conditions']), $join_conditions['args']);
+            $query->condition('n.type', $bundle_id);
+            break;
+
+          case 'taxonomy_term':
+            // Join the taxonomy term data table to the base query; based on
+            // the link templates of taxonomy term entities.
+            $join_conditions = $this->getJoinConditions('taxonomy_term', 'ttd.tid');
+            $query->join('taxonomy_term_data', 'ttd', implode(' OR ', $join_conditions['conditions']), $join_conditions['args']);
+            $query->fields('ttd', ['vid']);
+            // Since the "taxonomy_term_data" table contains only the taxonomy
+            // vocabulary ID, but not the vocabulary name, we have to inner
+            // join the "taxonomy_vocabulary" table as well.
+            $query->join('taxonomy_vocabulary', 'tv', 'ttd.vid = tv.vid');
+            $query->condition('tv.machine_name', $bundle_id);
+            break;
+        }
+      }
+    }
+    elseif (!empty($this->configuration['excluded_entity_type_ids'])) {
+      // Add the opposite of the links template specific conditions we added for
+      // the entity_type specific "$base_query": operator is "NOT LIKE", and the
+      // conjunction is "AND". Since "AND" is the default, we don't need a
+      // separate condition group in this case.
+      $this->addExcludedEntityTypeRestrictions($query, $this->configuration['excluded_entity_type_ids']);
     }

👏

This rebases the patch on latest 9.0.x and massively improves query performance.

For a site with 28K nodes, 50K path aliases and 7 node types, this makes the computing of the per-node-bundle derivatives d7_url_alias:node:* go down from 61 seconds to 20 milliseconds — or about 3000 times faster!

With #15 solved, \Drupal\path\Plugin\migrate\source\d7\UrlAlias::count() was still super slow. And in the project I'm working on, we're hitting that all the time.

But … public function count($refresh = FALSE) is the signature. The refresh is optional. And if the source DB doesn't change after discovery time, then the counts stay the same. Clearing all caches after refreshing the source DB is no big deal obviously.

So … YAY, rather than doing expensive queries and trying to optimize those (which I first did: I was bringing the same more efficient query pattern from #15 to \Drupal\path\Plugin\migrate\source\d7\UrlAlias::query()), we can just use the numbers at query time! Much faster!

Oops.

Well, #17 made ::count() run in constant time, but we still need to speed up ::query() since that's actively used by the migration system also.

Applying a similar query optimization as in #17.

@huzooka queued PostgreSQL & SQLite test runs of #21, those show that the migrations are now failing on those databases. This is due to the use of SUBSTRING_INDEX(), which is MySQL-specific. Figuring out an alternative query … 🤔

This should make the patch also work on SQLite.

😬 Forgot to git add a few bits 🙈

+++ b/core/modules/path/src/Plugin/migrate/D7PathAliasDeriver.php
@@ -85,15 +85,12 @@ public function getDerivativeDefinitions($base_plugin_definition) {
+          // We assume the first link template is the canonical one and the
+          // shortest one; the one that is the subset of all others.
           $link_templates = $this->getPreparedLinkTemplates($entity_type_id);
-          $min_path_parts = 1000;
-          foreach ($link_templates as $link_template) {
-            $min_path_parts = min($min_path_parts, count(explode('/', $link_template)));
-          }
+          $base_path = str_replace($this->entityTypeParameter, '', $link_templates[0]);

@huzooka rightfully pointed out in chat to me just now that it's wrong to assume the first link template is the canonical one.

However, for the currently supported entity types (Node and Term), this assumption holds true 🙈🤓

Whenever we expand this to support more entity types (or before this patch gets committed to Drupal core), we'd need to fix this.

+++ b/core/modules/path/src/Plugin/migrate/D7PathAliasDeriver.php
@@ -83,43 +82,48 @@ public function getDerivativeDefinitions($base_plugin_definition) {
             case 'taxonomy_term':
-              // Join the taxonomy term data table to the base query; based on
-              // the link templates of taxonomy term entities.
-              $join_conditions = $this->getJoinConditions('taxonomy_term', 'ttd.tid');
-              $base_query->join('taxonomy_term_data', 'ttd', implode(' OR ', $join_conditions['conditions']), $join_conditions['args']);
-              $base_query->fields('ttd', ['vid']);
-              // Since the "taxonomy_term_data" table contains only the taxonomy
-              // vocabulary ID, but not the vocabulary name, we have to inner
-              // join the "taxonomy_vocabulary" table as well.
-              $base_query->join('taxonomy_vocabulary', 'tv', 'ttd.vid = tv.vid');
-              $base_query->fields('tv', ['machine_name', 'name']);
-
-              // Get all of the path aliases whose source is a taxonomy term
-              // URL.
-              $aliases_grouped_by_bundle = array_reduce($base_query->execute()->fetchAllAssoc('machine_name'), function (array $carry, $row) {
+              $query = $db->select('taxonomy_term_data', 'ttd')
+                ->fields('ttd', ['vid']);
+              $query->innerJoin('taxonomy_vocabulary', 'tv', 'ttd.vid = tv.vid');
+              $query->fields('tv', ['machine_name', 'name']);
+              $query->condition('ttd.tid', $inner_select, 'IN');
+              $query->groupBy('vid');
+              $query->groupBy('machine_name');
+              $query->groupBy('name');
+              // @todo use the count?
+              $query->addExpression('COUNT(*)', 'count');
+              $aliases_grouped_by_bundle = array_reduce($query->execute()->fetchAllAssoc('machine_name'), function (array $carry, $row) {
                 $carry[$row->machine_name] = $row->name;
                 return $carry;
               }, []);

This introduced a regression.

It only counts the unique number of terms of a vocabulary that have an URL alias.

That means it fails to count every URL alias for terms in a given vocabulary.

In other words: it always counts a single URL alias per term, even if a term has lots of historical path aliases.

The consequence when combined with the __count_at_discovery_time optimization is that the reported total count can be lower than the imported count! 🙃🙈

This interdiff fixes that, and reverts a bunch of the now obsolete changes that I introduced in #15.

+++ b/core/modules/path/path.module
@@ -156,3 +158,253 @@ function path_field_widget_form_alter(&$element, FormStateInterface $form_state,
+      // Skip path alias migrations.
+      if ($entity_type_id === 'path_alias') {
+        continue;
+      }

We should also skip redirect migrations (for https://www.drupal.org/project/redirect).

#17 was true at the time but now with #3190815: Source count caching broken: impossible to enable source count caching for SqlBase-based source plugins (plus, unneeded cache I/O), a better/simpler approach is possible: let the migration system's built-in source count caching take care of this!

Oops. That included a *.orig file 🙈

The patch in #37 is incorrect. The interdiff is correct though.

Ah, apparently this conflicts with #3096951: d7_node migration should have dependency on d7_node_title_label migration 😬Which the migration maintainers have shot down. Ah well, then there won't be a patch that applies.

Added reroll of patch #38.

The patches so far generate optional dependencies on d7_shortcut, d7_custom_block, et cetera. This does not make sense, because they only have admin-facing URLs.