Migrate never unsets existing data for content entitites

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?

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

+++ 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?

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: 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).

Ok, sorry for the rant then :)

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...

#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

We'll work on this today.

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

Thanks for the reroll - now on to the tests!

Will have a go at the test.

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

+++ 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?

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

Looks good, thanks!

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!

+++ 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.

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

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

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

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!

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

I'll have a go at the change record

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

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

+++ 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.

Self-assigning for review.

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.

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

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())

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

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

Correct the test errors.

+++ 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!

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

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

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.

Coolio. Back to RTBC.

Errors looked unrelated

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?

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).

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

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

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

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

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

+++ 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 :)

Let's tidy that up (for completeness).

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

Here we go with a little cleaner. :D

Putting back to needs review for running the patch.

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

@JO You are super fast! Cheers!

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

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

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

I'll have a go at an 8.4 backport.

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.

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.

Back to RTBC.

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.

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

Published change record.