D10: Update recommendation for Pathauto (drupal/pathauto)

After adding the snippet and regenerating the recommendations file, I ran the acli command and was met with:

  - Applying patches for drupal/pathauto
    https://www.drupal.org/files/issues/2022-01-06/pathauto-migrate_forum_pattern_if_forum_is_enabled_on_source-3179835-22--complete.patch ([AMA_MIGRATE_FIX] Issue #3179835: Migrate forum pattern to taxonomy term forums if forum is enabled on the source site)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2022-01-06/pathauto-migrate_forum_pattern_if_forum_is_enabled_on_source-3179835-22--complete.patch
    https://www.drupal.org/files/issues/2021-04-06/pathauto-derive_pathauto_migrations_per_entity_type-3179865-25.patch ([AMA_MIGRATE_FIX] Issue #3179865: Derive pathauto pattern migrations to solve inaccurate pattern migration dependencies)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2021-04-06/pathauto-derive_pathauto_migrations_per_entity_type-3179865-25.patch
    https://www.drupal.org/files/issues/2022-01-06/pathauto-migrate_multilingual_patterns-3182708--12.patch ([AMA_MIGRATE_FIX] Issue #3182708: Migrate language-specific patterns)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2022-01-06/pathauto-migrate_multilingual_patterns-3182708--12.patch
    https://www.drupal.org/files/issues/2022-01-06/pathauto-prevent_losing_custom_aliases-3079275-27--combined--do-not-test.patch ([AMA_MIGRATE_FIX] Issue #3079275: Custom aliases (which are not generated with the actual patterns) can be lost during the migration)
    https://www.drupal.org/files/issues/2021-01-05/3190980-2.patch ([AMA_MIGRATE_FIX] Issue #3190980: [PP-1] Allow source counts to be cached: implement ::doCount() instead of ::count())
   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2021-01-05/3190980-2.patch

Issue #1:

  - Applying patches for drupal/pathauto
    https://www.drupal.org/files/issues/2022-01-06/pathauto-migrate_forum_pattern_if_forum_is_enabled_on_source-3179835-22--complete.patch ([AMA_MIGRATE_FIX] Issue #3179835: Migrate forum pattern to taxonomy term forums if forum is enabled on the source site)
   Could not apply patch! Skipping. The error was: Cannot apply patch https://www.drupal.org/files/issues/2022-01-06/pathauto-migrate_forum_pattern_if_forum_is_enabled_on_source-3179835-22--complete.patch

This has a newer patch. Altering reference to https://www.drupal.org/files/issues/2023-05-08/pathauto-n3179835-25.patch → and trying again did not help the situation. I think this needs a complete re-roll. Did that here https://www.drupal.org/project/pathauto/issues/3179835#comment-15466623 ✨ Migrate forum pattern to taxonomy term forums if forum is enabled on the source site Needs review and now everything installs cleanly. Interesting...I thought I would have to re-roll a few of these but I guess the other patches must rely on the first.

www-data@2030d29ffb9a:/app/docroot/core$ ../../vendor/bin/phpunit --list-groups
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

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

I think this is something straightforward...like we are missing a void declaration in our function signatures..I updated that file locally and then hit the same issue in a number of different files:

1. PathautoPatternLabelTest
2. PathautoSourceTestBase
3. PathautoMigrateUiTest

After that was met with:

www-data@2030d29ffb9a:/app/docroot/core$ ../../vendor/bin/phpunit
PHP Fatal error:  Declaration of Drupal\Tests\pathauto\Kernel\Plugin\migrate\source\PathautoSourceTestBase::testSource(array $source_data, array $expected_data, $expected_count = null, array $configuration = [], $high_water = null) must be compatible with Drupal\Tests\migrate\Kernel\MigrateSqlSourceTestBase::testSource(array $source_data, array $expected_data, $expected_count = null, array $configuration = [], $high_water = null, $expected_cache_key = null) in /app/docroot/modules/contrib/pathauto/tests/src/Kernel/Plugin/migrate/source/PathautoSourceTestBase.php on line 162

I think this function is missing $expected_cache_key = []. I added that...then could finally run ../../vendor/bin/phpunit --group=pathauto

