Problem/Motivation

If the source ceases to have data , this is never reflected in the destination. There are two problems (at least), this patch attacks the field level in the content entity base, config entities and raw config entities are also broken but probably needs to be different issue and less severe because this only comes up in reruns, the migrate_drupal path does not really support reruns. And then there is the case when a whole row / entity disappears but that is an entirely different issue.

Proposed resolution

Unset outdated data.

Remaining tasks

Test this. Think it through.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#92 interdiff-82-91.txt1.13 KBjofitz
#92 2711353-91.patch7.92 KBjofitz
#82 interdiff-2711353-80-82.txt974 bytesrakesh.gectcr
#82 2711353-82.patch7.91 KBrakesh.gectcr
#81 interdiff-2711353-73-76.txt607 bytesrakesh.gectcr
#81 interdiff-2711353-62-79.txt1.53 KBrakesh.gectcr
#81 2711353-79.patch7.91 KBrakesh.gectcr
#77 interdiff-2711353-62-76.txt1.1 KBrakesh.gectcr
#76 interdiff-2711353-63-76.txt0 bytesrakesh.gectcr
#76 interdiff-2711353-73-76.txt607 bytesrakesh.gectcr
#76 2711353-76.patch8.18 KBrakesh.gectcr
#73 interdiff-2711353-62-73.txt523 bytesrakesh.gectcr
#73 2711353-73.patch7.95 KBrakesh.gectcr
#62 interdiff.txt726 bytesquietone
#62 2711353-62.patch7.82 KBquietone
#58 interdiff-56-58.txt1.3 KBjofitz
#58 2711353-58.patch7.82 KBjofitz
#56 interdiff-53-56.txt1.45 KBjofitz
#56 2711353-56.patch7.79 KBjofitz
#53 interdiff-2711353-51-53.txt1.25 KByogeshmpawar
#53 migrate_never_unsets-2711353-53.patch7.79 KByogeshmpawar
#51 interdiff-2711353-49-51.txt3.12 KByogeshmpawar
#51 migrate_never_unsets-2711353-51.patch7.86 KByogeshmpawar
#47 shortinterdiff.txt988 bytesPavan B S
#47 2711353-46.patch7.95 KBPavan B S
#37 interdiff-31-37.txt959 bytesjofitz
#37 2711353-37.patch7.96 KBjofitz
#36 interdiff-31-36.txt959 bytesjofitz
#36 2711353-36.patch7.82 KBjofitz
#31 interdiff.txt3.98 KBquietone
#31 2711353-31.patch7.93 KBquietone
#27 interdiff.txt5.28 KBquietone
#27 2711353-27.patch8.19 KBquietone
#27 2711353-27-fail.patch4.77 KBquietone
#24 2711353_24.patch3.24 KBbiguzis
#9 2711353_9.patch3.08 KBchx
#5 2711353_5.patch3.18 KBchx
#2 migrate_unset.patch1.66 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx created an issue. See original summary.

chx’s picture

Status: Needs review » Needs work

The last submitted patch, 2: migrate_unset.patch, failed testing.

Berdir’s picture

Quick feedback/things to think about:

We can't remove all fields, only those that are actually defined in the migration, otherwise this is going to kill partial migrations that import some additional fields in real fun ways (I expect the patch is going to fail badly in some tests).

What if we change getDestination() to return all fields, even if they don't have a value? If $values is NULL, then things will basically just work? This logic would have to change:

      // No plugins or no value means do not set.
      if ($plugins && !is_null($value)) {
        $row->setDestinationProperty($destination, $value);
      }

If the second condition is removed, will anything break?

chx’s picture

dawehner’s picture

I mean I get why its working, but its absolutely not clear what an empty destination value would do.

chx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: 2711353_5.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
3.08 KB
chx’s picture

Issue summary: View changes
chx’s picture

Issue summary: View changes
mikeryan’s picture

Issue tags: +Needs tests
benjy’s picture

