Problem/Motivation

Node authors don't always migrate properly.

Proposed resolution

Make node and comment migrations dependent on user migrations.

Remaining tasks

Updated node and comment configuration files, manually test.

User interface changes

None.

API changes

None.

Original report by @eliza411

Migrate documentation says that it doesn't matter what order you put migration in in the manifest file: https://www.drupal.org/node/2257723

Using a manifest created from the drush listing, node authors weren't migrating. I moved the user migrations to the top of the manifest and the node authors migrated as expected.

Either the documentation needs to be updated or something needs to be adjusted so that user migrations are run first. If it's a question of docs, I can make those updates, but I want to verify that it's indeed the case.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eliza411’s picture

Title: Node author not migrated unless user migration is run first » Migrate dependency order system doesn't seem to be working
Priority: Normal » Major
Issue summary: View changes
FileSize
1.9 KB
1.9 KB

After verifying that the order should not matter, I'm setting this to major and retitling.

What I'm seeing is that nodes aren't associated their authors when the user migrations are at the bottom of the file (except for user1), but they ARE associated when the user migrations are at the top.

Using the drush order (alphabetical)

  1. I created a .yml file based on drush's output.
  2. I commented out a few migrations that were either causing issues or that were unneeded - maybe I've introduced a problem with that.
  3. If I keep the default order (see the attached default.yml), then:
    • users are migrated
    • they are NOT associated with the nodes they have authored
    • user profiles load but text fields are blank

Moving user migrations to the top

I tried an experiment: I just moved the user migrations to the top of the file so that nodes would migrate later, did a drush si, and ran the migration. (See the attached user.yml manifest which has the same commented out migrations as the first) In this case:

  1. users are migrated
  2. they ARE associated with the nodes they have authored
  3. user profiles don't load and logs show:
    LogicException: A stray drupal_render() invocation with $is_root_call = TRUE is causing bubbling of attached assets to break. in drupal_render() (line 2861 of /var/www/eight/drupal/core/includes/common.inc).
    
    Recoverable fatal error: Argument 7 passed to Drupal\Core\Field\FormatterBase::__construct() must be of the type array, null given, called in /var/www/eight/drupal/core/lib/Drupal/Core/Field/FormatterPluginManager.php on line 72 and defined in Drupal\Core\Field\FormatterBase->__construct() (line 65 of /var/www/eight/drupal/core/lib/Drupal/Core/Field/FormatterBase.php).
    
    Notice: Undefined index: third_party_settings in Drupal\Core\Field\FormatterPluginManager->createInstance() (line 72 of /var/www/eight/drupal/core/lib/Drupal/Core/Field/FormatterPluginManager.php).
  4. editing the text fields themselves causes the following errors:
    Notice: Undefined index: third_party_settings in Drupal\Core\Field\WidgetPluginManager->createInstance() (line 130 of /var/www/eight/drupal/core/lib/Drupal/Core/Field/WidgetPluginManager.php).
    
    Recoverable fatal error: Argument 5 passed to Drupal\Core\Field\WidgetBase::__construct() must be of the type array, null given, called in /var/www/eight/drupal/core/lib/Drupal/Core/Field/WidgetPluginManager.php on line 130 and defined in Drupal\Core\Field\WidgetBase->__construct() (line 54 of /var/www/eight/drupal/core/lib/Drupal/Core/Field/WidgetBase.php).
  5. Here's a comparison of the actual order they ran in:
    https://docs.google.com/spreadsheets/d/1BtLemKzDWUQiIYELpowVU26qYzholMyz...

eliza411’s picture

Category: Support request » Bug report

Changing to a bug report

ultimike’s picture

Issue summary: View changes

chx and I talked about this issue earlier this week during the weekly hangout, from what I gathered, we need to make all migrations that involve "entities with authors" dependent on users because "entities do not want to store invalid data" (chx mentioned this was related to a core bug). This includes nodes, node revisions, and comments (any others?)

I'll go ahead and update the issue summary (chx - what is the core bug that you referred to in the hangout?) and then do some manual testing to confirm that adding the dependencies solve this issue.

Thanks,
-mike

chx’s picture

We agreed on starting with adding a user dependency to node , node_revision, and comments.

ultimike’s picture

FileSize
703 bytes

I've banged on this one for a few hours and I'm not convinced this is a dependency issue.

I have a simple D6 site with a handful of users, nodes, revisions, and comments that I've been testing with. I first tried to duplicated eliza411's issue of things not getting their proper authors using her default.yml manifest. I was able to reproduce the issue she was having with nodes not getting the proper authors.

I then tried using eliza411's author.yml manifest, but that didn't fix the missing node authors for me.

I then tried adding d6_user as a dependency on d6_node, d6_node_revision, and d6_node_comment and that didn't fix the missing node authors either.

After some digging, I found that the d6 node source plugin was pulling the user id for node from the node revision table, not the node table. On my d6 site, the user id of the node revision is set at the user actually making the revision - not necessarily the user listed as the author of the revision.

I've attached a patch that solves this issue for me. I'd like to get weal's feedback on this because I seem to recall that he worked a similar issue

Thanks,
-mike

ultimike’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: 2376141-5.patch, failed testing.

ultimike’s picture

I dug into this one a bit more. I've been unable to find time to chat with weal about this yet.

DB structure differences

In D6:

  • The uid of the user who created the revision is stored in the node_revisions table (uid). This is the user who actually created the revision, not the listed author of the content at that revision.
  • The current listed author of the content is stored in the node table (uid).

