Rolling back a migration implementing MigrationWithFollowUpInterface does not clear the generated follow up migrations from the cache.

Per \Drupal\Tests\migrate_drupal\Kernel\Plugin\migrate\source\ContentEntityTest::testConstructorInvalidBundle(), it looks like this was intentional.

Introduced in #2935951: Copy migrate source plugin from migrate_drupal_d8 into migrate_drupal, specifically in #2935951-71: Copy migrate source plugin from migrate_drupal_d8 into migrate_drupal, in response to reviewer feedback. This was 100% intentional.

Based on the issue summary, it sounds like this is only a problem during rollbacks. Those were not discussed in #2935951 at all. Suggestions:

• Can we make this source plugin context-aware, i.e. behave differently in case of rollbacks?
• Otherwise, can we introduce a new exception_on_invalid_bundle configuration key that defaults to TRUE? That way, we retain backwards compatibility while allowing migration developers wanting to support rollbacks with this @MigrateSource plugin.

@huzooka pointed out that another use case for \Drupal\migrate_drupal\Plugin\migrate\source\ContentEntity is the one where you want to query all nodes/terms/whatever entities, regardless of bundle. So the bundle configuration key should not be required.

This issue is not specific to Drupal 7 sources, removing tags. One could argue that this could have a migrate-d8+-d8+ tag, but there are only two source plugins for the current version of Drupal so I don't think it is necessary.

+1

throws an exception without any good reason: since it adds the bundle condition to its entity query, that will return zero rows for a missing bundle.

So @huzooka's point is basically this: it is harmless to still add a non-existent bundle to the query (as long as you're not joining on it at least).

