Taxonomy term hierarchy migration incomplete

I can reproduce this with the tip of 8.1.x, migrating my site from Drupal 6 - taxonomy term references are actually fine, nodes are properly tagged, but no terms are listed at, e.g., /admin/structure/taxonomy/manage/tags/overview.

This is pretty significant - I think we should consider it a Migrate-critical. Is the solution as simple as adding a default_value process plugin to set to 0 for the parent field migration in d6_taxonomy_term.yml/d7_taxonomy_term.yml? Anyway, first step should be a test that catches this scenario.

Couldn't help playing with this... I gave a quick try of setting the default_value to 0 in d6_taxonomy_term and it didn't work - because, of course, the MigrateSkipProcessException skipped it...

This begs the question, why is the migration process plugin throwing MigrateSkipProcessException anyway? If it would simply return NULL, we could use default_value with it. This feels like a familiar discussion, but I can't find an issue for it...

So it's not throwing because the lookup fails. Instead, it's calling $this->skipOnEmpty($value);. And then skipOnEmpty() doesn't distinguish between NULL/FALSE/zero. Why are we doing this anyhow? I'll check the blame.

It looks like it comes from #2411233: Stub in migration process plugin does not do complete process. I'm not sure how it has anything to do with that, though.

Here's a test that fails, to show what's going wrong.

Here's an attempted fix. I made the 'migration' process only skip if its input is NULL, not zero.

My only concern now is that I had to turn stubbing off, so that we don't attempt to make a term with tid = 0. This will be a problem, however, if any term is imported before its parent.

What we want to say is:

if parent is zero:
  return zero
else
  lookup, and stub if necessary

It's not clear to me that there's a way to do this with the current process plugins, so I may have to write a custom one.

Looks like it failed because comments are out of hierarchy order. Glad we at least test that :)

This begs the question, why is the migration process plugin throwing MigrateSkipProcessException anyway? If it would simply return NULL, we could use default_value with it. This feels like a familiar discussion, but I can't find an issue for it...

Mike, are you looking for an issue other than the one boromino referenced? #2692373: Value should be null when is produced skip process

I came across this issue because I have a Drupal 7 to Drupal 8 migration for which I am writing tests. The test is pretty simple. It just visits a "admin/structure/taxonomy/manage/[vocab_machine_name]/overview" and checks for a couple of terms that should be there. After updating to 8.1.2 yesterday, this test failed. (FWIW, the tables taxonomy_term_field_data and taxonomy_term_data were still getting filled with records) I found this problem right after publishing a blog post about how I was testing and rerunning migrations while building out a D8 site. I'll do a follow up post on how I found the problem issue by checking out individual commits between 8.1.1 and 8.1.2. Reverting the commit from #2692373: Value should be null when is produced skip process got my test to pass again.

I'm surprised #2692373: Value should be null when is produced skip process didn't cause a test failure. Is there a way in core to test the results of migrations from real(ish) D6/D7 databases?

I've added a new process plugin to deal with situations like this, called 'entity_parent'.

This patch (2744639-13) is giving me the following error for Taxonomy Terms when attempting a migrate from D7 to D8.1.x.

Missing bundle for entity type taxonomy_term (/var/www/html/drupal8/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:83)

Tax

@jeffwpetersen: Do you have any more details?

Also, I think it may be possible to do this with just process, not a custom plugin. I'll give it another shot.

I ran the migrate import against a D8 install with existing Taxonomy Terms. No error. but the terms overwrote the existing terms in the existing Taxonomy. And the taxonomy_term_hierarchy table was not updated.

A problem with stubs?

@jeffwpetersen: The upgrade path doesn't support having existing content. See [#2350603], under "Do not configure the Drupal 8 site". You can use custom migrations if you need to import into a site with existing content.

It was an experiment as you asked for more details.
This was with the 2744639-13.patch applied
A fresh D8 site with no Taxonomy Term throws an error.
A fresh D8 site with taxonomy terms added in the database overwrites the terms but still doesn't include the taxonomy_term_hierarchy.

Some issue with stubbing?

I've tried a migration from a simple D7 site to both 8.1.x and 8.2.x with the latest patch here, and it worked fine for me (using both the core UI and drush migrate-upgrade) - the taxonomy hierarchy was properly migrated without any errors.

@jeffwpeterson, anything unusual about your D7 source site? Could there be terms in your taxonomy_term_data that are pointing to non-existent vocabularies? Does that site migrate without the patch with no errors?

