Create stub entities with proper default values

This is pretty important - we fixed this for comments, but need a more general fix. Even simple node stubbing fails with NULL titles... Can Entity::processStubRow() automatically identify all required properties/fields and populate them?

I started work on #2555127: Process Migration Always Ignores no_stub Due to Pre-Existing Cached Entity Migrations by trying to create a fail test, and realized we basically have no tests of stubbing at all (and also that pretty much any test I constructed would fail due to this current issue). So, I'm starting with tests of stubbing for migrated content entity types, which fail in different ways. I'm not entirely sure whether we can automatically populate all required base field properties, but I'll give it a try. Some entities will certainly still require particular work in their own processStubRow() methods.

I've got the basic solution, which really isn't bad at all. The biggest hole here is stubbing entity_reference fields, since of course we need a valid entity on the other end. Next stage is to see if we can generalize the comment migration approach of looking for an arbitrary entity of the appropriate type and point to it - if not, it may be up to the particular entities to handle it as comment does. And yes, what if there is no existing entity of the right type? No, I do not want to create a second level of stub... This seems like an appropriate place for a SkipRowException.

Note that user stubbing fails due to #2602066: User name max length differs between storage and field definition.

Tested the patch in #10 with a D7 migration that gives Integrity contstraint violations when migrating taxonomy terms.
The error still persists.

Quick debug showed that the $field_definition->isRequired() seems to return FALSE for the weight field on the taxonomy term.
Will debug in more depth.

Hmm seems Drupal\taxonomy\Entity\Term::baseFieldDefinitions() doesn't call ->setRequired(TRUE) for $fields['weight'].
Alternatively, the database structure could have a default value of 0 for this database field, but it does not.

Should we add a workaround for terms in this patch, or try to fix the problem in a related issue?

Discussed with svendecabooter in IRC - I've stepped through the taxonomy term stub test, and in Entity::getEntity()

$entity = $this->storage->create($row->getDestination());

Produces a term entity with the weight set to 0. Stepping into it I saw that being set in FieldItemList::applyDefaultValue(). He will try debugging his scenario tomorrow to see how that's not being set in his case.

It turns out entity_reference fields do implement generateSampleValue() by doing just what I wanted, randomly selecting a random referenceable entity. So, the question remains, what if there are no such entities (as is the case with the tests right now, which only create the stub in isolation? I do not think we should try to create a second layer of stubs - the stubs we're creating now are proxies for real source system data and (unless they're orphan references in the source) end up getting filled in with real entities, while this second layer of stubs would have no relationship to the source data, and would not be cleaned up by completion of the migration process, they would leave dummy data lying around. So, in this case, I think we should throw a SkipRowException. In most real-world cases, where the migrations are run in some sort of rational order, this should not happen, and the exception should let the migration developer know they need to rethink their dependencies.

The tests that involve required entity_reference fields should be modified to test first with no referenceable entities, verifying that the exception is received, and then to test after creating appropriate entities and confirm that the stubs are properly created.

Progress:

1. Now throwing an exception when we can't generate a value for a required field (in practice, this means entity references where there's no entity to reference.
2. Along with that, added tests of that exception for entities depending on a required entity reference, along with tests with the necessary entity present.
3. Implemented workarounds for core bugs #2602066: User name max length differs between storage and field definition and #2602662: Feed ID should be required base field for aggregator items.
4. Removed the comment destination handler's stuff for populating its node reference, now handled in the base processStubRow(). It still needs to override processStubRow() to set 'name' though.
5. The shortcut module automatically installs a default shortcut_set, having the test install that config to reflect the real world makes things work smoother.
6. Removed the taxonomy_term destination plugin, which was doing stub setup in getEntity() that a. belonged in processStubRow(), and b. is no longer necessary.

contact and file are the only ones remaining.

Duh, we don't migrate contact message entities, so removing that useless test.

Just files left, and there is actually a generate for them, we just need to have the migration plugin skip its attempt to copy a file in the stub case...