The only other possible work-around is significantly changing the way rollbacks work in the migration system (see Zoltan's example in the issue summary).

I do think it could be good to modify the patch to log a "debug" level message instead of throwing the exception. That'd at least aid migration developers :)

The changes in test coverage here would then be:

1. an exception should no longer be expected
2. zero rows returned is what should now be expected

Re-rolled patch to 9.3.x

Here is the updated patch as suggested in #9

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -256,7 +269,7 @@ public function count($refresh = FALSE) {
   -    return $this->query()->count()->execute();
   +    return (int) $this->query()->count()->execute();
   

   Technically out of scope, but this change makes sense: it makes the return value comply with the interface it implements, so 👍

2. +++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/ContentEntityTest.php
   @@ -225,18 +225,30 @@ public function testConstructorNotBundable() {
       * Tests the constructor for invalid entity bundle.
   

   This line is outdated now 🤓

Marking Needs work for the second point 😅

Fixed. Thanks Wim :)

IMHO this is ready now!

We still have the problem that the testbot does not change the issue status when a patch fails to apply. It looks as though this issue needs a reroll.

Here's a reroll. There was a conflict in ContentEntityTest related to some recent changes: https://git.drupalcode.org/project/drupal/-/commit/c14450bc96f110baa01d4...

That commit essentially moved ContentEntityTest::testConstructorInvalidBundle() along with a few other functions into a new test class called ContentEntityConstructorTest. The patch here changes testConstructorInvalidBundle() to testNonExistingBundle() and changes a few things in that function, keeping it as part of ContentEntityTest. So this patch may kind of undo some of the re-organization that was trying to be accomplished.

Anyway, the reroll should be good to go, but it would be worth taking a closer look.

It looks to me like the failing test case simply needs to be removed, since it's expecting this issue not to have been fixed. I'll let someone else confirm that thinking since I'm new to the issue. I just went ahead and posted such a patch below.

Here's the re-rolled patch with the failing test removed. The failing test is definitely obsolete and represents a test for the behavior that this issue is fixing.

I am comparing the patches in #13 and #23.

The reason #13 stopped applying is that #3200534: Use dataprovider for constructor test in ContentEntityTest removed 4 tests from ContentEntityTest and replaced them with a single test, with a data provider, in the new test class ContentEntityConstructorTest. The patch in #13 renamed one of those tests and gave it a data provider with two cases.

There are no conflicts outside the tests.

The re-roll in #23 restores testNonExistingBundle(), the test method with two cases from a data provider, to ContentEntityTest and removes the corresponding test case from the data provider in ContentEntityConstructorTest. I think a better approach would be to leave ContentEntityTest alone and add a new test case to the data provider in ContentEntityConstructorTest.

I am setting the issue to NW for that.

I am updating the issue summary to mention the debug message in the "Proposed resolution" section.

I would like to see some test coverage of the debug message, first suggested in #9. Can we test that there are no messages logged in most of the test cases, but one is logged when expected?

I am not sure what the right level is: debug, info, notice? So maybe I will leave that to people who have stronger opinions.

Last point:

   protected function doCount() {
-    return $this->query()->count()->execute();
+    return (int) $this->query()->count()->execute();

This looks like an unrelated bug fix. I will not insist that you revert this change, but I will warn you that the core committers may ask you to do that. Maybe, with a relatively small patch like this one, they will accept a little unrelated fix like this.

Here's a patch that adds a Unit test that checks the logging. I tried to keep this in the Kernel test but it was a total mess. I need to mock the logger which means I also have to mock everything else. I didn't want to have to keep mocking such that the query would run. So I split this out into a new thing.

That's the only change I made, but stay with me! I

I think a better approach would be to leave ContentEntityTest alone and add a new test case to the data provider in ContentEntityConstructorTest.

I thought about that myself too, but I'm not convinced it's the right thing to do. First, per #9

The changes in test coverage here would then be:

1. an exception should no longer be expected
2. zero rows returned is what should now be expected

So this has to test more than simply the constructor, which would make ContentEntityConstructorTest a misleading place to have it. And since we need to test more than the constructor, the existing test in that class can't be easily adapted to this case. There would have to be a substantial modification.

This looks like an unrelated bug fix.

Looks like that first showed up in #11. @Wim Leers gave it the thumbs up in #12 while noting the same thing you did. I'll leave it for now since it had some positive reactions earlier. I'll be happy to remove it if there's a strong opinion.

+++ b/core/modules/migrate_drupal/tests/src/Unit/source/ContentEntityTest.php
@@ -0,0 +1,88 @@
+    $entity_type_bundle_info->getBundleInfo('node_type')->willReturn(['bar' => 'bar']);

I want to change this line so the return is more like ['bar' => ['definition']]

Having the key and the value both be bar was lazy and isn't good enough. I could change array_keys to array_values in the constructor being tested and this test would pass.

I point this out in part to let reviewers know I'm going to need to post something new, so feel free to call out nits that you may otherwise just let go!

#24, FWIW, the potential out of scope change is also being made in #2937166: Improve count() for ContentEntity source plugin.

I skimmed the patch and noticed this.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
@@ -130,7 +139,10 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
+        $this->logger->debug('The provided bundle (@bundle) is not valid for the (@entity_type) entity type.', [

I think this needs more context, at least the migration that causes this.

nit: When displaying messages and exceptions the migration system usually follows the example in coding standards for Exception messages, where a single quote not parenthesis surrounds the variable. But, I see that this is consistent with what this file is doing. Searching with PhpStorm for $this->logger-> shows that the use of the parenthesis is rare, a visual scan shows only three files that use parenthesis this way.

I stopped looked at the code as I thought about this change. So I went back to the IS.

When I read the IS it is not clear to me the steps that were taken. It seems that a full migration has been run. Then what? What is the action taken that causes the problem? Is this actually caused because the rollbacks were not done in the correct order? #2838995: Rollback does not verify dependent migrations have been rolled back.

I decided to see if I could reproduce the problem. I migrated the drupal 7 fixture and then rolled back all the node complete migrations. Then I did `ddev drush mr d7_entity_reference_translation:node__test_content_type`. That worked as expected, that is. without an Exception. Then I rolled back d7_node_type and tried to rollback d7_entity_reference_translation:node__test_content_type again and got the Exception. For me, that tells me the migrations were not rolled back in reverse dependency order (and the situation is avoidable).

I am not sure about the change to a log message as well. If, during an import the bundle does not exist then what will happen? I tested that as well. Turns out that a row with bundle that does not exist on the destination will migrate without error or a message logged. But try to view it and you get the 'website had an error' message. So, logging a message is an improvement but I think the exception is better. We should not migrate data that we know will cause an error.

The root issue here has nothing to do with the exception thrown by ContentEntity. That exception is good and correct. If you get to that point with a bundle name that doesn't exist in the system, then there is a bigger problem somewhere that needs to be fixed. That's why that exception exists and it should stay. The ability to get all bundles by not supplying a bundle already exists and works, and is a completely different case than the error of supplying a non-existent bundle.

In this case, the 'bigger problem somewhere else' is the fact that rolling back a migration that implements MigrationWithFollowUpInterface leaves those follow up migrations in the cache. The actual solution here is to have MigrateExecutable::rollback() check to see `if ($migration instanceof MigrationWithFollowUpInterface)` and if so, we need to clear the cached plugin definitions so they can be re-derived. It can actually just call $migration->generateFollowUpMigrations(), which will clear and rebuild the cache and remove the offending migrations.