Problem/Motivation

Identified while working on Bean Migrate:

The EntityContentComplete migration destination plugin blindly marks every migrated content entity revision as default revision. This is wrong, because a moderated entity might have forward (so not-yet-default or not-yet-published) revisions as well.

Proposed resolution

Check whether the row being migrated has a revision_default destination property.

  • If the property's value is FALSE, do not set default revision.
  • If the property is not set at all, or its value is NULL, or it has a value, mark the entity revision as default revision.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#9 core-allow_migrating_forward_revisions-3200949-9.patch1.1 KBhuzooka
#8 bean_migrate-remove_source_count_caching_workaround-3199294-9.patch1.4 KBhuzooka
#3 interdiff-3200949-2-3.txt1.32 KBhuzooka
#3 core-allow_migrating_forward_revisions-3200949-3.patch1.3 KBhuzooka
#2 core-allow_migrating_forward_revisions-3200949-2.patch1.03 KBhuzooka

Comments

huzooka created an issue. See original summary.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Assigned: huzooka » Unassigned
Status: Active » Needs review
StatusFileSize
new core-allow_migrating_forward_revisions-3200949-2.patch1.03 KB
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
StatusFileSize
new core-allow_migrating_forward_revisions-3200949-3.patch1.3 KB
new interdiff-3200949-2-3.txt1.32 KB
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Title: Unpublished entity revisions get published during migration because EntityContentComplete does not allows creating forward (non-default) revisions » Migration of unpublished entity revisions is impossible because EntityContentComplete does not allows creating forward (non-default) revisions
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Priority: Normal » Major
Status: Needs review » Needs work
Issue tags: +Needs tests

AFAICT this also affects the migration of nodes from D7, which on relatively many sites have forward revisions. So that means unpublished forward revisions would get migrated into D8|9 as default revisions 😨

Given that, bumping to Major.

Of course, having tests that prove this problem are essential for us to land this 😅

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented 
+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentComplete.php
@@ -126,7 +130,7 @@ protected function save(ContentEntityInterface $entity, array $old_destination_i
-    ];
+    ];      ¶

This is what triggered the phpcs failure:

FILE: ...migrate/src/Plugin/migrate/destination/EntityContentComplete.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 133 | ERROR | [x] Whitespace found at end of line
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Easy fix obviously :)

That'd be worth fixing, to see if this is triggering failures elsewhere in Drupal core tests 🤓

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented

Re #6 omg, that should be my broken command key :(

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
StatusFileSize
new bean_migrate-remove_source_count_caching_workaround-3199294-9.patch1.4 KB

Re #5:

For testing this even with kernel or functional tests, I think that the node revision (or the node translation? or just the node complete?..) migration source plugin should provide source data.
OR
Maybe the migration plugin definition should take care of that destination property?...

(Note: Bean migrate uses the "status" property for this revision default destination prop: if a revision wasn't published, then we can assume that it wasn't ever a default revision.)

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
StatusFileSize
new core-allow_migrating_forward_revisions-3200949-9.patch1.1 KB

#8 contains a proper patch, but it's not for Drupal core.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Zero test failures suggests that indeed we have no test coverage for this edge case.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Title: Migration of unpublished entity revisions is impossible because EntityContentComplete does not allows creating forward (non-default) revisions » Non-default entity revisions are migrated as default revision because EntityContentComplete does not allows creating forward (and non-default) revisions

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review

We need feedback from the migration system maintainers about how they would like to see this tested.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » quietone
Issue tags: +Needs subsystem maintainer review
quietone’s picture

quietone commented
Assigned: quietone » Unassigned
Status: Needs review » Needs work
Issue tags: -migrate-d6-d7, -Needs subsystem maintainer review +migrate-d6-d8, +migrate-d7-d8

This seems reasonable to me, although I know very little about Content Moderation. Since the default is set in EntityContentComplete then we need the fixtures changed because the functional tests run with the node complete migrations. I don't know how this will effect the existing Kernel tests though. I am guessing that the two MigrateNodeComplete tests can be updated to check that the default revision is correct.

Hope that helps.

quietone’s picture

quietone commented
Related issues: +#3164497: Content moderation "current revision" not set on nodes migrated from Drupal 7 where there are both a published and draft revisions

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

firewaller’s picture

firewaller commented

+1

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Status: Needs work » Postponed

The Migrate Drupal Module was approved for removal in #3371229: [Policy] Migrate Drupal and Migrate Drupal UI after Drupal 7 EOL.

This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

The deprecation work is in #3522602: [meta] Tasks to remove Migrate Drupal module and the removal work in #3522602: [meta] Tasks to remove Migrate Drupal module.

Migrate Drupal will not be moved to a contributed project. It will be removed from core after the Drupal 12.x branch is open.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

quietone’s picture

quietone commented
Status: Postponed » Closed (won't fix)

The Migrate Drupal Module and Migrate Drupal UI are deprecated and they are not in Drupal 12.0.0.

Issues for these modules should now be on the 11.x branch. And the changes are limited to critical and major bug fixes. Other changes are allowed at the discretion of the core Release Managers in consultation with the Migrate subsystem maintainers.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.