@mikeryan: Tested again with my D7 test site, Drupal 8 up to date with HEAD & patch in #18 applied.
Still the problem persists with the migration of a few simple hierarchical taxonomy terms.
The default weight of 0 doesn't get set after

$entity = $this->storage->create($row->getDestination());

Also, for the weight field FieldItemList::applyDefaultValue() never gets called when I put a breakpoint there.
No idea what the problem is, especially since it's a really simple D7 source.
The vocabulary is used for the Forum module BTW, though that shouldn't be an issue AFAIK.

More debugging:

\Drupal\Core\Entity\ContentEntityStorageBase::doCreate():

foreach ($entity as $name => $field) {
      if (isset($values[$name])) {
        $entity->$name = $values[$name];
      }
      elseif (!array_key_exists($name, $values)) {
        $entity->get($name)->applyDefaultValue();
      }
      unset($values[$name]);
    }

$values["weight"] = NULL, so the if and elseif get skipped.

@svendecabooter: OK, I now understand your issue. The Migration process plugin, when a stub is to be created, runs processRow() using the migration's process pipeline. It has the comment "Only keep the process necessary to produce the destination ID", which appears to be a lie - the whole process config is passed to processRow(). For your d7_taxonomy_term migration, this includes 'weight: weight'. Since there is no source data for weight, the destination property 'weight' is assigned NULL (in my tests there was no mapping for weight, so no destination property set for it). So, in the snippet you showed above, array_key_exists('weight', ['weight' => NULL, ...]) returns TRUE.

So many possible things we can do here...