+++ b/core/modules/migrate/src/MigrateExecutable.php
@@ -397,8 +397,13 @@ public function processRow(Row $row, array $process = NULL, $value = NULL) {
+          $row->setDestinationProperty($destination, $value);      ¶

Could setDestinationProperty() not just accept NULL's and then loop over them to get the empty values in the destination?

chx’s picture

> If the second condition is removed, will anything break?
> Could setDestinationProperty() not just accept NULL's and then loop over them to get the empty values in the destination?

*spreads arms* I have no idea and I am not keen on trying to find out. I don't even want to try to think over the possible ramifications of changing how the destination is stored / set. But I will try to think on it for a few days.

vasi’s picture

Version: 8.1.x-dev » 8.2.x-dev

Some of my migrations change existing entities, and use NULL destination fields to indicate "I don't want this field to be changed". I think we need to keep at least some way to do that.

We should probably decide that NULL means one of "don't change this field" or "erase this field". And then use a custom class to indicate the other?

vasi’s picture

Status: Needs review » Needs work
mikeryan’s picture

@vasi: The default assumption when migrating is that the source data is the system-of-record - each time an item is (re-)imported, the destination should be an exact replica of the source data, including omitting data that is not on the source. In D7 we had an explicit system-of-record switch, but in D8 we use overwrite_properties - if this is set, the fields listed there are the only ones to be overwritten by migration, any other fields will be left at their existing D8 values (if any).

vasi’s picture

Status: Needs work » Needs review

Ok, sorry for the rant then :)

