Migrate forum pattern to taxonomy term forums if forum is enabled on the source site

1. Hm, #8's test-only patch is failing on assertNodePattern(), not on the forum pathauto pattern migration. Why is that? 🤔

2. +++ b/migrations/d7_pathauto_patterns.yml
   @@ -8,14 +8,45 @@ source:
   -  label: label
   -  type: type
   -  pattern: pattern
   -  selection_criteria: selection_criteria
   +  entity_type:
   +    plugin: skip_on_empty
   +    source: entity_type
   +    method: row
   +  bundle:
   +    plugin: skip_on_empty
   +    source: bundle
   +    method: process
   +  label:
   +    plugin: pathauto_pattern_label
   +    source:
   +      - '@entity_type'
   +      - '@bundle'
   +  type:
   +    plugin: concat
   +    source:
   +      - constants/type_prefix
   +      - '@entity_type'
   +    delimiter: ':'
   +  pattern:
   +    plugin: callback
   +    source: value
   +    callable: unserialize
   +  selection_criteria:
   +    -
   +      plugin: skip_on_empty
   +      method: process
   +      source: '@bundle'
   +    -
   +      plugin: pathauto_pattern_selection_criteria
   +      source:
   +        - '@entity_type'
   +        - '@bundle'
   

   🤔 So … this is all basically wrong/overly simplistic in HEAD? 🤯

3. +++ b/migrations/d7_pathauto_patterns.yml
   @@ -8,14 +8,45 @@ source:
   +  weight: weight
   

   🤔 … and this was missing?! 🙈