1. For one thing, if 'weight' were marked as required in the Term base fields, the patch in this issue would have assigned a default value (it's using an is_null() instead of array_key_exists). But, I'm thinking what "required" means in a field definition is that a field value is required to be provided when saving an entity, and weight is not required here because the default value would (should) be applied when it's not there.
2. doCreate() could account for NULL values by adding || is_null($values[$name]).
3. We could implement what the comment "Only keep the process necessary to produce the destination ID" implies - remove all process entries other than those mapped to destination keys. Not sure of the implications of this, and it may actually be useful to let those that set, say, a default_value apply to stubs.
4. Change processRow() to not assign NULL values - leave fields with no value coming out of the process pipeline unset rather than assigning NULL. This feels familiar, like we've discussed this before...
5. Clean NULL values out in processStubRow().

Created #2603626: Apply default values when existing value is NULL to see if the doCreate() change will fly. In the meantime, in the context of this patch I'll decide which of the last two options to pursue.

OK, here's a fail patch with a test demonstrating the stub-term-parent issue when weight is mapped in the migration, and a patch fixing processRow() to not set NULL destination properties (and a previously-not-uploaded change to remove the pointless contact test). The "good" patch is still going to fail on the file entity stubbing, which is my next task.

Adding links to bugs we need to work around in this patch.

Another one - *sigh*.

OK, this adds a workaround to #2603726: Uri should be required base field for file entities. That alone is *so* close to working just fine - the one remaining problem after that is that File::preSave() unconditionally calls filesize() on the generated (fake) uri and triggers a "stat failed" warning. The stub entity does, though, get successfully created and validated, the warning is the only issue with that. This patch includes a (admittedly gross) work-around for that, so this should pass the bot and be ready for manual review. Alternative answers to the filesize problem welcome!

Well, thinking some more, maybe that solution isn't that gross - a stub file entity should have a stub file, right? And having it point to a real file is a sort of referential integrity...

The first test error is exposing yet another bug outside the direct scope of this issue: #2603798: dblog_settings tests prove nothing.

The second is because the upload test results in stubbing, which in this case is not working with our actual population of the stub, looking into that...

OK, two fixes here for the two test errors:

1. On dblog_settings, it was triggered by #2603798: dblog_settings tests prove nothing but that did point out a little bug, in that now that we're not setting NULL properties in processRow(), $row->rawDestination might never get set - it needed to be initialized to an empty array.
2. The upload test doesn't run the d6_file migration, so the filefield in the story node migration needs to stub the fid it's pointing to. Before reaching the actual stubbing code the file_uri process plugin is applied, receiving (because this is a stub with no real source data) an array of NULL values which for reasons produces a uri of 'private://'. By returning NULL for the uri in the stub case, we let the stubbing code do its job.

I think this is finally the one that passes...

Reviewed the patch quickly myself and made one more tweak - EntityFile::processStubRow() started as purely a workaround for #2603726: Uri should be required base field for file entities, but then I added the file creation bit that will be necessary even if/when that one is fixed. Indeed, if that issue got committed, it would break this code by skipping the file creation, so I rearranged it so it will work whether or not #2603726: Uri should be required base field for file entities is fixed.

1. +++ b/core/modules/aggregator/src/Plugin/migrate/destination/AggregatorItem.php
   @@ -0,0 +1,42 @@
   +  protected function processStubRow(Row $row) {
   +    parent::processStubRow($row);
   +    // Work-around for https://www.drupal.org/node/2602662 - this entire class
   +    // can be removed when that's committed.
   

   Ideally this patch will be blocked on #2602662: Feed ID should be required base field for aggregator items so this extra class will not be necessary, but the other patches may not be RC eligible. Will need to run that past core committers.

2. +++ b/core/modules/aggregator/src/Plugin/migrate/destination/AggregatorItem.php
   @@ -0,0 +1,42 @@
   +        throw new MigrateException($this->t('Stubbing failed, unable to generate value for field @name',
   +          ['@name' => 'fid']));
   

   Exceptions are not supposed to be translated or formatted -- this should be a totally static string. (Imagine trying to debug an exception in Russian or Thai or Klingon?)

3. +++ b/core/modules/aggregator/src/Tests/Migrate/MigrateAggregatorStubTest.php
   @@ -0,0 +1,60 @@
   +      // We expect an exception, because there's no feed to reference.
   +      $this->performStubTest('aggregator_item');
   +      $this->fail(t('Expected exception has not been thrown.'));
   

   Like with exceptions, I don't think there's any need to translate test messages.

4. +++ b/core/modules/comment/src/Tests/Migrate/MigrateCommentStubTest.php
   @@ -0,0 +1,53 @@
   +      // We expect an exception, because there's no node to reference.
   +      $this->performStubTest('comment');
   +      $this->fail(t('Expected exception has not been thrown.'));
   

   The fail message should not be translated.

5. +++ b/core/modules/comment/src/Tests/Migrate/d6/MigrateCommentTest.php
   @@ -22,13 +22,7 @@ class MigrateCommentTest extends MigrateDrupal6TestBase {
   -  public static $modules = [
   -    'comment',
   -    // Directly testing that a stub comment's entity_id is populated upon
   -    // importing is not straightforward, but RDF module serves as an implicit
   -    // test - its hook_comment_storage_load() references a stubbed comment.
   -    'rdf',
   -  ];
   +  public static $modules = ['comment'];
   

   So if I understand correctly, we're killing the dependency on RDF here because MigrateCommentStubTest specifically tests the condition which would have caused the RDF fatal?

6. +++ b/core/modules/file/src/Plugin/migrate/destination/EntityFile.php
   @@ -256,4 +269,29 @@ protected function urlencode($filename) {
   +    if (!$row->getDestinationProperty('uri')) {
   +      $field_definitions = \Drupal::entityManager()
   +        ->getFieldDefinitions($this->storage->getEntityTypeId(),
   +                              $this->getKey('bundle'));
   

   Ideally, the entity manager (or one of its constituent services) should be injected, but that should be done for all entity destinations and is a huge change deserving of its own follow-up issue. Maybe just add a TODO to inject the service?

7. +++ b/core/modules/file/src/Plugin/migrate/destination/EntityFile.php
   @@ -256,4 +269,29 @@ protected function urlencode($filename) {
   +      if (is_null($value)) {
   

   I wonder if is_null() is what we want -- what if a generateSampleValue() call returns FALSE or something? empty() might be a better choice here.

8. +++ b/core/modules/file/src/Plugin/migrate/destination/EntityFile.php
   @@ -256,4 +269,29 @@ protected function urlencode($filename) {
   +        throw new MigrateException($this->t('Stubbing failed, unable to generate value for field @name',
   +          ['@name' => 'uri']));
   

   Exception should not be translated, and uri does not need to be a variable.

9. +++ b/core/modules/file/src/Plugin/migrate/destination/EntityFile.php
   @@ -256,4 +269,29 @@ protected function urlencode($filename) {
   +      // Create a real file, so File::preSave() can do filesize() on it.
   +      touch($value);
   

   If I'm not mistaken, the file_system service provides a touch() method, so we should probably use that instead.

10. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
    @@ -147,6 +150,40 @@ protected function processStubRow(Row $row) {
    +    $fields = \Drupal::entityManager()
    +      ->getFieldDefinitions($this->storage->getEntityTypeId(), $bundle_key);
    

    Again, a TODO about injecting the entity manager would be good here.

11. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
    @@ -147,6 +150,40 @@ protected function processStubRow(Row $row) {
    +          $field_type_class = \Drupal::service('plugin.manager.field.field_type')
    +            ->getPluginClass($field_definition->getType());
    

    Ditto.

12. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
    @@ -147,6 +150,40 @@ protected function processStubRow(Row $row) {
    +                throw new MigrateException($this->t('Stubbing failed, unable to generate value for field @name',
    +                  ['@name' => $field_name]));
    

    Exceptions: Don't translate 'em :)

13. +++ b/core/modules/migrate_drupal/src/Tests/MigrateDrupalTestBase.php
    @@ -73,4 +76,47 @@ protected function installMigrations($version) {
    +  /**
    +   * Test that creating a stub of the given entity type results in a valid
    +   * entity.
    +   *
    +   * @param string $entity_type_id
    +   *   The entity type we are stubbing.
    +   */
    +  protected function performStubTest($entity_type_id) {
    

    I wonder if this and createStub() should not be in a trait, since most functional migration tests don't care about how or if stubbing is done.

14. +++ b/core/modules/migrate_drupal/src/Tests/MigrateDrupalTestBase.php
    @@ -73,4 +76,47 @@ protected function installMigrations($version) {
    +    if ($stub_entity) {
    +      $this->assertTrue($stub_entity->validate(), 'Stub is a valid entity');
    +    }
    

    IIRC, validate() does not return boolean, it returns a list of constraint violations. This should probably assert that violation count is 0.

15. +++ b/core/modules/taxonomy/src/Tests/Migrate/MigrateTaxonomyTermStubTest.php
    @@ -0,0 +1,110 @@
    +    if ($stub_entity) {
    +      $this->assertTrue($stub_entity->validate(), 'Stub is a valid entity');
    +    }
    

    Ditto here with asserting a truthy ConstraintViolationListInterface.

Ideally this patch will be blocked on #2602662: Feed ID should be required base field for aggregator items so this extra class will not be necessary, but the other patches may not be RC eligible. Will need to run that past core committers.

We'll try to get that one in, but this stub patch really needs in for 8.0.0 (and ideally rc3 if there is one) - we should set a deadline and if that patch isn't in by then, pursue this patch with the work-around.

You didn't explicitly mention it, but we also are ideally blocked on #2602066: User name max length differs between storage and field definition. That one is pretty ugly and I'm not optimistic about fixing it for 8.0.0 unless someone with deeper knowledge of field definition internals takes an interest in it, so I'm inclined on including the work-around in the stub patch.

Exceptions are not supposed to be translated or formatted -- this should be a totally static string.

This and the various other unnecessary translations removed.

So if I understand correctly, we're killing the dependency on RDF here because MigrateCommentStubTest specifically tests the condition which would have caused the RDF fatal?

Yes, the RDF thing was a horrible indirect bass-ackwards way to "test" the stubbing failure, now we have clean explicit tests.

Ideally, the entity manager (or one of its constituent services) should be injected, but that should be done for all entity destinations and is a huge change deserving of its own follow-up issue. Maybe just add a TODO to inject the service?

Done - see #2604566: Inject entity manager and field type manager to Entity destinations.

I wonder if is_null() is what we want -- what if a generateSampleValue() call returns FALSE or something? empty() might be a better choice here.

The default implementation returns NULL (well, the default implementation is {}) - the interface specifies it should return an array, so changing to empty().

If I'm not mistaken, the file_system service provides a touch() method, so we should probably use that instead.

Nope, no touching in FileSystem.

I wonder if this and createStub() should not be in a trait, since most functional migration tests don't care about how or if stubbing is done.

Trait created.

IIRC, validate() does not return boolean, it returns a list of constraint violations. This should probably assert that violation count is 0.

Done for this and the other instance. Oops, but there are constraint violations being returned, damn...

So, thanks (semi-sarcastically;) for pointing out the misapplication of validate() - it turns out a bunch of stuff is actually failing validation. I think this is not a fault of the stubbing code itself, but of the tests - the one I've looked at in detail, block_content, is failing because there are no existing block_content_type config entities for its 'type' entity_reference field to be stubbed to point to - so, in this case, we need to test for the failure in the absence of the block_content_type, and success with it. I need to go through entity type by entity type and track this stuff down...

This patch has unveiled yet another core bug (yay?)! #2605150: UrlItem::generateSampleValue() does not generate a valid uri

I'll submit a patch for that issue, and for the time being incorporate a work-around into this patch.

Added a list of core bugs we're working around to the issue summary.

And... another core bug discovered and to be worked around: #2605254: LinkItem::generateSamplevalue() does not respect link_type setting

OK, I think this should pass the tests... To recap, phenaproxima pointed out that I was misusing validate() in the tests, and it turns out I was missing validation errors all over the place. Good news - none of them was due to inherent functional flaws in the stubbing support, the approach we're taking in Entity::processStubRow() seems solid. So-so news - some of the validation errors showed tests to be incomplete (mainly needing some more setup for all the pieces to be in place to create valid stubs). Bad news - yet more core bugs found, which we have to work around here until they're fixed.

Two test errors - one was from a test for a related issue I was working on that doesn't belong here (especially without the associated fix!). The other is an error in FileStubTest which I can't reproduce locally, I can't figure out specifically what the issue is but imagine the uri field is the most likely culprit. I've got an improvement to the uri code here, which should make absolutely no difference to the test, but let's run it through anyway...

#2536374: Generate placeholder content for ListItemBase Field types has been committed, so removing the work-around for that one.

So, we added those todos to inject the entity manager? The thing is, it's already injected - in EntityContentBase. The processStubRow() implementation was added to the Entity class, which doesn't have the entity manager injected - but stubbing is only supported by content entities. So, I've moved processStubRow() to EntityContentBase where it can use the injected entity manager. This just left the issue of injecting the field type manager, and I decided to go ahead and do it as part of this patch.

Related: the entity manager we're using all over the place is now deprecated in favor of a bunch of individual little managers. It's out of scope to replace it in this issue, but I'll open a follow-up. What's the policy on replacing the deprecated call - target for 8.0.0, or should it be done later?

For anyone watching this issue with bated breath - you can help by reviewing #2605150: UrlItem::generateSampleValue() does not generate a valid uri and/or #2605254: LinkItem::generateSamplevalue() does not respect link_type setting, once those get committed we can remove the work-arounds from this patch - I'll then be comfortable trying to get this patch in with only the user work-around.

Weird bot glitch? Trying again...

EntityManager removal issue: #2607688: Replace deprecated entity manager with specific services. Doing that analysis, it seems like the only current usage of the entity manager is in the processStubRow() implementations, using getFieldDefinitions(), so it just might be possible to replace EntityManager with EntityFieldManager in this patch. Or is that overreaching?

Found a test that needed to inject the field type manager.

I've run into this issue as well, and the solution looks really good to me!

All except one nit-picks below are just the fact that I haven't seen the line break or indentation patterns elsewhere in core. If they do in fact align with core's coding standards, please ignore those points.

1. +++ b/core/modules/aggregator/src/Tests/Migrate/MigrateAggregatorStubTest.php
   @@ -0,0 +1,63 @@
   +    catch (MigrateException $e) {
   +      $this->assertIdentical($e->getMessage(),
   +        'Stubbing failed, unable to generate value for field fid');
   +    }
   

   Nit-pick: Indentation patterns not common in core.

2. +++ b/core/modules/comment/src/Tests/Migrate/MigrateCommentStubTest.php
   @@ -0,0 +1,78 @@
   +    catch (MigrateException $e) {
   +      $this->assertIdentical($e->getMessage(),
   +        'Stubbing failed, unable to generate value for field entity_id');
   +    }
   

   Nit-pick: Indentation patterns not common in core.

3. +++ b/core/modules/file/src/Plugin/migrate/destination/EntityFile.php
   @@ -256,4 +271,29 @@ protected function urlencode($filename) {
   +      $field_definitions = $this->entityManager
   +        ->getFieldDefinitions($this->storage->getEntityTypeId(),
   +                              $this->getKey('bundle'));
   

   Nit-pick: Indentation patterns not common in core.

4. +++ b/core/modules/user/src/Plugin/migrate/destination/EntityUser.php
   @@ -98,4 +104,28 @@ public function import(Row $row, array $old_destination_id_values = array()) {
   +    $field_definitions = $this->entityManager
   +      ->getFieldDefinitions($this->storage->getEntityTypeId(),
   +                            $this->getKey('bundle'));
   

   Nit-pick: Indentation patterns not common in core.

5. +++ b/core/modules/user/src/Plugin/migrate/destination/EntityUser.php
   @@ -98,4 +104,28 @@ public function import(Row $row, array $old_destination_id_values = array()) {
   +    // %todo Work-around for https://www.drupal.org/node/2602066.
   

   Should be // @todo

I do try to follow https://www.drupal.org/coding-standards#linelength. That being said, I've had trouble getting PhpStorm to not align arguments like

+        ->getFieldDefinitions($this->storage->getEntityTypeId(),
+                              $this->getKey('bundle'));

so correcting those.

Should be // @todo

Done.

Looks good to me.

Removed the work-arounds for #2605150: UrlItem::generateSampleValue() does not generate a valid uri and #2605254: LinkItem::generateSamplevalue() does not respect link_type setting - once it passes the bot, this is ready to go as far as I'm concerned.

OK, a core committer's guide to what's happening in this patch:

1. +++ b/core/modules/aggregator/src/Tests/Migrate/MigrateAggregatorStubTest.php
   @@ -0,0 +1,63 @@
   + * Contains \Drupal\aggregator\Tests\Migrate\MigrateAggregatorStubTest.
   

   Volume wise, the biggest thing added is tests to demonstrate that stubbing works for each entity type supported by migration.

2. +++ b/core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
   @@ -110,32 +114,9 @@ public function import(Row $row, array $old_destination_id_values = array()) {
   -    $stub_commented_entity_type = $row->getDestinationProperty('entity_type');
   -
   -    // While parent::getEntity() fills the bundle property for stub entities
   -    // if it's still empty, here we must also make sure entity_id/entity_type
   -    // are filled (so $comment->getCommentedEntity() always returns a value).
   -    if (empty($this->stubCommentedEntityIds[$stub_commented_entity_type])) {
   -      // Fill stub entity id. Any id will do, as long as it exists.
   -      $entity_type = $this->entityManager->getDefinition($stub_commented_entity_type);
   ...
   

   Before this patch, we hacked in specific stubbing support for comments (in particular for the parent entity reference) - we're now able to remove that stuff because the general processStubRow() does it.

3. +++ b/core/modules/file/src/Plugin/migrate/destination/EntityFile.php
   @@ -77,6 +80,12 @@ public static function create(ContainerInterface $container, array $configuratio
   +    // For stub rows, there is no real file to deal with, let the stubbing
   +    // process take its default path.
   +    if ($row->isStub()) {
   +      return parent::getEntity($row, $old_destination_id_values);
   +    }
   

   File entities are special snowflakes when doing real migrations, but less so for stubs, so we short-circuit the special processing here and a couple places below for stubs.

4. +++ b/core/modules/file/src/Plugin/migrate/destination/EntityFile.php
   @@ -256,4 +271,29 @@ protected function urlencode($filename) {
   +  protected function processStubRow(Row $row) {
   ...
   +    if (!$row->getDestinationProperty('uri')) {
   

   The one snowflakeness needed for stubs is the need to make sure the stub entity has a real file to point to, so we override processStubRow() for that.

5. +++ b/core/modules/migrate/src/MigrateExecutable.php
   @@ -401,8 +401,8 @@ public function processRow(Row $row, array $process = NULL, $value = NULL) {
   -      // No plugins means do not set.
   -      if ($plugins) {
   +      // No plugins or no value means do not set.
   +      if ($plugins && !is_null($value)) {
   

   In most cases of real migration, setting a destination property NULL effective behaves the same as not setting it. However, so default values work properly in stubbed entities, we need to leave the destination property unset if there's no real value.

6. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
   @@ -137,19 +137,6 @@ protected function getEntityId(Row $row) {
   -  protected function processStubRow(Row $row) {
   

   Only content entities support stubbing, so moved the stubbing code to EntityContentBase.

7. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
   @@ -132,4 +148,44 @@ protected function updateEntity(EntityInterface $entity, Row $row) {
   +  protected function processStubRow(Row $row) {
   +    $bundle_key = $this->getKey('bundle');
   +    if ($bundle_key && empty($row->getDestinationProperty($bundle_key))) {
   +      $row->setDestinationProperty($bundle_key, reset($this->bundles));
   +    }
   ...
   

   This is the heart of the patch - making sure that every required field on an entity being stubbed is populated with a valid value.

8. +++ b/core/modules/migrate/src/Row.php
   @@ -73,7 +73,7 @@ class Row {
   -  protected $rawDestination;
   +  protected $rawDestination = [];
   

   Leaving empty destination properties unset revealed we needed an empty array here, in case all properties are left unset (and require stubbing), else foreach is unhappy.

9. +++ /dev/null
   @@ -1,30 +0,0 @@
   - * @file
   - * Contains \Drupal\taxonomy\Plugin\migrate\destination\EntityTaxonomyTerm.
   

   This class only existed to provide a stub name for terms - no longer necessary with the general stubbing support.

10. +++ b/core/modules/user/src/Plugin/migrate/destination/EntityUser.php
    @@ -98,4 +104,28 @@ public function import(Row $row, array $old_destination_id_values = array()) {
    +    // @todo Work-around for https://www.drupal.org/node/2602066.
    +    $name = $row->getDestinationProperty('name');
    ...
    

    Unfortunately, fixing the root issue here (internal discrepancies in the length of the username field) is a quagmire, so we work around it. Without this, the stubbing code will generate a username too long to fit in the real schema length (60).

Interdiff looks good. Back to RTBC.

+1,000 RTBC. Migrate needs this, bad. Kudos to @mikeryan for a glorious and stellar patch.

1. +++ b/core/modules/migrate_drupal/src/Tests/StubTestTrait.php
   @@ -0,0 +1,80 @@
   +      if (!$this->assertIdentical(count($violations), 0, 'Stub is a valid entity')) {
   

   I didn't know assertIdentical() returned a boolean. Seems to me like it should just loop over $violations without asserting its count -- if there are any violations, the test will not pass :) But that's more of a style thing, really.

2. +++ b/core/modules/migrate_drupal/src/Tests/StubTestTrait.php
   @@ -0,0 +1,80 @@
   +          $this->fail((string)$violation->getMessage());
   

   Nit: There should be a space after (string), but that's fixable on commit.

Everything in here touches Migrate only, which if I'm not mistaken makes it RC eligible. Let's do it.

Committed and pushed to 8.0.x. Thanks!