@vasi: Although it works for me, I'm not entirely thrilled with introducing a new narrowly-focused process plugin. Couldn't we accomplish this by adding a default_value key to the migration process plugin, and setting default_value: 0 on the taxonomy parent mapping?

@mikeyran: It's complicated, because we want to stub when given a positive parent, but not stub when given zero.

I think we'll need to do something like:

  parent_id:
    - plugin: skip_on_empty
      method: process
    - plugin: migration
      migration: d6_taxonomy_term
      source: parent
  parent:
    plugin: default_value
    default_value: 0
    source: @parent_id    

Does that look ok to you?

With D8.2.x the migration import ran fine with the patch applied.
The only error is in the taxonomy_term_hierarchy table, parent values that should be 0 have a value of 1.

Ok, here's a version using only existing process plugins. Please test.

I still think that it's an error for the migration process to throw when given zero as input, there's no reason zero couldn't be a legitimate source ID for some source. If that seems reasonable to others, I'll make that a follow-up.

I still think that it's an error for the migration process to throw when given zero as input, there's no reason zero couldn't be a legitimate source ID for some source. If that seems reasonable to others, I'll make that a follow-up.

Yep, I agree.

+++ b/core/modules/taxonomy/migration_templates/d6_taxonomy_term.yml
@@ -13,10 +13,16 @@ process:
+    - plugin: skip_on_empty

I haven't seen this syntax before, usually the hyphen is on its own line. Grepping around, I see d*_user_role.yml using it, but only where there's a singleton value (only the plugin key itself).

So, I'll call it a nit and not worth bumping back to "needs work", since it does not violate the (rather minimal) coding standards - ideally it would be consistent with common practice, though.

But, needs work because only D6 is fixed here...

Added the followup: #2751825: MigrationLookup plugin should accept zero as a legitimate input

Added D7. Fixed syntax nitpick.

Ummm... vasi, the patch seems to have grown just a little bit...

Oops! Let's try that again.

Manually tested D6 and D7 migrations with hierarchical terms on D8.2 - worked perfectly for both.

Submitted a retest of the 8.1.x branch - errors did not look in any way connected to migrate...

Re-rolling.

With 2744639-34.patch
All values are 0 in taxonomy_term_hierarchy table regardless of their initial value.

Bug reports should be assigned to 8.1.x, restoring.

@jeffwpetersen: Can you describe the precise steps you're performing to arrive at those results? I cannot reproduce your problem on either 8.1.x or 8.2.x, migrating from either D6 or D7. Here are my steps:

git checkout 8.1.x # or 8.2.x
git apply -v ~/tmp/2744639-34.patch
drush si -y standard --account-name=admin --account-pass=admin --db-url=mysql://user:pass@127.0.0.1/d8db
drush en -y migrate_upgrade
drush migrate-upgrade --legacy-db-url=mysql://user:pass@127.0.0.1/d6db # or d7db

In all four cases the vocabulary hierarchies are properly displayed at /admin/structure/taxonomy/manage/vocabulary_name/overview, and the taxonomy_term_hierarchy table is properly populated (with 0 for all root-level terms).

I can confirm that patch 2744639-34.patch does indeed work using your instruction.

git checkout 8.1.x # or 8.2.x
git apply -v ~/tmp/2744639-34.patch
drush si -y standard --account-name=admin --account-pass=admin --db-url=mysql://user:pass@127.0.0.1/d8db
drush en -y migrate_upgrade
drush migrate-upgrade --legacy-db-url=mysql://user:pass@127.0.0.1/d6db # or d7db

I was using drush migrate-upgrade --configure-only : drush mi --all with the migrate_tools module.
This combination does not work and all migrated parent values of taxonomy_term_hierarchy are 0.

@jeffwpetersen: Ah-ha! Looks like you're hitting #2709349: --configure-only should recurse while adding the upgrade_ prefix - the changes here move the migration process plugin for the parent one level deeper in the configuration, so --configure-only is failing to rewrite the plugin ID.

So, the core patch here is good, I think...

+1 for RTBC. I manually applied the config change to my pre-existing migration config and my taxonomy migration started working again: https://github.com/stevector/nerdologues-d8/pull/111

Since this is genuine data-loss that might be missed when reviewing a site after upgrade, bumping to independently critical.