4. +++ b/pathauto.api.php
   @@ -157,3 +157,24 @@ function hook_pathauto_punctuation_chars_alter(array &$punctuation) {
   +function hook_pathauto_migration_pattern_variable_id_map_alter(array &$map) {
   +  // In Drupal 7, there was a "pathauto_blog_pattern" variable which cannot
   +  // be migrated into a Drupal 8 or Drupal 9 pathauto pattern, because in later
   +  // Drupal versions there is no such a think like "blog" in Drupal 7.
   +  // However, we have a custom entity (without bundles) which matches.
   +  $map += [
   +    'blog' => [
   +      'entity_type' => 'custom_entity',
   +    ],
   +  ];
   +
   +  // We want to migrate the "forum" pattern ID to "node"s with type "forum".
   +  $map['forum'] = [
   +    'entity_type' => 'node',
   +    'bundle' => 'forum',
   +  ];
   +}
   

   🤩👏 Absolutely superb comments for this very tricky thing.

   🤔 But I wonder … why is this in a sample hook implementation? Why is this not applied automatically, at least the forum node example?

5. +++ b/src/Plugin/migrate/process/PathautoPatternLabel.php
   @@ -0,0 +1,108 @@
   + * @MigrateProcessPlugin(
   + *   id = "pathauto_pattern_label"
   + * )
   

   👍 Looks great!

6. +++ b/src/Plugin/migrate/process/PathautoPatternSelectionCriteria.php
   @@ -0,0 +1,51 @@
   + * @MigrateProcessPlugin(
   + *   id = "pathauto_pattern_selection_criteria"
   + * )
   

   👍 Looks great!

7. +++ b/src/Plugin/migrate/process/PathautoPatternSelectionCriteria.php
   @@ -0,0 +1,51 @@
   +      throw new MigrateSkipProcessException('The entity_type, the bundle or both sources are missing');
   

   🤓 I think s/sources// would be clearer.

8. +++ b/src/Plugin/migrate/source/PathautoPattern.php
   @@ -18,21 +19,41 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
   +  protected $variableIdEntityTypeMap = [
   +    'forum' => [
   +      'entity_type' => 'taxonomy_term',
   +      'bundle' => 'forums',
   +    ],
   +  ];
   

   🤔 Referring back to my point 4 — why is this in here but not that other example?

   Also, this could use an @see to that hook 🤓

9. +++ b/src/Plugin/migrate/source/PathautoPattern.php
   @@ -55,10 +76,71 @@ class PathautoPattern extends DrupalSqlBase {
   +    // Exclude forum pattern if forum wasn't enabled on the source site.
   +    if (!$this->moduleExists('forum')) {
   +      $query->condition('name', 'pathauto_forum_pattern', '<>');
   +    }
   

   🤔 So … this pattern would exist even if the forum module was not enabled on the source site?

10. +++ b/src/Plugin/migrate/source/PathautoPattern.php
    @@ -55,10 +76,71 @@ class PathautoPattern extends DrupalSqlBase {
    +        // This loop tries to find the destination entity type. If the ID is
    +        // "taxonomy_term_tags", then for first, it tries to find a content
    +        // entity definition with ID "taxonomy", then "taxonomy_term", and
    +        // finally "taxonomy_term_tags".
    

    👍 Nice heuristic!

11. +++ b/src/Plugin/migrate/source/PathautoPattern.php
    @@ -55,10 +76,71 @@ class PathautoPattern extends DrupalSqlBase {
    +      // "Default" patterns (which do not have entity bundle restriction) should
    +      // get higher weight to act as a fallback.
    +      $rows[] = $row + [
    +        'weight' => !empty($row['bundle']) ? 0 : 1,
    +      ];
    

    👍

    🤔 I presume that the D7 pathauto module already functioned this way, and in D8 it's just more explicit?

1. +++ b/src/Plugin/migrate/process/PathautoPatternSelectionCriteria.php
   @@ -31,7 +31,7 @@ class PathautoPatternSelectionCriteria extends ProcessPluginBase {
   -      throw new MigrateSkipProcessException('The entity_type, the bundle or both sources are missing');
   +      throw new MigrateSkipProcessException('The entity_type, the bundle or both source are missing');
   

   🤓This did not address #10.7 — I asked "sources" to be removed.

2. 🙏 #11.9: can you add that as a comment then?
3. 🙈 #11.4 & #11.8: OOPS! And … much better! 👍
4. 👍#12 & #13: nice 🤓👍
5. 🤔
   #14: I don't understand this — PathautoPattern::count() inherited \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::count(), which calls \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::doCount(), which does

     protected function doCount() {
       $iterator = $this->getIterator();
       return $iterator instanceof \Countable ? $iterator->count() : iterator_count($this->initializeIterator());
     }
   

5. 🤔
   #14: I don't understand this — PathautoPattern::count() inherited \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::count(), which calls \Drupal\migrate\Plugin\migrate\source\SourcePluginBase::doCount(), which does

     protected function doCount() {
       $iterator = $this->getIterator();
       return $iterator instanceof \Countable ? $iterator->count() : iterator_count($this->initializeIterator());
     }
   

Oops! This is wrong! @huzooka pointed out that PathautoPattern::count() will result in a call to \Drupal\migrate\Plugin\migrate\source\SqlBase::doCount(), which does do what Zoltan's comment in #14 described.

So … that means the only remark I have left is that silly nitpick in #15.1. Obviously that's non-essential.

So … 🚢🥳

Thanks for all the work on pathauto migrate stuff and the extensive reviews.

In theory, there still is forum specific integration for forum aliases but it's broken, also because core has been broken since forever, so I'm not completely sure about this. If we do this and decide that forum specific integration is no longer necessary/useful, shouldn't we also remove that? There are quite a few issues and it's causing confusion.

I know it is a about the pattern migration. My point is that \Drupal\pathauto\Plugin\pathauto\AliasType\ForumAliasType *does* exist, but it doesn't work because core's forum prefix is broken. It's *supposed* to be forum/ID, not taxonomy/term/ID but has never worked in D8. See #2010132: Canonical taxonomy term link for forum vocabulary is broken. And as a result, our plugin is also broken (likely not just because of that core issue).

As a result, it is challenging to review/test/accept changes in this very broken context and I'm not quite sure I understand what this issue is doing exactly and what the overall intent is.

#20++

Patch rerolled for failing test on 9.3.0

Rerolled.

Rerolled to adjust for some fuzz.

Might one of the maintainers be willing to review this and provide feedback, especially around requirements for getting this committed? Thank you.

Patch is failing to apply for me on Drupal 10.2.3 + 8.x-1.12 -- attempt at rerolling ... but something not quite right. This patch was made against 8.x-1.12, which explains why it is failing on 8.x-1.x. Things have changed.

#27: Right — we only need MigratePathautoTest to pass on 1.12 for this to be incorporated into #3424401: D10: Update recommendation for Pathauto (drupal/pathauto).

(There seems to be little point in making this work against 8.x-1.x because it's been green multiple times over the past few years and the maintainer hasn't committed it.)

We can't ask DrupalCI or GitLab CI to test against a specific tag very easily, so just sharing the output of running the test suite locally will suffice 👍

> (There seems to be little point in making this work against 8.x-1.x because it's been green multiple times over the past few years and the maintainer hasn't committed it.)

The issue wasn't RTBC for 2 years.

I'm still confused about the focus on this, soon enough forum won't even be in core anymore. It's literally a 1000 lines of code (that I'll have to maintain and keep working forever) to migrate what's essentially a single pattern for the few sites that are using forum module that will usually take a few minutes to set up again. A lot of that is tests but still. I've spent quite some time on fixing Migrate tests in various modules already for D9/D10 compatibility and it's one thing if there's data involved that would actually be a lot of work, but for something like pathauto, I'm struggling to see the benefit.

I've personally never did a ull 1:1 migration of a D7 project. If people see a benefit in maintaining migrations for this, then it might be better suited in a separate project?

+++ b/src/Plugin/migrate/process/PathautoPatternSelectionCriteria.php
@@ -0,0 +1,51 @@
+    return [
+      [
+        'id' => ($entity_type == 'node') ? 'node_type' : 'entity_bundle:' . $entity_type,
+        'bundles' => [$bundle => $bundle],

this is outdated and doesn't work anymore on D10. node_type no longer exists.

Oh yes, sorry — it was not meant in a negative way at all. I think it's fine that it's up to us to keep maintaining this 👍😊

(This is meant for those people for whom writing the code/YAML files to use the Drupal core migration system is unrealistic.)

I'd like for this to remain an open issue here for now to increase the odds of such non-expert users discovering it.

P.S.: the value of this issue alone seems limited for sure, but it's a blocker for #3179865: [PP-1] Derive pathauto pattern migrations to solve inaccurate pattern migration dependencies, and that is what brings the real value!

Attaching reroll with updates to tests to avoid the following errors:

PHP Fatal error:  Declaration of Drupal\Tests\pathauto\Kernel\Migrate\d7\MigratePathautoTest::setUp() must be compatible with Drupal\Tests\migrate_drupal\Kernel\d7\MigrateDrupal7TestBase::setUp(): void in /app/docroot/modules/contrib/pathauto/tests/src/Kernel/Migrate/d7/MigratePathautoTest.php on line 91

Drupal\Tests\pathauto\Kernel\Migrate\d7\MigratePathautoTest::testPathautoMigrations with data set "Disabled forum on source site" (false)
Error: Call to undefined function Drupal\Tests\pathauto\Kernel\Migrate\d7\drupal_get_path()

Hey Dan!! I know you.

The node_type condition plugin was deprecated since Drupal 9.3 in favor of a generalized entity_bundle:* condition plugin.

This pathauto issue applies this change to 1.x branch.

The patch in this issue still references the removed node_type condition plugin and results in test failures.

The attached patch replaces references to the node_type condition plugin with references to the generalized entity_bundle:node condition plugin.

It seems that this issue combines 4 pathauto 1.x issues including this one. So I applied the same change to that issue to make the combined patch pass tests.