Problem/Motivation

Split off from the parent to keep all the migration system changes in one issue and, hopefully, easier to review.

It looks like the block migration tests for d6_block and d7_block will require changes to the migration and that is probably best dealt with in a separate issue.

Steps to reproduce

Proposed resolution

Copy the migrate fixtures to the aggregator module so that the aggregator tests are independent of future changes to the fixtures.
Move migrate related tests to the aggregator module

Remaining tasks

Review

Make a separate issue for the block migrations and tests.
A followup could be made in aggregator to add testing of aggregator Id conflicts. This is not tested in core so is a new test. I did check this locally and it works as expected.

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3265424

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

quietone created an issue. See original summary.

quietone opened merge request !1849

quietone’s picture

quietone commented
Issue summary: View changes
Status: Active » Needs review

grep finds the following;

$ grep -Ir aggregator core/modules/ | grep -v fixture | grep -v .js | grep -v modules/aggregator | grep -v modules/migrate_drupal_ui | grep -i migrat | grep Test.php | awk -F: '{print $1}' | sort -u | nl
     1  core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockContentTranslationTest.php
     2  core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
     3  core/modules/block/tests/src/Kernel/Migrate/d7/MigrateBlockContentTranslationTest.php
     4  core/modules/migrate_drupal/tests/src/Kernel/d6/MigrateDrupal6AuditIdsTest.php
     5  core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7AuditIdsTest.php
     6  core/modules/migrate/tests/src/Kernel/Plugin/MigrationPluginListTest.php

1,2,3 - Still think should be handled in a separate issue
4,5,6 - has an @todo for removing when the module is removed from core.

This is now ready for review.

catch’s picture

catch
he/him
Primary language English
commented

Opened #3265483: Handle block migration for modules moved to contrib for blocks.

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

@quietone, I still see a mention of aggregator in Drupal\Tests\migrate_drupal\Kernel\StateFileExists in the declaration of $stateFileRequired

Does that need a @todo as well?

How did that escape your grep in #3? I'm not a grep-spert myself. Is it that the StateFileExists contains neither migrat not Test.php?

quietone’s picture

quietone commented

@danflanagan8, well spotted.

I have added an other @todo. Also filed an issue to fix the test file name, #3266443: Rename StateFileExists to StateFileExistsTest.

quietone’s picture

quietone commented
Related issues: +#3266491: Add content in migrate UI functional tests only when needed

There is an error in the migration functional tests, a bit of cleanup missed when those tests were refactored, and will be fixed in #3266491: Add content in migrate UI functional tests only when needed. Once that is done the new aggregator functional tests need a change. I have made an issue for that and added @todo #3267040: Remove @todo in migration functional tests.

I think this covers everything now.

daffie’s picture

daffie commented
Status: Needs review » Needs work

Looks good to me.
Just a couple of nitpicks.

quietone’s picture

quietone commented
Parent issue: #3264122: Move all non migration aggregator tests to the module in preparation of removal in d10 » #3188549: [Meta] Tasks to deprecate both Aggregator and our dependency on Laminas Feed
quietone’s picture

quietone commented
Status: Needs work » Needs review

I answered all the unresolved issues, setting back to NR

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

All the threads on the MR are answered.
For me it is RTBC.

  • catch committed c91106e on 10.0.x 
    Issue #3265424 by quietone, daffie, danflanagan8: Move migrate related...

  • catch committed 817c080 on 9.4.x 
    Issue #3265424 by quietone, daffie, danflanagan8: Move migrate related...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Got this from phpstan (and also the d7 version), but fixed on commit.

I'm slightly concerned about the duplication of test coverage in 9.4.x, but having the contrib and core modules as much in sync as possible seems better than worrying about that too much.

Running PHPStan on changed files.
PHP Fatal error:  Declaration of Drupal\Tests\aggregator\Kernel\Migrate\d6\MigrateDrupal6TestBase::setUp() must be compatible with Drupal\Tests\migrate_drupal\Kernel\MigrateDrupalTestBase::setUp(): void in /home/catch/www/drupal/core/modules/aggregator/tests/src/Kernel/Migrate/d6/MigrateDrupal6TestBase.php on line 26
Fatal error: Declaration of Drupal\Tests\aggregator\Kernel\Migrate\d6\MigrateDrupal6TestBase::setUp() must be compatible with Drupal\Tests\migrate_drupal\Kernel\MigrateDrupalTestBase::setUp(): void in /home/catch/www/drupal/core/modules/aggregator/tests/src/Kernel/Migrate/d6/MigrateDrupal6TestBase.php on line 26

Committed/pushed to 10.0.x and cherry-picked to 9.4.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.