Destination bundle set in destination plugin, not in process

Added some notes on issues this is blocking. There are some fairly significant core and contrib tasks that are blocked so assigning the critical tag.

Would it make sense to make 'bundle' another attribute of the destination? I'm not sure we really want to further overload the name of the destination.

Thanks @Charlotte17 and @heddn! Discussed with @alexpott, @mikeryan, et al at the Migrate triage sprint and we agreed to keep this as a Migrate critical for now since itis blocking so much other work for the API. It may not be a critical bug for the core migrations per se but it is definitely worth keeping the visibility.

Working on this now. In #5 vasi suggests an alternative, so let's compare.

destination:
  plugin: entity:node:article

vs.

destination:
  plugin: entity:node
  bundle: article

I have a mild preference for the former, I think vasi's preference for the latter is also mild - any other opinions?

Having weird things happen in apparently-unrelated areas when trying to test manually, let's see what the testbot does with this.

Looking more carefully, I was missing the role of the MigrateEntity derivative - vasi's idea is looking better to me now, we shouldn't mess with the destination plugin ID...

I think there's some other broken stuff getting in the way of my manual testing, but here's the basic patch. Thinking it might be nice if the derivers would configure the destination plugins with the bundles...

Well, this should be a bit better...

That is being tested - most of the migrations in core are still setting the bundle in the process pipeline (e.g. type: type).

When/where/how would the destination bundle be set in the source?

OK, time to add explicit tests. If I do have the derivers make use of this, maybe we should make explicit a test for the type:type pattern...

+++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
@@ -6,6 +6,7 @@
+use Drupal\migrate\MigrateException;

@@ -82,6 +83,8 @@ public static function create(ContainerInterface $container, array $configuratio
+   * @throws \Drupal\migrate\MigrateException

Leftover from my attempt to implement plugin: entity:type:bundle (throwing an exception if malformed), I will remove.

Actually, first, let's see what (if anything) breaks if the node derivers set the bundle in the destination...

This looks good so far! I don't think you missed any relevant content types.

+++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
@@ -112,6 +122,10 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
+      if ($bundle = $this->getEntityBundleId()) {
+        $row->setDestinationProperty($this->getKey('bundle'), $bundle);
+      }
+

This will override anything set by process. We should decide if that's the behaviour we want, instead of eg: throwing if they conflict. We should also document this behaviour.

What tests do you envision needing? Maybe:

• Unit: can set bundle as a destination configuration
• Unit: can still set bundle via process plugins
• Unit: behviour if process/config values conflict
• Integration tests for the above should be already covered by the existing migrate tests
• Since stubbing is one of the use cases, maybe ensure that's covered?
• Are there any existing tests that need to be updated?
• Anything else?

@heddn, there's only one migration for terms from all vocabularies. The vid has to come from the process in that case.

Time to work on tests...

This will override anything set by process. We should decide if that's the behaviour we want, instead of eg: throwing if they conflict. We should also document this behaviour.

Perhaps process should override, since it's more specific (i.e., at the granularity of the row), and the destination bundle should function as a default bundle. This could accommodate a scenario where you want to divert just a few special cases to a different bundle. I'll do this in the next patch.

Unit: can set bundle as a destination configuration
Unit: can still set bundle via process plugins
Unit: behaviour if process/config values conflict
Integration tests for the above should be already covered by the existing migrate tests

Ayup.

Since stubbing is one of the use cases, maybe ensure that's covered?

Can you explain the stubbing use case? This doesn't really seem to affect processStubRow(), which just uses the bundle destination property directly without referencing process or destination configuration.

Are there any existing tests that need to be updated?
Anything else?

Here we go...

Ummm, the actual patch is helpful...

1. +++ b/core/modules/block_content/migration_templates/d7_custom_block.yml
   @@ -17,6 +14,7 @@ process:
   +  bundle: basic
   
   +++ b/core/modules/menu_link_content/migration_templates/menu_links.yml
   @@ -49,6 +46,7 @@ process:
   +  bundle: menu_link_content
   

   I think we should call this default_bundle, which is more honest since you can technically have migrations that import multiple bundles.

2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
   @@ -89,6 +89,16 @@ protected static function getEntityTypeId($plugin_id) {
   +  protected function getEntityBundleId() {
   +    return isset($this->configuration['bundle']) ? $this->configuration['bundle'] : '';
   
   @@ -112,6 +122,12 @@ protected function getEntity(Row $row, array $old_destination_id_values) {
   +      $bundle_key = $this->getKey('bundle');
   +      // Apply any configured bundle if the bundle has not already been set.
   +      if (($bundle = $this->getEntityBundleId()) && empty($row->getDestinationProperty($bundle_key))) {
   +        $row->setDestinationProperty($bundle_key, $bundle);
   +      }
   

   Lets make this getBundle(Row $row) and just let the getBundle() method figure out which to use.

Rest looks good.

You can have migrations that import entities of many different bundles in the one migration, in that case, the bundle is derived per row. Unless we remove that functionality, what we have here is the best outcome.

IMO, the UI could have just shown all fields for the entity type on all bundles and left it to the user to map the correct fields, that's how Panels works in D7 when placing fields and it seems fine.

Part of my motivation of seeing this get in is the ability to determine in a standard manner the destination bundle during source processing.

Having the source know anything about the destination feels very wrong to me. We're trying (for example in #2695297: Refactor EntityFile and use process plugins instead) to better decouple the migration stages and keep them focused on their specific duties, and the source's responsibility is simply to deliver the raw source data in a canonical form - it should not need to know anything about where the data is going.

Can you describe the use case? How would you use the destination bundle information at source time?

IMO, the UI could have just shown all fields for the entity type on all bundles and left it to the user to map the correct fields, that's how Panels works in D7 when placing fields and it seems fine.

With the D7 migration tools, it is really very helpful to see the specific fields that are relevant to the migration (which is always bundle-specific in that case). Migrating multiple bundles in a single migration is an edge case, and as a best practice I always discourage it - we should optimize for the common case, which is one migration per bundle.

Addressing benjy's feedback.

@heddn: The implementation of Entity::fields() for destinations will be able to use the destination default_bundle just fine.

As for the lookup/generate process plugins (I see now you were talking about process plugins, not source plugins) - the idea is that they need the field instance, not just the field storage, for some reason? What precisely do they need from the field instance?

Should this be up on MigrateDestinationInterface as well?

I think it's too entity specific to go on the generic destination interface? I've made the method public, we probably need a new interface.

Additionally, process plugins technically run during the source phase. Just very late in the flow. We know this because after process runs the source is frozen.

This isn't a great way to think about, sure process plugins are applied one row at a time to the source data but the two things are decoupled. Otherwise, we'd be back into D7 days where all processing happens in prepareRow().

Attached a new patch, I actually meant to have a method to calculate the bundle but still do the set onto the row in getEntity().

This looks great to me.

Retested, cause i can't see any failures.

What about existing migrations in contrib and the wild? Will any of the changes here break them? Looking at https://www.drupal.org/core/experimental we're still in alpha so nothing to worry about here.

Committed 3e5ef21 and pushed to 8.1.x and 8.2.x. Thanks!

We still support setting the bundle in process so they should be fine.

Additionally, process plugins technically run during the source phase. Just very late in the flow. We know this because after process runs the source is frozen.

The source freezing happens before the process plugins run. This was a very important design decision, if it would be otherwise there would be no reason to freeze.

The only way to change the source is in prepare row.