Need to give the patch one more look-over before commit (or another committer could commit it, didn't see anything wrong just in the middle of something else).

Committed 47df378 and pushed to 8.1.x and 8.2.x. Thanks!

I putted the patch in the sites' root directory.

Did in Ubuntu-terminal:

patch < 345853 and got the surprising input "patch: **** Only garbage was found in the patch input".

I am quite new Drupal patching. Can you please explain what I should have done otherwise?

@Benia, hi. I use git for applying patches, like the snippet in comment #34 above. But the handbook has more detailed instructions for Applying patches. There is also the option of asking on #drupal on IRC. Chat with the Drupal Community on IRC has more details about that. Good luck.

I know this option but it didn't work for me with git either.

@Benia, 'fatal: unrecognized input' is telling you that the input file is not a valid patch file. But your question is really a support request and should be in separate issue.

Ah, I see you started #2764951: Patch fails to run and successfully applied that patch. So, this one should be similar. Just make sure you are using the correct patch file. Maybe download it again. And, I agree with what cilefen said in https://www.drupal.org/node/2764951#comment-11396861 that using source control is needed if you are patching files.

Please note I used the 8.1.x patch by vasi in #35 ...

Its the file but will use the previous or wait for 8.1.6 ...

#35 failed for me in both patch < filename / git apply filename

@Benia, please open a new issue for support or go to #drupal on IRC. They are plenty of folks there who can help. I'm changing the status back to fixed because this issue is about fixing the taxonomy migration in core and that has been committed to core. Your issue is a support request which is better handled in a separate issue.

I don't understand. If someone tries to ran a patch and it fails, shouldn't the ticket be changed? ..

Past year I used IRC but sadly very few people was there. Anyway, given the patch before #35 seem to have failed I might just wait to 8.1.6 .

The issue status reflects the status of the reported bug, which has been fixed. Trouble applying a fix locally does not change that.

Note that patches are meant to be applied with git, to a git checkout, and if you have an up-to-date git checkout of core then the fix is already there and the patch will not apply.

Just a small last question, please Mike. Would the change be part of Drupal 8.1.7 or later?

I need to know that to make better moves in regards to another Drupal project.

@benia, the patch here is committed to the 8.1 branch, so will be released with the next patch release. The one coming out today is a security release only, so should be included in 8.1.8, which will come out first Wednesday of august. (See https://www.drupal.org/core/release-cycle-overview for more details)

I'm not sure how much this is related but the second taxonomy problem I have wasn't solved with the 8.1.8 update.

Will most appreciate your take on this:

https://www.drupal.org/node/2778531

The problem I encounter since migrating from D7.44 to D 8.0.6 (now 8.1.8) is live and kicking. The migration experts might be able to help:

https://www.drupal.org/node/2783423

I upgraded to the 8.1.9, rolled back migrations and did a fresh import. I am still not seeing any terms on the UI, although they are in the database tables, taxonomy_term_hierarchy is not fully populated.

@Matt B this issue has been closed for some time. The bug you mention sounds like #2783423: Taxonomy terms not displayed after migration which is open, see you over there!

I'm getting this issue on 8.3.2:

After having done a migrate-upgrade --configure-only :
$ drush mi --group=migrate_drupal_6

[...]
Missing bundle for entity type taxonomy_term (/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:83) [error]
[...]

$ drush mmsg upgrade_d6_field_instance
944449d916bc3fdf14f08763a4431963d61e28ccff36cbafbb1409d27e989509 1 Missing bundle for entity type taxonomy_term (/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:83)

#2737501: migrate-import fails with field_instances, upgrade_d6_field_instance came back too

Having started to try to migrate from D6 again.... I have now retested using Drupal 8.3.2, migrate upgrade 8.x-3.0-rc1, migrate_plus 8.x-4.0-beta1 and migrate_tools 8.x-4.0-beta1 and I'm getting the same error as drzaf in #61

I have used the below line of code on my taxonomy term migration yml file and Term hierarchy migration working fine.

parent_id:
    -
      plugin: skip_on_empty
      method: process
      source: parent
    -
      plugin: migration_lookup
      migration: taxonomy_terms_product_classification_mymig
  parent:
    plugin: default_value
    default_value: 0
    source: '@parent_id'
  forum_container: is_container
  changed: timestamp

Thanks
Enireddy Polayya Reddy

'Parent' is coming as array from taxonomy plugin source, so you have to use 'get' plugin to process the array. This code section fixed for me.

parent:
plugin: get
source: parent