ContentEntity migration source adds revision ID as source key, incompatible with Drupal 8.8 and earlier

The fix provided works on D8.9 and it fixes my migrations after updating core from D8.8 to D8.9

@geek-merlin, thanks for working on this issue. And thanks to @rutiolma for testing: that means that I can just do a review.

I left several comments on the MR. The most important is that we have to make the default behavior consistent with the existing behavior of Drupal 9. It is unfortunate that we have an unintended BC break between D8 and D9, but the way to fix a BC break between major versions is not to introduce a BC break between minor versions.

I am also adding a tag for a change record. That is the right place to give an extended explanation of the new option, usage examples, and instructions for updating from Drupal 8.

Oops, I forgot to click the "submit review" button.

Done.

I would like to keep the scope of this issue focused and keep the changes as simple as possible. If we do not do that, then there is a good chance that we get this issue to RTBC but then a core commiter will set it back to NW for just that reason.

As part of narrowing the scope, we should change the issue title and update the proposed resolution. I was thinking of that in my first review, but I will make it official by adding the issue tag for that.

Let's make it so that nothing changes from this fix unless the developer adds the new, optional parameter.

1. A recurring migration that worked on Drupal 8.8 no longer works after upgrading to 8.9 or 9.x.
2. The content_entity source plugin creates a row for each revision, and there is no way to get a row for each entity.

We can fix both of these problems with the new, optional parameter.

We say the current state is so broken against the basic contract of the migration machinery ...

That may be true, but I have not thought about it enough to decide. If it is true, then let's discuss it in a separate issue.

We say we keep BC anyway and add a special legacy setting (0=no-revisions, 1=revisions, 2=no-revisions-in-migration-but-in-ids)

It might be a good idea to add a third option in the suggested follow-up issue. Let's use a string-valued option instead of an integer-valued one with a glossary. In this issue, the new option has two possible values, and we can add a third option in the follow-up issue. So something like this: the new option is called revisions, and this issue provides two options: add key (the default, for the current D9 behavior) and none (for the D8 behavior). Later, we can add a third option, all. Maybe you can come up with better names.

@geek-merlin:

Maybe I did not understand exactly what you wanted in each of the two issues, but I have been consistent in this: I do not want to fix a BC break between D8 and D9 by introducing a BC break between D9.0 and D9.2. The initial patch on the other issue changed behavior without providing an option. I thought the difference between the issues was that this one made the behavior configurable.

To be clear: Drupal 8.9 is only receiving security fixes, not bug fixes. At this point, it is too late to fix the BC break between 8.8 and 8.9. This issue currently targets 9.2. If we keep the scope narrow, then it is likely to be back-ported to 9.1.

Short answer: yes, I confirm what I said in #16.

The quickest way to get this issue fixed is to narrow the scope and to keep the changes as simple as possible.

Adding a deprecation notice does not meet those criteria. Neither does adding two options instead of one.

We discussed the plans in several comments on this issue before you went into code-mode. If you did not like the idea of string-valued revisions option with three possible values, why did you not continue the discussion first?

I do not like "d9bc" as one of the options. It describes the history of the project rather than what the option does.

Drupal 8.9.12 was released yesterday, and it seems that I was wrong in #18:

At this point, it is too late to fix the BC break between 8.8 and 8.9.

Some bug fixes, not just security issues, are still being applied to the 8.9.x branch. I still think I was right when I said that the best chance for getting the fix back-ported is to keep the scope narrow.

I've been keeping an eye on this and finally sat down to read it thoroughly. It took a while and I wish the IS was updated. What I see is that the solution is doing more than it needs to and differs from the suggestions in the reviews by benjifisher. the new dataprovider for the tests is nice and adds test coverage.

I think there is value in seeing what the solution suggested in the reviews looks like. With that in mind I have made a patch, there is no interdiff because this is a different approach.

Setting NR to start the tests.

@quietone, thanks for implementing the suggestions from my earlier reviews. I think this approach is sound, and we just need to do a little cleanup.

