Problem/Motivation

I found this in attempt to manually test node migration as in #2225165: Missing titles in simple D6->D8 node migration. I setup a new D6 site with admin_menu, cck (text and number), devel_generate. I set up a new D8 instance with latest HEAD and ran:

drush migrate-manifest --legacy-db-url=mysql://..:..@localhost/d6 ../D6Manifest-Node_0.yml

The manifest is from #2222375-6: Manual Testing: Nodes and I slightly modified it as per migrate_upgrade's manifest. I changed d6_cck_field_values:* and d6_cck_field_revision:* to d6_cck_field_values and d6_cck_field_revision respectively.

The first run failed and I assumed there was something wrong with field migrations as I had not actually added any cck fields to nodes yet. I did that, regenerated 50 nodes and ran migration again (and also with the UI module just in case). It eventually showed that node migration was successful but did not actually migrate any rows. I found that the map table had set all source rows to STATUS_IGNORED (2).

On debugging, I found the reason to be that the devel_generate had generated all nodes with format = 0 and this process would skip the row:

  'body/format':
    plugin: migration
    migration: d6_filter_format
    source: format

On searching, I found another issue: #2207823: Handle bogus/missing D6 format values which went about it quite differently (the patch in the issue is overwritten with a different approach in the current HEAD). Current relevant block:

  signature_format:
    -
# Drupal 6 stores 0 as the signature format when signatures are disabled.
# Drupal 8 stores a NULL.
      plugin: static_map
      bypass: true
      source: signature_format
      map:
        0: NULL
    -
# Do not try to migrate the NULL format.
      plugin: skip_process_on_empty
    -
      plugin: migration
      migration: d6_filter_format
      no_stub: 1

The reasoning seems okay for signature field (as it is assumed disabled if the format is 0, but in case of nodes, there could be valid reasons to migrate the nodes even if this happens.

Proposed resolution

We could use the same approach as the current implementation in migration.migrate.d6_user.yml as shown in the block above.

Remaining tasks

Write the patch.

User interface changes

None

API changes

None

CommentFileSizeAuthor
#17 2306049-17.patch2.62 KBultimike
#15 2306049-15.patch2.62 KBultimike
#10 interdiff.txt534 bytesbenjy
#10 2306049-10.patch746 bytesbenjy
#1 d6_d8_node_migration-2306049-1.patch790 byteshussainweb
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented
Status: Active » Needs review
FileSize
d6_d8_node_migration-2306049-1.patch790 bytes

Putting in a basic patch just for testing. It worked in manual testing. All nodes migrated using the drush command shown in the issue summary.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented

The patch works and manually migrating from D6 -> D8 works fine. However, I think this wouldn't handle non-zero missing filter id's. Is there a way for 'migration' process to skip process instead of skipping row?

benjy’s picture

benjy CreditAttribution: benjy commented 
+++ b/core/modules/migrate_drupal/config/install/migrate.migration.d6_node.yml
@@ -17,9 +17,18 @@ process:
+      no_stub: 1

I know you just copied this wholesale from the d6_user migration but I'm not even sure where this is used? It doesn't seem to be documented either: https://www.drupal.org/node/2149801

Is there a way for 'migration' process to skip process instead of skipping row?

Isn't that exactly what you're doing with skip_process_on_empty. ?

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented

@benjy: You are right about no_stub. It was copied from d6_user migration. I just did that to get this started. There seems to be a reference to this in Drupal\migrate\Plugin\migrate\process\Migration:

    if (!$destination_ids && (($self && empty($this->configuration['no stub'])) || isset($this->configuration['stub_id']) || count($migrations) == 1))  {
      // If the lookup didn't succeed, figure out which migration will do the
      // stubbing.

Of course, it seems it was changed later but the reference from 'no stub' doesn't seem to have been updated. The patch in #2207823-4: Handle bogus/missing D6 format values shows the original key in yml file as 'no stub', but currently it is no_stub. Like I said, I just copied it but this could be another issue?

Isn't that exactly what you're doing with skip_process_on_empty. ?

Yes, but I have to set up this elaborate structure to do that and it all happens before migration plugin is even called. Even with this, all scenarios are not handled like I explained. For non-zero format id's, it will still fall to migration and skip the entire row rather than just the process.

I think what is required here is having an option on plugin like this:

    plugin: migration
    migration: d6_filter_format
    source: format
    fail_behavior: skip_process

if fail_behaviour is skip_process, the appropriate exception is thrown instead. New issue for this too?

benjy’s picture

benjy CreditAttribution: benjy commented

A default_value option on the migration plugin would work? That would be more inline with what we've done elsewhere, eg static_map

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb commented

@benjy: That is more or less what I mean but I think default_value sounds a bit misleading. If the migrate process plugin fails, we are not going to assign a default value, but skip the process entirely (or the whole row, by default). I understand that skipping the process amounts to just setting a default value on that field, but there may be cases when the default value should be left to the runtime (i.e., when the destination is created, and not defined by the mapping.)

What do you think?

chx’s picture

chx CreditAttribution: chx commented

Replace 0 with the D6 default format in the source prepareRow?

benjy’s picture

benjy CreditAttribution: benjy commented

Lets just go with skipping the whole row if the migration look up fails.

And yes, I think we need a new issues for the "no stub" issue.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Needs review » Needs work
benjy’s picture

benjy CreditAttribution: benjy commented
Status: Needs work » Postponed
FileSize
2306049-10.patch746 bytes
interdiff.txt534 bytes

If we fix the stub issue then the Migration process plugin will skip the row for us when the filter lookup misses and no_stub is true. Patch attached but postponing on #2337749: no_stub is never used

benjy’s picture

benjy CreditAttribution: benjy commented
Status: Postponed » Needs review
benjy’s picture

benjy CreditAttribution: benjy commented
Assigned: hussainweb » Unassigned
chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

I was wondering whether normal d6 would create 0 format but no, that's just not happening, it's still a form driven Drupal and the filter options are coming from a serial and serials are fundamentally unzero (unlike users.uid which causes so much pain).

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Correct me if I'm wrong, but it seems like we should have tests for this? Esp. since we'd want it to handle the same condition in D7 as well, ideally.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
Status: Needs work » Needs review
FileSize
2306049-15.patch2.62 KB

Patch re-rolled with test.

-mke

benjy’s picture

benjy CreditAttribution: benjy commented 
+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateNodeTest.php
@@ -79,12 +80,15 @@ public function testNode() {
+    $this->assertEqual($node->body->format, NULL);

Can we change this to assertIdentical. Over in #2345833: Convert assetEqual to assertIdentical in migrate_drupal i'm planning on getting rid of as many assertEquals as possible, especially when the values we're comparing are 0, '', 1, true, false, null etc

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike commented
FileSize
2306049-17.patch2.62 KB

Benjy - no problem - patch updated with assertIdentical.

No interdiff, but the only change is that one line.

-mike

Status: Needs review » Needs work

The last submitted patch, 17: 2306049-17.patch, failed testing.

Status: Needs work » Needs review

benjy queued 17: 2306049-17.patch for re-testing.

benjy’s picture

benjy CreditAttribution: benjy commented
Status: Needs review » Reviewed & tested by the community

Test looks fine to me, should be green so RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 6fa8507 and pushed to 8.0.x. Thanks!

  • alexpott committed 6fa8507 on 8.0.x 
    Issue #2306049 by ultimike, benjy, hussainweb: Fixed D6->D8 node...
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito commented

Is #2207823: Handle bogus/missing D6 format values a duplicate of this one?

Status: Fixed » Closed (fixed)

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