In D8::

  • The uid of the user who created the revision is stored in the node_revision table (revision_uid). This is the user who actually created the revision, not the listed author of the content at that revision.
  • The author of the content is stored in the node_field_data table (uid). This value changes from revision to revision depending on who the current listed author is.
  • The listed author for each revision is stored in the node_field_revision table (uid).
  • No author/uid information is stored in the node table.

So, in D6, there is no information about who the listed author is at each revision, only the current revision. In D8, we know who the listed author is at each revision (node_field_revision table).

Manual testing

Starting with a simple D6 site with a single node with 4 revision, where the current author is uid=2, I ran the migration to Drupal 8 and while the node appears to migrate fine, the author is incorrect. When attempting to view the list of revisions (node/2/revisions), I get a PHP fatal error: http://note.io/1sHbWZm

I'm pretty sure the author is incorrect because the node source plugin is pulling the uid from the node_revision table, not the node table. In fact, I think that we need to be pulling both node.uid (content author) and node_revisions.uid (revision author) and mapping those to the proper D8 fields.

-mike

ultimike’s picture

Status: Needs work » Needs review
FileSize
3.77 KB

Patch attached that takes into account both the node author uid and the revision author uid.

I was surprised that the d6_node migration wasn't dependent on the d6_user migration, so I added that as well.

Based on what I found, I'm still not convinced that there is an dependency/ordering issue. I've been unable to reproduce the behavior that eliza411 reported.

I've run MigrateNodeTest, MigrateNodeRevisionTest, MigrateDrupal6Test, and a manual test locally - all passed.

-mike

Status: Needs review » Needs work

The last submitted patch, 9: 2376141-9.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
5.72 KB
5.72 KB

Thanks. It seems you got almost there; only tests failing for test reasons; I fixed them.

chx’s picture

FileSize
1.94 KB

Hrm. That wanted to be the interdiff. Sorry!

chx’s picture

FileSize
3 KB
6.77 KB

Here's some test coverage.

chx’s picture

FileSize
1003 bytes
6.79 KB

Since d6_user has uid: uid and in general migrate_drupal (but not migrate) keeps numeric entity ids I have moved the migration lookup of uids into a comment keeping it because it's quite likely people will start at node to learn about migration (thanks benjy for the suggestion!).

benjy’s picture

Title: Migrate dependency order system doesn't seem to be working » Fix node and node revision author ids
Status: Needs review » Reviewed & tested by the community

Patch fixes the author id problems which I think should be the main focus of this issue. Test coverage has been added, RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2376141_14.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
6.79 KB

Straight re-roll

chx’s picture

FileSize
8.12 KB
2.74 KB

The test I wrote, that was testing bupkis.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/migrate_drupal/config/install/migrate.migration.d6_node.yml
@@ -13,7 +13,11 @@ process:
+  uid: node_uid
+# migrate_drupal keeps numeric entity ids. If it wouldn't then:
+#    plugin: migration
+#    migration: d6_user
+#    source: node_uid

+++ b/core/modules/migrate_drupal/config/install/migrate.migration.d6_node_revision.yml
@@ -13,7 +13,10 @@ process:
+  uid:
+    plugin: migration
+    migration: d6_user
+    source: node_uid

I don't understand how this works in one place but not the other. Testing with the suggested structure in the comments passes all the migrate_drupal tests.

benjy’s picture

That should just be removed from node_revision or commented out the same as node. It works with or without the code because we map the uid in D6/D7 to the uid in D8 therefore the ids are the same in both the source and destination sites. This makes the lookup superfluous.

Same for revision_uid.

benjy’s picture

Status: Needs work » Needs review
FileSize
8 KB
901 bytes

Removed as mentioned in #21

chx’s picture

Status: Needs review » Reviewed & tested by the community

Looks like Alex's concerns are addressed.

webchick’s picture

Assigned: Unassigned » alexpott

Hm. Not sure Alex's concerns were totally addressed. He seemed to also be asking why we comment that stuff out, since it seems to work fine either way. Moving back to him for further comment. (The commented out stuff also gave me pause, since we don't do that anywhere else that I'm aware of?)

chx’s picture

It's left there as an explanation and with a comment. Yes we don't typically do it but we felt that nodes will be the place people look first for uid migrations and custom migrations will need the lookup.

benjy’s picture

Yeah, I do think it's worthwhile leaving, core serves as a great example for developers and the node migration is likely to be a popular example.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/migrate_drupal/config/install/migrate.migration.d6_node.yml
@@ -13,7 +13,11 @@ process:
+  uid: node_uid
+# migrate_drupal keeps numeric entity ids. If it wouldn't then:
+#    plugin: migration
+#    migration: d6_user
+#    source: node_uid

Honestly, I have no idea what to do with this information. At the moment it just makes me think where does migrate_drupal keep these ids. To actually be helpful to people writing migrations the comment needs to be more verbose.

benjy’s picture

Proposed improved comment.

"Core migrations are designed for replacing the upgrade path and therefore all node and user ids are preserved. For that reason we do not need to look-up the user id for the node. If you're writing a custom migration, user ids will vary from the source site and a lookup as shown below will be required."

benjy’s picture

Status: Needs work » Needs review
FileSize
8.24 KB
888 bytes

Re-roll with proposed comment in 28.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

@alexpott and @chx approved the comment in IRC, setting back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Migrate is excluded from beta evaluation. Committed 498bcc7 and pushed to 8.0.x. Thanks!

  • alexpott committed 498bcc7 on 8.0.x
    Issue #2376141 by chx, benjy, ultimike, eliza411: Fix node and node...

Status: Fixed » Closed (fixed)

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