1. update reroll in https://www.drupal.org/project/pathauto/issues/3179835#comment-15466623 ✨ Migrate forum pattern to taxonomy term forums if forum is enabled on the source site Needs review to include new function signatures
2. update patch here - https://www.drupal.org/project/pathauto/issues/3179865 → - which needs signature updates as well
3. investigate issues with phpunit tests

Output of phpunit attached here → .

1. After adding the snippet and regenerating the recommendations file, I ran the acli command and was met with:

   This should not be possible. The patch MUST still apply. Unless you are not targeting Pathauto 1.9 anymore, because that's the version all patches applied to, and will continue to apply to for eternity.

   The MR is still empty so I can't see it.

   I bet you updated to https://www.drupal.org/project/pathauto/releases/8.x-1.11 → , because that's the first version with Drupal 10 compatibility.

2. Assuming you updated to 1.11, then yes, all patches may need to be rerolled. But as you noticed, it's also possible only some need a reroll. It simply depends on whether the patched hunks have changed upstream or not! 😊
3. RE: tests failing: yes, you'll need to update their signatures — this is due to Drupal 10 changing the base method signature, which itself happened because PHPUnit forced us to do that.

4. There were some errors that need to be reviewed. This is getting pretty confusing as these issues are not actually present in the underlying module but are put in place by multiple different patches.

   Let's create a sibling MR in this issue against the 1.9.x branch of the module, so we can see how

   </code> works with these recommendations. To tell it to use the updated recommendations, modify this in that MR against 1.9.x:
   <code>
       AMA__RECOMMENDATIONS__VERSION: <name of the branch you create>
   

   Most of the test failures are trivial to resolve though: drupal_get_path() does not exist anymore and is a trivial string replacement, and errors like TypeError: Cannot access offset of type string on string are explicitly about weird magic that PHP <=7 used to do but PHP >=8 does not anymore! 💡 That's exactly why these migrations need to be re-vetted, and why I've been saying that we can't assume migrations that were developed against PHP 7 will work flawlessly on PHP 8 too!

MR looks great, and is green! 🤩

Next step: #3.4, to verify the integration tests in the AM:A module actually pass too 🤓 See 📌 Update: drupal/core:9.5.11 Fixed for a prior example of creating such a sibling MR.

I was trying to get this to run locally but am encountering this:

www-data@2030d29ffb9a:/app/docroot/core$ ../../vendor/bin/phpunit ../modules/contrib/acquia_migrate/tests/src/FunctionalJavascript/PathautoMigrationTest.php -vvv
PHPUnit 9.6.17 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.16
Configuration: /app/docroot/core/phpunit.xml

Testing Drupal\Tests\acquia_migrate\FunctionalJavascript\PathautoMigrationTest
S                                                                   1 / 1 (100%)

Time: 01:09.909, Memory: 10.00 MB

There was 1 skipped test:

1) Drupal\Tests\acquia_migrate\FunctionalJavascript\PathautoMigrationTest::testPathautoMigrations
The test wasn't able to connect to your webdriver instance. For more information read core/tests/README.md.

The original message while starting Mink: Could not open connection: Curl error thrown for http POST to http://localhost:4444/session with params: {"desiredCapabilities":{"browserName":"chrome","name":"Behat Test"}}

Failed to connect to localhost port 4444: Connection refused

/app/docroot/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php:58
/app/docroot/core/tests/Drupal/Tests/BrowserTestBase.php:372
/app/docroot/modules/contrib/acquia_migrate/tests/src/FunctionalJavascript/PathautoMigrationTest.php:52
/app/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

OK, but incomplete, skipped, or risky tests!
Tests: 1, Assertions: 1, Skipped: 1.

Which I believe is a problem with my local setup. I did have chromedriver running but not inside the container which I think was the ultimate issue. That might take a while to sort out, so perhaps easier to just test in the CI 😇

chromedriver is a nightmare.

Yes, go for CI 😄 It's going to be MUUUUUUUUUUUUUCH faster anyway.

fatal: '3424401-update-pathauto' matched multiple (2) remote tracking branches

