Improve handling of uid 1 during migration

I learned in #2423103: Migration Files for Drupal 7 Content that we don't export uid 1 currently at all from the dump scripts, which is a pretty big deviation since literally every Drupal database has uid 1 (often with nodes/comments attributed to them).

Escalating to a Migrate critical.

One possibility is to add configuration to the entity:user destination detailing how to handle uid 1. Something like this:

destination:
  plugin: entity:user
  overwrite_properties:
    1:
      - name
      - mail

This would instruct the destination to allow only the name and mail properties of user 1 to be overwritten by data from the migration.

I'm actually going to argue a bit different. Just migrate the whole account an overwrite it unconditionally. If people are using best practices, then they're still using best practices. If they aren't, they're going to be frustrated and complain and work around it afterwards anyways.

Also, that will allow us to migrate d.o someday. We love you Dries.

Blocked by #2562695: migrate-db.sh skips uid 1 but shouldn't, which is needed in order to create tests.

I talked about this a little with the good people of IRC, and we concluded that by default we want to migrate uid 1 as it exists in the source site, so as to keep the upgrade path as non-disruptive as possible. But at the same time we want to give power users the option of controlling the properties that get overwritten. This patch adds such overwrite-protection support to all content entity destinations (in order to avoid repeating code), and removes the explicit skip over uid 1 from the source plugins.

Unblox0r3d.

Re-rolled.

This oughta pass the tests, at least...

This approach makes sense, we had something similar in D7 with System of Record. NW because we definitely need some good test coverage around this.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -109,7 +109,18 @@ public function getIds() {
+    // Allow the migration to specify the properties to be overwritten.
+    if (isset($this->configuration['overwrite'])) {
+      $overwrite = $this->configuration['overwrite'];
+    }
+    else {
+      $overwrite = array_keys($destination);
+    }

Lets improve the comments to say, if overwrite is not set then we overwrite every field.

It might also be worth some docs somewhere that explains how'd you go about overwriting all but one field, probably a hook_entity_ENTITY_TYPE_load().

NW because we definitely need some good test coverage around this.

Agreed.

Added a test, but I'm not sure if it's enough. Any other test case ideas are welcome!

if overwrite is not set then we overwrite every field.

Please, can we make this clearer, change the variable name to reflect what it does instead of what it doesn't do? The default case is to overwrite everything, so the new key is about declaring what fields are to be preserved/saved. So why not something like 'preserve' or 'save'?

Tests look good. I also agree with quietone, preserve seems the more logical way to approach this although I do wonder if we should be more explicit like preserve_fields ? Probably good to get some DX import from others on that one.

Flipped the logic and renamed the configuration option to preserve_properties.

That is much easier to follow now, thx.

+1 RTBC

I would be much happier with an overwrite properties configuration which supports deep properties. Here's why:

if (isset($this->configuration['overwrite_only_these'])) {
  // Alternatively, create a "half clone" method, cloneRowWithoutDestination
  $new_row = (new Row($row->getSource(), ???, $row->isStub()))->freezeSource();
  foreach ($this->configuration['overwrite_only_these'] as $property) {
    // Look how pretty this is.
    $new_row->setDestinationProperty($property, $row->getDestinationProperty($property));
  }
  $row = $new_row;
}

Supporting deep with preserve has some serious challenges.

@chx: I don't follow...what is the benefit?

+1 to supporting nested properties, that's a good idea.

I'm in favor of nested property support as well, I simply a) don't understand how chx's code snippet accomplishes this, and b) why that necessitates "overwrite" logic rather than "preserve".

@phenaproxima sorry I edited my comment: I got to this from wanting to support deep properties (as we do everywhere) and I can't readily see how to (easily) implement that for preserve.

Ahhh, OK -- looking at the code of Row::setDestinationProperty(), I see why this makes sense. Yeah, I like that approach as well, and I can see why preserve logic would be much harder with nested properties.

Will alter the patch tomorrow.

OK, took a slightly different path here. By adding a method to clear destination properties (Row::clearDestinationProperty), we can support both overwrite and preserve logic. The tests pass, so...maybe this will please everyone?

Er.

D'oh! Forgot to remove Row::cloneSource(), which is an artifact from my implementation of @chx's idea in #23.

I just realized that my attempt to, um, preserve the preservation logic may not actually work, and we may only be able to support overwrite logic. Because to preserve nested destination properties, we'd need to be able to get a list of every single destination property at every level in order to avoid overwriting nested properties that are not listed in preserve_properties.

Re-did it, implementing #23.

I totally agree with chx about supporting deep properties.
Happy with this the way it is.

benjy, just pointed out on IRC that this 'needs tests for the nested property aspect'.

I think we should add tests for the nested property aspect that we now support.

Also, I think we need to document an example of how to use this, maybe in the codebase? and then definitely here somewhere: https://www.drupal.org/node/2127611

Please do not use the name of a living person in the tests (or anywhere).

Please do not use the name of a living person in the tests (or anywhere).

So it's OK if I change it to Graham Chapman? =P

Fixed! And added a test of nested property overwrites, too :)

PIFR has been shut off, so I don't think we'll be seeing any more bogus failures. :)

RTBC for me, maybe we should have a change record here as another way to document this functionality, and then a link to the migrate docs?

Add a testmodule.info.yml, a testmigration under it instead of the overrides in PHP code. It's imperative we show people how to use stuff. If core doesn't do something contrib will rarely do it (vica versa: if core does something consistently, contrib will copy it irregardless whether it's relevant or not).

Like this?

The sample module and sample migration look good, I just think we should use them in the test.

How's this?

Lets do it!

Awesome, really happy to see this fixed. The overwrite ability seems like it may come in handy other places too, not just here.

Committed and pushed to 8.0.x. Thanks! :D

Oops, forgot my tags. Since this is a) only touching Migrate, which is an experimental module and b) a "critical" (as in Migrate critical), it is eligible for RC.