1. Nit: add some commas to these comments, and rewrap as needed.

    +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
    @@ -30,16 +30,30 @@
    + * - revisions: (optional) Determines if the revision key is added to the IDs.
    + *   The default is 'add key' which adds the revision key to the IDs. If 'none'
    + *   the revision key is not added to the source IDs.
      *
      * Examples:
      *
      * This will return all nodes, from every bundle and every translation. It does
    - * not return all revisions, just the default one.
    + * not return all revisions, just the default one. This is the default and
    + * it is not necessary to declare the 'revisions' option.

   I think we need commas before “which adds”, after “If ‘none’”, and after “This is the default”. In fact, I would add “, then” after “If ‘none’”.

2. I do not think the comment is useful:

    + * @code
    + * source:
    + *   plugin: content_entity:node
    + *   # Setting this is mandatory to not get inconsistent migrations.
    + *   revisions: none
    + * @endcode

3. I like the decision to test !== 'none' rather than === 'add key':

    @@ -265,7 +280,7 @@ public function fields() {
    ...
    -    if ($this->entityType->isRevisionable()) {
    +    if ($this->configuration['revisions'] !== 'none' && $this->entityType->isRevisionable()) {

   As I said at the end of #16, we plan to add a third possibility for this option in a follow-up issue. If we do that, then we will not have to change this line.

4. This is not the typical data provider:

    +++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/ContentEntityTest.php
    @@ -239,20 +241,69 @@ public function testConstructorInvalidBundle() {
    ...
    +  /**
    +   * Provides source configuration for the migration.
    +   */
    +  public function migrationConfigurationProvider() {
    +    $data = [];
    +    foreach ([FALSE, TRUE] as $include_translations) {
    +      foreach (['add key', 'none'] as $revisions) {
    +        $configuration = [
    +          'include_translations' => $include_translations,
    +          'revisions' => $revisions,
    +        ];
    +        // Add an array key for this data set.
    +        $data[http_build_query($configuration)] = [$configuration];
    +      }
    +    }
    +    return $data;
    +  }

   I think the one-line summary should be something like “Data provider for several test methods.” and we should add @see comments for each test method.

5. I do not think it is a requirement, but we normally define the data provider after the related test method.

6. A test-only patch would show us how well http_build_query($configuration) works as a key.

7. I think we should test !== 'none' rather than === 'add key' for the same reason as (3):

    +  protected function assertIds(MigrateSourceInterface $source, array $configuration) {
    ...
    +    if ($entity_type->isRevisionable() && $configuration['revisions'] === 'add key') {
    +      $ids_count_expected++;
    +      $this->assertArrayHasKey($entity_type->getKey('revision'), $ids);
    +    }

8. This applies to several (all?) of the test methods:

       /**
        * Tests user source plugin.
    +   *
    +   * Legacy group is needed to expect deprecation.
    +   * @group legacy
    +   *
    +   * @dataProvider migrationConfigurationProvider
        */

   I think that @group legacy is left over from the earlier patches on this issue and should be removed. If I am wrong about that, then I still think we should remove the comment, “Legacy group is needed …”.

9. I am a little worried that this change is out of scope. Do we need it? Again, this applies to several or all of the test methods.

    -    $migration = $this->migrationPluginManager->createStubMigration($this->migrationDefinition('content_entity:user'));
    -    $user_source = $this->sourcePluginManager->createInstance('content_entity:user', $configuration, $migration);
    +    $migration = $this->migrationPluginManager->createStubMigration($this->migrationDefinition('content_entity:user', $configuration));
    +    $user_source = $migration->getSourcePlugin();

   I see that we need to add $configuration when defining $migration, but do we need to change how we get $user_source? Even if it is out of scope, it does seem like an improvement.

10. If we decide to keep that change, then we should remove the $sourcePluginManager property and the line in setUp() that sets it.

11. Same snippet: break the line before ->createStubMigration().

12. Class properties are always camelCase. Other variables can be either camelCase or snake_case, but should be consistent within one file:

     +    $expectRevisionAsIdWithNoDelta = $configuration['revisions'] === 'add key';
     +    if ($expectRevisionAsIdWithNoDelta) {
     +      $this->assertEquals(1, $values['vid']);
     +    }
     +    else {
     +      $this->assertEquals([0 => ['value' => 1]], $values['vid']);
     +    }
     ...
     +      if ($expectRevisionAsIdWithNoDelta) {
     +        $this->assertEquals(1, $values['vid']);
     +      }
     +      else {
     +        $this->assertEquals([0 => ['value' => 1]], $values['vid']);
     +      }
     ...
     +    $expectRevisionAsIdWithNoDelta = $configuration['revisions'] === 'add key';
     +    if ($expectRevisionAsIdWithNoDelta) {
     +      $this->assertEquals(1, $values['vid']);
     +    }
     +    else {
     +      $this->assertEquals([0 => ['value' => 1]], $values['vid']);
     +    }

    We could remove the last usage and put the comparison inside the if(...), but it makes sense to use the same pattern in both test methods.

13. Same snippet: do we want to compare === 'add key' instead of !== 'none'?

14. This is a little surprising:

     -  public function testMediaSource() {
     +  public function testMediaSource(array $configuration) {
     ...
     @@ -357,17 +433,16 @@ public function testMediaSource() {
     ...
     -    $configuration = [
     -      'include_translations' => FALSE,
     +    $configuration += [
            'bundle' => 'image',
          ];
     -    $migration = $this->migrationPluginManager->createStubMigration($this->migrationDefinition('content_entity:media'));
     -    $media_source = $this->sourcePluginManager->createInstance('content_entity:media', $configuration, $migration);
     +    $migration = $this->migrationPluginManager->createStubMigration($this->migrationDefinition('content_entity:media', $configuration));
     +    $media_source = $migration->getSourcePlugin();

    The existing test defined $configuration and used it when defining $media_source but not $migration. That makes me wonder whether we could use the same migration in all the test methods and define it in setUp(), and go back to the original definition using $this->sourcePluginManager. That makes me lean more towards deferring this simplification to a follow-up issue if it is out of scope.

Yes, I put very little effort into cleanup.

#25, the first half
1,2 Fixed.
3. I was hoping you would like that!
4. Fixed. That is much better.
5. Moved the provider to after testTermSource
6. Agreed. Fail patch will be provided when 9-14 are fixed.
7. Fixed
8. Fixed
13. Fixed
9, 10, 11, 12, 14. todo

Edit: 13 was fixed as well

And now for the rest of #25.
25
9. It is needed because $this->migrationDefinition adds the configuration to the source plugin before creating the migration. If this is not done then the rewind results in a Undefined index: revision_id. Somewhere in rewind the source plugin is rebuilt and the 'source' needs to have the configuration. If not, the default configuration is used, which sets revisions to 'add key'. I'm not sure why the original test did a create instance for the source plugin, getting from the migration makes more sense to me.
10. $this->sourcePluginManager removed
11. Fixed
12. Fixed
14. I don't think the migration can be in setup, they are derived and need to have the unique entity type in the ids.

@geek-merlin:

By making this bug permanent …

I am trying to control scope on this issue. I want a simple change to fix the problem that migrations that used to work are now broken. We can have follow-up issues to do more, so I do not think anything we do on this issue makes the bug permanent.

monotonicity: A target object is only deleted or orphaned, if a source object is deleted.

I am familiar with the mathematical meaning of "monotonic", and this is not what it means. Either the word has additional meaning(s) or you should use a different term.

@quietone:

1. I do not see a test-only patch (#25.6).

2. I still see several uses of @group legacy (#25.8).

3. Follow-up to #25.9: I agree that using getSourcePlugin() has advantages: it is more like "real life". The disadvantage is that it is less direct, so it is less clear exactly what we are testing. But if the tests fail somewhere in the rewind(), then that makes it an easy decision. Thanks for explaining that.

All the other points from #25 have been addressed. Thanks for that! NW for (1) and (2).

And the fixes for #30.

30.1 I think I intended to come back and make a fail patch after #27 passed tests, and I just forgot.
30.2 Removed 'legacy'.

I forget why I set this issue to NW the last time I looked at it. Maybe I did not notice that the failure was from the test-only patch. Maybe I meant to say it was for an IS update.

I looked at the failures from the test-only patch. There are 5 tests using the same data provider:

• user: pass
• file: pass
• node: fail
• media: fail
• taxonomy term: fail

That is because User and File entities are not revisionable; the other three types are. +1

All three failures are similar: they fail for the two test cases with revisions=none. On the one hand, this suggests that the tests are redundant. On the other hand, the point of automated tests is to catch mistakes that we have not yet made, so I think it is good to use the data provider for all entity types.

As I said in #25.6,

A test-only patch would show us how well http_build_query($configuration) works as a key.

It works pretty well! For example,

1) Drupal\Tests\migrate_drupal\Kernel\Plugin\migrate\source\ContentEntityTest::testNodeSource with data set "include_translations=0&revisions=none" (array(false, 'none'))
Failed asserting that 3 matches expected 2.

In #30.2, I said there were "several uses of @group legacy", but I think there was just one, and it is removed in #31. Thanks for that.

The code looks good to me. I will update that CR and the IS.

I have updated the change record, the issue summary, and the title of this issue.

It occurs to me that this issue is now reviewed but not tested. It would be good to get confirmation from people who have run into this bug that the solution actually works! I hope that @geek-merlin and/or @rutiolma can confirm.

I looked again at the sample migration cited in #3184627-8: D8.9 breaks D8-to-foo entity migrations. The odd thing is that this migration uses plugin: content_entity:user. That comment also refers to revisions of User entities.

Since User entities are not revisionable, I would expect the patches on this issue to have no effect when the source entity type is User: see my comments on the test results in #34. Perhaps there is an additional module that makes User entities support revisions? I see the User Revision module, but that is only for Drupal 7.

Conclusion: I am leaving the status at NW, and I am adding the "Needs manual testing" issue tag. If we can get some manual testing (and the patch works), then the issue will be RTBC.

Sorry, I meant to mark this issue NR in my last comment.

I am doing the manual testing now. If I do not comment within 24 hours, feel free to reassign this issue.

Manual testing

I installed Drupal 8.8.9 with the Umami demo profile, so I had some content for the test.

I added a plain text field, field_copy_of_title, to the Article content type.

I added the Migrate Plus and Migrate Tools modules and Drush, so that I could run migrations from the CLI.

I added a custom module with a simple migration:

id: migrate_test
label: Test migration
source:
  plugin: content_entity:node
  bundle: article
destination:
  plugin: entity:node
  translations: true
process:
  nid: nid
  langcode: langcode
  field_copy_of_title: title

I enabled the custom module, along with Migrate Plus, Migrate Tools, and Migrate Drupal.

I ran the migration. It populated the new field as expected.

I backed up the database.

I checked out the HEAD of 9.2.x. I re-installed the contrib modules and Drush with Composer.

I ran database updates.

I ran the migration again (drush migrate:import migrate_test). I got an error message similar to the one described in #3184627: D8.9 breaks D8-to-foo entity migrations.

I applied the patch from #31.

I dropped the database and imported the backup.

I ran database updates.

I ran the migration again. I got the error again.

I added revisions: none to the source configuration and cleared the plugin cache.

I reset the test migration to Idle (drush migrate:reset-status migrate_test).

I ran the migration. No error. Success!

Just for good measure, I added another Article node, leaving the extra field empty, and ran the migration yet again. It populated the test field as expected.

This issue is now tested as well as reviewed.

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -30,16 +30,29 @@
   + * - revisions: (optional) Determines if the revision key is added to the IDs.
   

   This name feels quite confusing. Is it possible to change this to add_revision_id and have the values as TRUE or FALSE? I think this was a string because we consider a tri-state solution earlier but thats no longer planned.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -30,16 +30,29 @@
   - * not return all revisions, just the default one.
   + * not return all revisions, just the default one. This is the default, and
   + * it is not necessary to declare the 'revisions' option.
   

   This comment feels very confusing. Because regardless of the value of the 'revisions' setting the default revision will be returned.

3. +++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/ContentEntityTest.php
   @@ -239,20 +233,49 @@ public function testConstructorInvalidBundle() {
   +    $this->assertEquals($ids_count_expected, count($ids));
   

   assertCount should be used.

As this is a major bug it is a candidate for backport to 8.9.x - if the current patch applies it'd be worth triggering a test run.

#38.1: I still want the flexibility to add a third option. If @geek-merlin is right, then we should deprecate, and eventually remove, the current default option.

@benjifisher deprecating the configuration option does not require a third state as far as I can see.

@alexpott:

I have more time now. Let me be more explicit.

There could be as many as four options regarding revisions:

The second row is there just for completeness: if we are going to return multiple revisions, then we need a source ID to distinguish them.

@geek-merlin says that the third row is equally useless. That may be true. I have not thought about whether there might be a use case for this (the current behavior).

The fourth row is why I want to use a string-valued option, not a boolean. It seems like the simplest way to let us add Yes/All in a later issue. We could have two boolean options, but then we would have to say which combinations are allowed. If we decide that only No/Default and Yes/All are reasonable, then we could have a single boolean option, but that would mean dropping the possibility of the current behavior.

As far as I am concerned, the 3-way option is not currently planned as part of this issue since I want to narrow the scope to fixing migrations that worked with earlier versions of Drupal.

Regarding #38.2, what do you think of this explanation?

This will return all nodes, from every bundle and every translation. It does not return all revisions, just the default one. By default, or if you include ‘revisions: add key’, the revision ID is included as a source ID.

The first two sentences are the current text. I would not object to switching it around:

This will return all nodes, from every bundle and every translation. It returns the default revision, not all revisions. By default, or if you include ‘revisions: add key’, the revision ID is included as a source ID.

I tested locally: the patch applies cleanly to the 8.9.x branch, and the ContentEntityTest test class passes. I will ask the testbot to confirm.

Edit: I see you already exercised the testbot.

@benjifisher thanks for #41. It doesn't really change my thoughts from #38. Meaningful configuration keys and values are helpful. So for the table in #41 - I think we can have two configuration keys - add_revision_id and all_revisions both of which are boolean. And if the add_revision_id is FALSE and all_revisions is TRUE then we can throw a runtime exception.

+1 to the documentation improvements suggested in #41 (after they are updated for the earlier part of this comment).

For this issue, we just need the add_revision_id option. I am updating the issue summary.

@alexpott, I gave two suggestions in #41. Both add to the existing text, and the second also rearranges the existing text. Do you have a preference?

I have been putting off deciding whether the current behavior (include revision key, return only the default revision) is ever useful. @geek-merlin argues that this behavior is fundamentally broken.

If so, then we will want to change the default and remove the include/default option (in follow-up issue(s)). If we have two boolean options, as agreed in the last two comments, then we will be in the odd position of using two options to choose between two behaviors and throwing runtime exceptions in two of four cases.

I am assigning this issue to myself until I find time to decide whether @geek-merlin is right. If I agree, then I will push back against the approach with a boolean option in this issue and a second boolean option in a follow-up issue.

I do not have a complete, realistic use case, but I think the current behavior (include revision key as a source ID, fetch just the current revision) could be useful.

Suppose I want to use a migration as an alternative to hook_entity_presave(). Every time a new entity (node, for example) is created, or an existing entity is updated, I want to do some processing. For the update case, I have configured the entity to always create a new revision.

The current (Drupal 8.9, 9.x) behavior of the ContentEntity source plugin supports this requirement. We should continue to support it.

So let's go with the boolean option add_revision_id as discussed in #42, #43. In #43, I already update the issue summary, so no one has to hunt through the comments. We may decide, in a follow-up issue, to add another boolean option , all_revisions.

@geek-merlin, do you think I am missing anything? You wrote on MR !63,

IDs must match migration objects (revisionId for revisions, otherwise not), everything else is insane

(That comment was copied to this issue, but not assigned a comment number here, making hard to refer to it.)

#38
1. The key is now a boolean add_revision_id
2. Comment changed. Since there was no agreement on which of two suggestion in #41, I created another possibility.
3. Fixed.

These changes seem to warrant a new fail patch, so that is added. I think this is now up to date.

@quietone, thanks for the update! This patch addresses all the points in #38, and I like your version of the comment better than the variants I proposed in #41.

Back to RTBC.

Committed and pushed 75efbd32eb to 9.2.x and e8001d02ef to 9.1.x. Thanks!

Backporting to 9.1.x because behaviour is not changed by default and you can now use this to get out of the problem if you're in it.

@alexpott:

In #38, you suggested that this patch might be applied to 8.9. Is that still an option?

The change record needs to be published. I updated it a few days ago. Can some check it and hopefully publish it?

Drupal 9.1.5 was release a few days ago, and it includes this fix. I updated the CR and published it.

The patch already passes tests on 8.9.x, so I am changing the status to RTBC for 8.9.x.

Discussed with @catch we agreed to put this in 8.9.x.

Thanks, @alexpott!

I update the CR, adding

Drupal 8.9

Issue #3184650 was fixed for the 8.9.x branch. The new add_revision_id should be available in the next bugfix release after 8.9.13.

I did not change the metadata (introduced in 9.1.x/9.1.5).