D'oh. Looks like the logic I added in the CI only works if the sibling MR lives on a different issue fork 🫣

Well...I got it to be green but I am not sure if it is quite working as I see this in the output:

Uploading artifacts...
recommendations-next-*.json: found 1 matching artifact files and directories 
WARNING: recommendations-pinned-issue-fork/3424401-update-pathauto.json: no matching files. Ensure that the artifact path is relative to the working directory (/builds/issue/acquia_migrate-3424401) 

So I think changing the filename was OK to get past the first part, but now when we build the artifact to export it with the same name because when we try to run ACLI later on we encounter this:

🤖 Scanning Drupal 7 site.
👍 Found Drupal 7 site (7.82 to be precise) at sites/default, with 28 modules enabled!
In NewFromDrupal7Command.php line 83:
                                                                               
  /builds/issue/acquia_migrate-3424401/recommendations-pinned-*.json could no  
  t be located. Check that the path is correct and try again.   

Yep — sorry, this is tricky stuff. This is the first MR like this against recommendations-10, so the first time we're running into this.

You just move on to a next major contrib module to vet for Drupal 10 — I'll figure this out!

👆 All that painful back-and-forth resulted in a solution, which I extracted into 📌 .gitlab-ci.yml: module MR to test a D10 recommendations MR does not work Active . That'll allow the "test MR" to become as trivial as it should be (a single line!) 👍

Now that tests are actually running, it's become clear that they're failing:

    Testing
    Drupal\Tests\acquia_migrate\FunctionalJavascript\PathautoMigrationTest
    F                                                                   1 / 1
    (100%)
    
    Time: 01:16.366, Memory: 8.00 MB
    
    There was 1 failure:
    
    1)
    Drupal\Tests\acquia_migrate\FunctionalJavascript\PathautoMigrationTest::testPathautoMigrations
    Failed asserting that two arrays are equal.
    --- Expected
    +++ Actual
    @@ @@
         'd7_pathauto_patterns:node:article' => Array (
             'total rows' => 4
             'processed rows' => 4
    -        'imported items' => 4
    +        'imported items' => 0
             'items need update' => 0
    -        'errored items' => 0
    +        'errored items' => 4
         )
         'd7_pathauto_patterns:node:blog' => Array (
             'total rows' => 1
             'processed rows' => 1
    -        'imported items' => 1
    +        'imported items' => 0
             'items need update' => 0
    -        'errored items' => 0
    +        'errored items' => 1
         )
         'd7_pathauto_patterns:node:et' => Array (
             'total rows' => 1
             'processed rows' => 1
    -        'imported items' => 1
    +        'imported items' => 0
             'items need update' => 0
    -        'errored items' => 0
    +        'errored items' => 1
         )
         'd7_pathauto_patterns:node_default' => Array (...)
         'd7_pathauto_patterns:taxonomy_term:sujet_de_discussion' => Array
    (...)
    
    /builds/project/acquia_migrate/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
    /builds/project/acquia_migrate/drupal_module_under_test/tests/src/Traits/InitialImportAssertionTrait.php:106
    /builds/project/acquia_migrate/drupal_module_under_test/tests/src/FunctionalJavascript/PathautoMigrationTest.php:181
    /builds/project/acquia_migrate/drupal_module_under_test/tests/src/FunctionalJavascript/PathautoMigrationTest.php:89
    /builds/project/acquia_migrate/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

See https://git.drupalcode.org/project/acquia_migrate/-/jobs/953545.