mikeryan’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -397,8 +397,13 @@ public function processRow(Row $row, array $process = NULL, $value = NULL) {
    +          $row->setDestinationProperty($destination, $value);      ¶
    

    Trailing whitespace.

  2. +++ b/core/modules/migrate/src/Row.php
    @@ -229,6 +234,22 @@ public function removeDestinationProperty($property) {
    +   * ¶
    

    Trailing whitespace.

Setting to needs work for tests...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

iMiksu’s picture

Issue tags: +Needs reroll

#9 needs a reroll.

Your branch is up-to-date with 'origin/8.3.x'.
$ curl https://www.drupal.org/files/issues/2711353_9.patch | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  3157  100  3157    0     0   5733      0 --:--:-- --:--:-- --:--:--  5729
patching file core/modules/migrate/src/MigrateExecutable.php
Hunk #1 succeeded at 409 (offset 12 lines).
patching file core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
Hunk #1 FAILED at 122.
Hunk #2 succeeded at 137 with fuzz 2 (offset -4 lines).
1 out of 2 hunks FAILED -- saving rejects to file core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php.rej
patching file core/modules/migrate/src/Row.php
iMiksu’s picture

Issue tags: +DCampBaltics

We'll work on this today.

biguzis’s picture

Assigned: Unassigned » biguzis
biguzis’s picture

Assigned: biguzis » Unassigned
Status: Needs work » Needs review
FileSize
3.24 KB

Rerolled.
Interdiff not available because #9 patch can't be applied.

mikeryan’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll

Thanks for the reroll - now on to the tests!

quietone’s picture

Assigned: Unassigned » quietone

Will have a go at the test.

quietone’s picture

Status: Needs work » Needs review
FileSize
4.77 KB
8.19 KB
5.28 KB

Adding a test for processRow and EntityContentBase. Also added some comments.

The last submitted patch, 27: 2711353-27-fail.patch, failed testing.

mikeryan’s picture

Assigned: quietone » mikeryan
mikeryan’s picture

Assigned: mikeryan » Unassigned
Status: Needs review » Needs work
Issue tags: -Needs tests
+++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentBaseTest.php
@@ -159,9 +160,9 @@ public function testTranslated() {
   /**
-   * Tests creation of ID columns table with definitions taken from entity type.
+   * Tests empty destinations.
    */
-  public function testEntityWithStringId() {
+  public function testEmptyDestinations() {

It looks like this patch is replacing an existing test rather than adding a new one?

quietone’s picture

Status: Needs work » Needs review
FileSize
7.93 KB
3.98 KB

Thanks, testEntityWithStringId() is restored to its former glory.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Version: 8.4.x-dev » 8.3.x-dev
Issue tags: +rc deadline

As an API addition for a beta experimental module, this one can go in during beta. So, moving to 8.3.x and tagging as rc deadline. If it's not in by RC then we would discuss whether to move to 8.4.x. Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/migrate/src/Row.php
@@ -78,6 +78,13 @@ class Row {
+  protected $emptyDestinations = [];

@@ -229,6 +236,26 @@ public function removeDestinationProperty($property) {
+  public function setEmptyDestinationProperty($property) {
+    $this->emptyDestinations[] = $property;
...
+  public function getEmptyDestinationProperties() {
+    return $this->emptyDestinations;

Let's make the property match the getter/setter - ie. protected $emptyDestinationProperties since they are not empty destinations.

jofitz’s picture

Status: Needs work » Needs review
FileSize
7.82 KB
959 bytes

Set the property to match the getter/setter, as recommended by @alexpott.

jofitz’s picture

FileSize
7.96 KB
959 bytes

Scrub that. I managed to screw up the patch. Try this.

The last submitted patch, 36: 2711353-36.patch, failed testing.

mikeryan’s picture

Issue tags: +Needs change record

Since there's a change in behavior, safest to have a change record in case anyone was counting on that behavior.

jofitz’s picture

Assigned: Unassigned » jofitz
xjm’s picture

Version: 8.3.x-dev » 8.4.x-dev

Since 8.3.x is now in commit freeze for its release candidate phase and Migrate is in beta, this disruptive change should now be targeted for 8.4.x. Thanks!

xjm’s picture

Issue tags: -rc deadline
jofitz’s picture

Assigned: jofitz » Unassigned

Unassigning myself. Sorry, I think I bit off more than I can chew by offering to write the Change Record for this issue.

quietone’s picture

Assigned: Unassigned » quietone

I'll have a go at the change record

quietone’s picture

Assigned: quietone » Unassigned

A start at the change record, https://www.drupal.org/node/2858640.

quietone’s picture

Title: Migrate never unsets existing data » Migrate never unsets existing data for content entitites

config entities and raw config entities are also broken but probably needs to be different issue and less severe because this only comes up in reruns, the migrate_drupal path does not really support reruns.

Made a follow up to look into the config entities. #2858708: Migrate never unsets existing data for config entities

Pavan B S’s picture

+++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
@@ -420,6 +420,37 @@ public function testProcessRowEmptyPipeline() {
+    $expected = array(
+      'test' => 'test destination',
+      'test1' => 'test1 destination',
+      'test2' => NULL,
+    );

use short array syntax (new coding standard).
Applying the patch, please review.

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Self-assigning for review.

phenaproxima’s picture

Assigned: phenaproxima » Unassigned
Status: Needs review » Needs work

Looks great. I have only minor requests.

  1. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentBaseTest.php
    @@ -204,4 +205,65 @@ public function testEntityWithStringId() {
    +          // type, the import will fail with a SQL exception.
    

    Nit: Should be "an SQL exception".

  2. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentBaseTest.php
    @@ -204,4 +205,65 @@ public function testEntityWithStringId() {
    +    $this->assertSame('foo', $entity->get('version')->value);
    

    More nits: this can just be $entity->version. No need for the get() call. But hey, it works, so whatever.

  3. +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
    @@ -420,6 +420,37 @@ public function testProcessRowEmptyPipeline() {
    +      $plugins[$key][0] = $this->getMock('Drupal\migrate\Plugin\MigrateProcessInterface');
    +      $plugins[$key][0]->expects($this->once())
    +        ->method('getPluginDefinition')
    +        ->will($this->returnValue([]));
    +      $plugins[$key][0]->expects($this->once())
    +        ->method('transform')
    +        ->will($this->returnValue($value));
    

    Let's use Prophecy here ($this->prophesize()) for greater readability.

  4. +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
    @@ -420,6 +420,37 @@ public function testProcessRowEmptyPipeline() {
    +    $this->migration->expects($this->once())
    +      ->method('getProcessPlugins')
    +      ->with(NULL)
    +      ->will($this->returnValue($plugins));
    

    And here.

  5. +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
    @@ -420,6 +420,37 @@ public function testProcessRowEmptyPipeline() {
    +    $this->assertSame(2, count($row->getDestination()));
    

    Let's use assertCount() here.

yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
FileSize
7.86 KB
3.12 KB

Updated patch as per comment #49 & also added interdiff.

phenaproxima’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
    @@ -429,11 +429,11 @@ public function testProcessRowEmptyDestination() {
    +      $prophecy = $this->prophesize(MigrateProcessInterface::CLASS);
    

    ::CLASS should be lowercase.

  2. +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
    @@ -429,11 +429,11 @@ public function testProcessRowEmptyDestination() {
    +      $prophecy->expects($this->once())
    

    With Prophecy, we don't need to use things like $this->once(). It's a lot more readable:

    $prophecy->getPluginDefinition()->willReturn([])

  3. +++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
    @@ -446,7 +446,7 @@ public function testProcessRowEmptyDestination() {
    +    $this->assertCount(2, count($row->getDestination()));
    

    assertCount() doesn't require a separate call to count(). It should just be this:

    $this->assertCount(2, $row->getDestination())

yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
7.79 KB
1.25 KB

Thanks @phenaproxima for the suggestions. please check the updated patch & interdiff.

The last submitted patch, 51: migrate_never_unsets-2711353-51.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 53: migrate_never_unsets-2711353-53.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
FileSize
7.79 KB
1.45 KB

Set up the test prophecy (drawing information from the previous method testProcessRowPipelineException()).

Status: Needs review » Needs work

The last submitted patch, 56: 2711353-56.patch, failed testing.

jofitz’s picture

Status: Needs work » Needs review
FileSize
7.82 KB
1.3 KB

Correct the test errors.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record
+++ b/core/modules/migrate/tests/src/Unit/MigrateExecutableTest.php
@@ -440,6 +440,33 @@ public function testProcessRowPipelineException() {
+      $this->assertSame($row->getDestinationProperty($key), $value);

Nit: These are supposed to be in expected, actual order (i.e., $value, $row->getDestinationProperty()), but it'll still work. Fixable on commit.

Otherwise, 'tis a fine thing!

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record updates

...but it'd be nice to add a little sample code to the change record.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record updates

Altered the change record to be a little clearer. I think this is ready to go.

quietone’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.82 KB
726 bytes

While this is RTBC, it is a small change to fix the assertion, #59. And, I really, really like the arguments in the expected order.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Coolio. Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 62: 2711353-62.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

Errors looked unrelated

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -387,8 +387,13 @@ public function processRow(Row $row, array $process = NULL, $value = NULL) {
           // No plugins or no value means do not set.
    

    This is now no longer true as far as I can see... or at least it needs to be moved. I think a bit more commentary as to what is going on.

  2. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -387,8 +387,13 @@ public function processRow(Row $row, array $process = NULL, $value = NULL) {
    +      if ($plugins) {
    

    If $plugins is not set shouldn't we call ->setEmptyDestinationProperty() - I don't know - thoughts?

mikeryan’s picture

This is now no longer true as far as I can see... or at least it needs to be moved. I think a bit more commentary as to what is going on.

Yes, I think this comment needs to be updated.

If $plugins is not set shouldn't we call ->setEmptyDestinationProperty() - I don't know - thoughts?

I don't believe that's necessary - I have fielded questions from various people about why fields they didn't map in an update migration resulted in existing values being cleared before, and explained the whole system-of-record is source thing, so it seems calling setEmptyDestinationProperty() is unnecessary. Don't immediately see why... but, we should add an assertion for this case (that removing a field mapping from the process pipeline causes the field value to be cleared on a reimport).

rakesh.gectcr’s picture

Assigned: Unassigned » rakesh.gectcr

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

Assigned: rakesh.gectcr » quietone

Look like rakesh.gectcr hasn't had time to work on this. On the migrate call, I decided to take this on.

quietone’s picture

Review this with rakesh.gectcr and we agree that there are 2 things left to do.

1) Update the comments (see #66.1 and #67.1)
2) Update the CR

rakesh.gectcr’s picture

Assigned: quietone » rakesh.gectcr
rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
7.95 KB
523 bytes

IMHO, The CR is fine, I have read and changed the comment block according to that.

Status: Needs review » Needs work

The last submitted patch, 73: 2711353-73.patch, failed testing. View results

maxocub’s picture

Issue tags: +Vienna2017

Adding a tag so we can see this issue on our Vienna migrate sprint kanban.

rakesh.gectcr’s picture

Status: Needs work » Needs review
FileSize
8.18 KB
607 bytes
0 bytes
rakesh.gectcr’s picture

jofitz’s picture

Assigned: rakesh.gectcr » Unassigned
Status: Needs review » Reviewed & tested by the community

Patch addresses @alexpott's first concern from #66 (@mikeryan addressed his second concern in #67) so this can (finally) be set back to RTBC.

phenaproxima’s picture

+++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentBaseTest.php
@@ -203,4 +205,65 @@ public function testEntityWithStringId() {
+    $executable = new MigrateExecutable($migration, new MigrateMessage());

Sorta-nit: We don't need to pass a new MigrateMessage() in anymore -- MigrateExecutable will create it on its own.

Not enough to derail RTBC though :)

jofitz’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
8.13 KB
750 bytes

Let's tidy that up (for completeness).

rakesh.gectcr’s picture

FileSize
7.91 KB
1.53 KB
607 bytes

Cool, @phenaproxima agreed..... :)

Here we go with a little cleaner. :D

Putting back to needs review for running the patch.

rakesh.gectcr’s picture

We need to remove the new MigrateMessage() one more place

@JO You are super fast! Cheers!

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for humoring me, guys :) Back to RTBC on the assumption that Drupal CI will pass it.

  • catch committed 3d524aa on 8.5.x
    Issue #2711353 by rakesh.gectcr, Jo Fitzgerald, quietone, chx, Yogesh...

  • catch committed d0dc1f1 on 8.4.x
    Issue #2711353 by rakesh.gectcr, Jo Fitzgerald, quietone, chx, Yogesh...
catch’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

  • catch committed c783750 on 8.5.x
    Revert "Issue #2711353 by rakesh.gectcr, Jo Fitzgerald, quietone, chx,...

  • catch committed acafddc on 8.4.x
    Revert "Issue #2711353 by rakesh.gectcr, Jo Fitzgerald, quietone, chx,...
catch’s picture

Status: Fixed » Needs work

8.4.x wasn't happy so I've reverted from both branches for now - might need an 8.4 backport.

jofitz’s picture

Assigned: Unassigned » jofitz

I'll have a go at an 8.4 backport.

jofitz’s picture

Assigned: jofitz » Unassigned

I was expecting to do a re-roll for 8.4, but the patch applied perfectly. Have set the testbot running the patch from #82 against 8.4.

jofitz’s picture

Status: Needs work » Needs review
FileSize
7.92 KB
1.13 KB

Here is the patch for 8.4.x (which will also work for 8.5.x just with a redundant parameter passed to MigrateExecutable).

In 8.5.x the second parameter of MigrateExecutable(), $message, is now optional, but it was required in 8.4.x.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 92: 2711353-91.patch, failed testing. View results

jofitz’s picture

Status: Needs work » Reviewed & tested by the community

It is no longer possible to have one single patch that applies to both 8.4.x and 8.5.x. The patch in #92 applies to 8.4.x, the patch in #82 applies to 8.5.x (I am re-running the tests on this patch to be sure). I think it is OK to return this to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed the separate patches to both branches, thanks for the backport.

  • catch committed 990aa5c on 8.5.x
    Issue #2711353 by Jo Fitzgerald, rakesh.gectcr, quietone, chx, Yogesh...

  • catch committed 9925ccb on 8.4.x
    Issue #2711353 by Jo Fitzgerald, rakesh.gectcr, quietone, chx, Yogesh...

Status: Fixed » Closed (fixed)

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

quietone’s picture

Published change record.