The D6 and D7 user migrations skip import of uid 1, because we don't want to overwrite the D8 admin account. If all Drupal websites followed best practices, and basically only used the uid 1 admin account for install.php/update.php, this would be fine. However, in a large proportion of real-world Drupal websites, the admin account has been used to author content. If that account had a different username that the D8 admin account, the original D8 username is displayed. If that account had profile fields, they are not applied to the D8 account.

The problem here is, the right thing to do is going to be site-specific. The current behavior will be correct (well, almost, see below) for a lot of sites. Others may want at least the username migrated, perhaps email address, quite likely any profile fields if they exist. Those who've seen the light might want to migrate the original admin account to a fresh non-admin account, to follow best practices going forward.

Even when skipping the admin account entirely is correct, we do have some problems with how that works now. Because the admin account is not recording in the user map table, attempts to use the migration process plugin for user references will fail, even though they should succeed because the D8 uid 1 does correspond to the source site's uid 1. So, minimally, in this case we should make a dummy map row (sourceid1=1, destid1=1) to fix this.

Beyond that, when we build custom Drupal-to-Drupal migration tools in contrib, we'll want to offer an array of options. What is the minimum we need to core to make it possible to do this? And, should we revisit whether skipping uid 1 is the right default for the core upgrade case?

CommentFileSizeAuthor
#47 2560637-47.patch10.55 KBphenaproxima
#45 interdiff-2560637-39-45.txt1.3 KBphenaproxima
#45 2560637-45.patch10.53 KBphenaproxima
#39 interdiff-2560637-33-39.txt3.29 KBphenaproxima
#39 2560637-39.patch8.83 KBphenaproxima
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 117,193 pass(es). View
#33 2560637-33.patch6.99 KBphenaproxima
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 117,328 pass(es). View
#31 2560637-31.patch7.75 KBphenaproxima
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 117,336 pass(es). View
#29 2560637-29.patch8.13 KBphenaproxima
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 117,364 pass(es). View
#21 interdiff-2560637-18-21.txt2.2 KBphenaproxima
#21 2560637-21.patch6.36 KBphenaproxima
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 115,890 pass(es). View
#18 2560637-18.patch6.36 KBphenaproxima
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,068 pass(es). View
#14 2560637-14.patch4.42 KBphenaproxima
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,458 pass(es). View
#11 2560637-11.patch2.15 KBphenaproxima
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,870 pass(es), 444 fail(s), and 25 exception(s). View
#6 2560637-6.patch3.39 KBphenaproxima
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2560637-6.patch. Unable to apply patch. See the log in the details link for more information. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

mikeryan created an issue. See original summary.

webchick’s picture

Issue tags: +Migrate critical

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.

mikeryan’s picture

Note that #2 is a different aspect of uid 1 WTFness - I was speaking of the fact that the source plugin excludes uid 1 from the query (affecting real site migrations), while #2 is about the fact that it isn't even in the dumps for testing (affecting tests).

phenaproxima’s picture

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.

neclimdul’s picture

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.

phenaproxima’s picture

Status: Active » Postponed
Issue tags: +Needs tests
FileSize
3.39 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2560637-6.patch. Unable to apply patch. See the log in the details link for more information. View

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.

phenaproxima’s picture

Status: Postponed » Needs review

Unblox0r3d.

Status: Needs review » Needs work

The last submitted patch, 6: 2560637-6.patch, failed testing.

The last submitted patch, 6: 2560637-6.patch, failed testing.

phenaproxima’s picture

Issue tags: +Needs reroll
phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.15 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,870 pass(es), 444 fail(s), and 25 exception(s). View

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 11: 2560637-11.patch, failed testing.

The last submitted patch, 11: 2560637-11.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
4.42 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,458 pass(es). View

This oughta pass the tests, at least...

benjy’s picture

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

phenaproxima’s picture

Status: Needs review » Needs work

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

Agreed.

mikeryan’s picture

Preumature for a full review, of course, but I like this approach.

phenaproxima’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
6.36 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,068 pass(es). View

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

quietone’s picture

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

benjy’s picture

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.

phenaproxima’s picture

Flipped the logic and renamed the configuration option to preserve_properties.

quietone’s picture

That is much easier to follow now, thx.

+1 RTBC

chx’s picture

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.

phenaproxima’s picture

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

benjy’s picture

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

phenaproxima’s picture

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

chx’s picture

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

phenaproxima’s picture

Status: Needs review » Needs work

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.

phenaproxima’s picture

FileSize
8.13 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 117,364 pass(es). View

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?

phenaproxima’s picture

Status: Needs work » Needs review

Er.

phenaproxima’s picture

FileSize
7.75 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 117,336 pass(es). View

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

phenaproxima’s picture

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.

phenaproxima’s picture

FileSize
6.99 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 117,328 pass(es). View

Re-did it, implementing #23.

quietone’s picture

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

quietone’s picture

Status: Needs review » Needs work

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

benjy’s picture

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

chx’s picture

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

phenaproxima’s picture

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

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
8.83 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 117,193 pass(es). View
3.29 KB

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

Status: Needs review » Needs work

The last submitted patch, 39: 2560637-39.patch, failed testing.

phenaproxima queued 39: 2560637-39.patch for re-testing.

phenaproxima’s picture

Status: Needs work » Needs review

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

benjy’s picture

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?

chx’s picture

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

benjy’s picture

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

phenaproxima’s picture

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Lets do it!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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

  • webchick committed 5e5738d on 8.0.x
    Issue #2560637 by phenaproxima, benjy, quietone, mikeryan, chx,...
webchick’s picture

Issue tags: +rc eligible

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.

Status: Fixed » Closed (fixed)

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