This might require changes in \Drupal\Tests\acquia_migrate\FunctionalJavascript\PathautoMigrationTest, but most likely it just needs changes upstream, in PathautoMigrationAssertionsTrait (which AM:A's test reuses) or the upstream kernel test that uses that same trait.

@dan612 is aware of that, and we agreed on how to proceed — captured it over at #3179835-28: Migrate forum pattern to taxonomy term forums if forum is enabled on the source site → .

Merged 📌 .gitlab-ci.yml: module MR to test a D10 recommendations MR does not work Active into the "test" MR, which now is a single line MR again like it was always supposed to be! 😄

I'd like to call attention to this patch - https://www.drupal.org/project/pathauto/issues/3079275#comment-15271879 🐛 Custom aliases (which are not generated with the actual patterns) can be lost during the migration Needs review

I believe work from the other patches is now looped into this one. For example, the patch from this issue - https://www.drupal.org/project/pathauto/issues/3179835#comment-15474081 ✨ Migrate forum pattern to taxonomy term forums if forum is enabled on the source site Needs review - introduces a hook_migrate_process_info_alter() to do

+
+/**
+ * Implements hook_migrate_process_info_alter().
+ */
+function pathauto_migrate_process_info_alter(&$definitions) {
+  if (
+    \Drupal::moduleHandler()->moduleExists('taxonomy') &&
+    !empty($definitions['pathauto_forum_vocabulary'])
+  ) {
+    $definitions['pathauto_forum_vocabulary']['class'] = ForumVocabulary::class;
+  }
+}

This same snippet is in the patch from #67 🐛 Custom aliases (which are not generated with the actual patterns) can be lost during the migration Needs review .
. I spot checked a few other classes/code changes and they were all present in the "combined" patch.

So...I think this means we can omit the first 3 patches and just use #67 🐛 Custom aliases (which are not generated with the actual patterns) can be lost during the migration Needs review . However, that patch also will fail validation and need to be updated - for example the setUp functions lack a : void

Need to test more.

trying something...

I took what was in #67 🐛 Custom aliases (which are not generated with the actual patterns) can be lost during the migration Needs review and then changed out some things to avoid errors. The issue label conflicts with the patch name, at the moment...but I just wanted to see what this would do in CI.

fatal: '3424401-update-pathauto' matched multiple (2) remote tracking branches

Argh, this again! Looks like d.o's GitLab CI keeps changing underneath us 😬

I'm hoping that this was caused by my hand being forced yesterday to hotfix us from chasing default-ref of the d.o GitLab CI template and my having pinned it to 1.1.2. In 📌 Update to version 1.2.1 of the d.o GitLab CI template and adopt the `cspell` job Needs review , I'm pinning that to 1.2.1 instead, and hopefully a simple rebase here will fix the problem cited in #17 🤞

📌 Update to version 1.2.1 of the d.o GitLab CI template and adopt the `cspell` job Needs review landed.

Merged latest 1.9.x into the "test" MR 🤞

Still fails tests:

   There was 1 failure:
    
    1)
    Drupal\Tests\acquia_migrate\FunctionalJavascript\PathautoMigrationTest::testPathautoMigrations
    Failed asserting that two arrays are equal.
    --- Expected
    +++ Actual
    @@ @@
         'd7_pathauto_patterns:node:article' => Array (
             'total rows' => 4
             'processed rows' => 4
    -        'imported items' => 4
    +        'imported items' => 0
             'items need update' => 0
    -        'errored items' => 0
    +        'errored items' => 4
         )
         'd7_pathauto_patterns:node:blog' => Array (
             'total rows' => 1
             'processed rows' => 1
    -        'imported items' => 1
    +        'imported items' => 0
             'items need update' => 0
    -        'errored items' => 0
    +        'errored items' => 1
         )
         'd7_pathauto_patterns:node:et' => Array (
             'total rows' => 1
             'processed rows' => 1
    -        'imported items' => 1
    +        'imported items' => 0
             'items need update' => 0
    -        'errored items' => 0
    +        'errored items' => 1
         )
         'd7_pathauto_patterns:node_default' => Array (...)
         'd7_pathauto_patterns:taxonomy_term:sujet_de_discussion' => Array
    (...)
    

swapped out node_type for entity_bundle to see if that helps things along

Confirming that Dan is awesome and did great work on this and I feel questionable and compromised trying to finish his work in the current context. So I'm doing this on my own time.

Tests are running okay locally now based on this updated patch 🐛 Custom aliases (which are not generated with the actual patterns) can be lost during the migration Needs review .

That patch combines 4 of the original pathauto patches from Drupal 9 and adjusts them to pass tests on Drupal 10.

We can't discern if the tests pass because our pipeline configuration is still failing either due to IEF in D10, bean_migrate in D9, or database connection issues. I'm moving around those for now but let me know if I can help there.