Copy migrate source plugin from migrate_drupal_d8 into migrate_drupal

This should add support for translations.

Needs tests for translations, so back to work.

This should fix the failing test.

Also, here's a start for testing translation support. The addtranslation() doesn't seems to have the desired effect. The node_field_data table only ends up with one row (the translated one) instead of two (the source and the translation).

Just two small things

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,316 @@
   + * Drupal contenty entity source from database.
   

   s/contenty/content/

2. +++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/d8/ContentEntityTestBase.php
   @@ -0,0 +1,189 @@
   +    $this->installConfig($this->modules);
   

   $modules?

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,316 @@
   +    if (empty($configuration['entity_type'])) {
   +      throw new InvalidPluginDefinitionException($plugin_id, 'Missing required entity_type definition');
   +    }
   

   The entity type should not be configuration. I think this should be a derived plugin, because that would also allow us to set the source_provider more accurately.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,316 @@
   +    $fieldConfig = $this->select('config', 'c')
   +      ->fields('c')
   +      ->condition('name', 'field.field.' . $entity_type . '.%', 'LIKE')
   +      ->execute()
   +      ->fetchAllAssoc('name');
   

   I *really* do not think we should be directly reading the database tables. This module exists in Drupal 8, running on Drupal 8's API. Why not just take advantage of Drupal 8's APIs??

   And policy dictates that API is going to stay stable for the foreseeable future, with plenty of time and notice to deal with deprecations. It seems pretty safe to rely on.

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,316 @@
   +    $table = $this->getDedicatedDataTableName($entity_type, $field_name);
   +
   +    $query = $this->select($table, 't')
   +      ->fields('t')
   +      ->condition('entity_id', $entity_id)
   +      ->condition('deleted', 0);
   

   Same here. I am not a fan of this. For example -- this approach offers no support for dealing with computed fields. Just take advantage of the APIs!

Thanks for taking advantage of the entity API! This is pretty straightforward now; it's just refinement from here.

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,217 @@
   +    if (empty($plugin_definition['entity_type'])) {
   +      throw new InvalidPluginDefinitionException($plugin_id, 'Missing required entity_type definition');
   +    }
   

   This can go away; the derivative ID is the entity type ID. Once parent::__construct() is called, $entity_type == $this->getDerivativeId().

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,217 @@
   +    $this->entityType = $plugin_definition['entity_type'];
   

   Call the parent constructor first, then this can be $this->getDerivativeId().

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,217 @@
   +    if (!empty($this->configuration['bundle'])) {
   +      if (!$this->bundleKey) {
   +        throw new InvalidPluginDefinitionException('A bundle was provided but entity is not bundleable.');
   +      }
   +    }
   

   I think we can remove this. It seems overzealous to me.

4. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,217 @@
   +   * @return array
   +   *   An array of the data for this source.
   +   */
   +  protected function initializeIterator() {
   +    $ids = $this->query()->execute();
   +    return $this->yield($ids);
   

   This returns a generator (\Generator), not an array, so we should adjust the doc comment as such.

5. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,217 @@
   +   *   The entity ids.
   

   IDs

6. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,217 @@
   +      $entity = $this->entityTypeManager->getStorage($this->entityType)->load($id);
   

   Micro-optimization: We should call getStorage() before the foreach, so we don't have to keep doing it.

7. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,217 @@
   +      foreach ($entity->toArray() as $field => $values) {
   

   We should be very careful with toArray(). It returns entity data in a specific style, where every item is an array of values that would each need to be processed with something like the sub_process plugin. Not sure that will be ideal for most process pipelines.

8. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,217 @@
   +  /**
   +   * Query to retrieve entities.
   +   *
   +   * @return \Drupal\Core\Entity\Query\QueryInterface
   +   *
   +   * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException
   +   * @throws \Drupal\migrate\MigrateException
   +   */
   +  public function query() {
   

   @return and @throws need descriptions.

9. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,217 @@
   +  protected function doCount() {
   +    $this->query()->count()->execute();
   +  }
   

   Should this return something?

10. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
    @@ -0,0 +1,217 @@
    +    $fieldDefinitions = $this->entityFieldManager->getBaseFieldDefinitions($this->entityType);
    

    Nit: $fieldDefinitions should be $field_definitions.

11. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
    @@ -0,0 +1,217 @@
    +        $fields[$fieldName] = (string) $definition->getLabel();
    

    $fieldName should be $field_name. Same thing everywhere else in this plugin.

12. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntityDeriver.php
    @@ -0,0 +1,66 @@
    +  /**
    +   * The base plugin ID this derivative is for.
    +   *
    +   * @var string
    +   */
    +  protected $basePluginId;
    

    I'm not seeing where this is actually used?

13. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntityDeriver.php
    @@ -0,0 +1,66 @@
    +    foreach ($this->entityTypeManager->getDefinitions() as $definition) {
    

    Nit: getDefinitions() is keyed by entity type ID, so we could do $id => $definition and lose the repeated calls to $definition->id().

14. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntityDeriver.php
    @@ -0,0 +1,66 @@
    +        $this->derivatives[$definition->id()]['entity_type'] = $definition->id();
    

    This is redundant and can be removed. The plugin's derivative ID is already the entity type ID.

This worked for me. Action needs system module, book needs user module., setup() method isn't needed.

There's mode change on run-test.sh and it's accidentally included in the patch since #20.

@maxocub, thanks. I was just working too late.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
@@ -0,0 +1,230 @@
+ *   source_provider = "migrate_drupal",

I think this should be 'source_module', right? When trying to use this plugin I have an exception thrown that it is missing the 'source_module' property.

How come #27 wasn't picked up by the tests?

1. +++ b/core/modules/action/tests/src/Kernel/Plugin/migrate/source/ActionTest.php
   @@ -15,7 +15,7 @@ class ActionTest extends MigrateSqlSourceTestBase {
   -  public static $modules = ['action', 'migrate_drupal'];
   +  public static $modules = ['action', 'migrate_drupal', 'system'];
   
   +++ b/core/modules/book/tests/src/Kernel/Plugin/migrate/source/d6/BookTest.php
   @@ -13,7 +13,7 @@ class BookTest extends MigrateSqlSourceTestBase {
   -  public static $modules = ['book', 'migrate_drupal'];
   +  public static $modules = ['user', 'node', 'book', 'migrate_drupal'];
   

   Originally I thought we probably ought to mention the possibility of needing to install additional modules in tests as a result of this change in the change record, but I've had a think and this probably is only try for core sources where the modules that provide the entities are somewhere else. Ie. the action entity is provided by system.

2. +++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/d8/ContentEntityTest.php
   @@ -0,0 +1,375 @@
   +/**
   + * Tests the entity content source plugin.
   + *
   + * @group migrate_drupal_8
   + */
   +class ContentEntityTest extends MigrateTestBase {
   

   It's great to see @group migrate_drupal_8 - these sources are important to Drupal's major version release plans. And obviously is it good we have test coverage of the sources here but it would also be great to see a full migration test like we have for Drupal 6 and 7. Basically a Drupal 8 equivalent of \Drupal\Tests\migrate_drupal_ui\Functional\d7\MigrateUpgrade7Test. Not in this issue but as a follow-up.

   One thing that surprised me is that we have one gigantic test and not many new tests all extending \Drupal\Tests\migrate\Kernel\MigrateSqlSourceTestBase. I guess that's because the code can be generic. This leads to the question about whether the approach taken here is correct. Ie. using D8 APIs in derivative discovery. Not sure.

Getting there...

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,230 @@
   +  /**
   +   * The entity type.
   +   *
   +   * @var string
   +   */
   +  protected $entityType;
   

   This shouldn't be a string. Why not simply let this be the result of $this->entityTypeManager->getDefinition(), and then if we need the entity type ID, we can call $this->entityType->id()?

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,230 @@
   +  /**
   +   * The entity key for a bundle or FALSE if not configured.
   +   *
   +   * @var bool|string
   +   */
   +  protected $bundleKey;
   

   We don't need this. As long as we set $this->entityType in the constructor, we can call $this->entityType->getKey('bundle').

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,230 @@
   +    if (empty($plugin_definition['entity_type'])) {
   +      throw new InvalidPluginDefinitionException($plugin_id, 'Missing required entity_type definition');
   +    }
   

   We should throw an exception if the given entity type is a config entity type.

4. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,230 @@
   +    if (!empty($this->configuration['bundle'])) {
   +      if (!$this->bundleKey) {
   +        throw new InvalidPluginDefinitionException('A bundle was provided but entity is not bundleable.');
   +      }
   +    }
   

   These two if statements can be combined into one.

5. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,230 @@
   +  protected function yield(array $ids) {
   

   I don't think we should call this method 'yield' -- it's vague. How about 'loadEntities' or 'streamEntities'?

6. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,230 @@
   +      foreach ($entity->getTranslationLanguages(FALSE) as $language) {
   +        yield $this->toArray($entity->getTranslation($language->getId()));
   +      }
   

   We should not be doing this unless $entity instanceof TranslatableInterface.

7. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,230 @@
   +   * Convert entity to array.
   +   *
   +   * @param \Drupal\Core\Entity\ContentEntityInterface $entity
   +   *   The entity to convert.
   +   *
   +   * @return array
   +   *   The entity converted to array.
   +   */
   

   Can we expand this doc comment to explain how, exactly, an entity is converted to an array, and why we are doing this extra massaging?

8. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,230 @@
   +    foreach ($entity->toArray() as $field => $values) {
   +      $return[$field] = $values;
   +    }
   

   This seems pointless. Can't we just do $return = $entity->toArray()?

9. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
   @@ -0,0 +1,230 @@
   +    // The ids must be flat, they cannot be nested for the id map.
   

   IDs, ID

10. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
    @@ -0,0 +1,230 @@
    +      $reset = reset($return[$id]);
    +      $return[$id] = reset($reset);
    

    What if $return[$id] is empty? We should probably throw some sort of exception.

11. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
    @@ -0,0 +1,230 @@
    +  /**
    +   * Query to retrieve entities.
    +   *
    +   * @return \Drupal\Core\Entity\Query\QueryInterface
    +   */
    +  public function query() {
    

    @return is missing a descrption.

12. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
    @@ -0,0 +1,230 @@
    +    $idKey = $entity_definition->getKey('id');
    +    $ids[$idKey] = $this->getDefinitionFromEntity($idKey);
    +    if ($entity_definition->isTranslatable()) {
    +      $langcode_key = $entity_definition->getKey('langcode');
    +      $ids[$langcode_key] = $this->getDefinitionFromEntity($langcode_key);
    

    Variable casing is inconsistent; $idKey should be $id_key.

Had a quick discussion with @catch.

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntity.php
@@ -0,0 +1,230 @@
+namespace Drupal\migrate_drupal\Plugin\migrate\source\d8;

--- /dev/null
+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntityDeriver.php

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d8/ContentEntityDeriver.php
@@ -0,0 +1,59 @@
+
+namespace Drupal\migrate_drupal\Plugin\migrate\source\d8;

+++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/d8/ContentEntityTest.php
@@ -0,0 +1,375 @@
+ * @group migrate_drupal_8

So one thought about the D8 API usage is that these are not actually D8 specific source plugins. They are current drupal version plugins. When we open D9 this will be the D9 source plugin deriver not the D8 source plugin deriver.

I'm not sure that the patch in #35 was the intended patch as it seems to be the same as the patch in #28 and not have the interdiff applied.

+++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/d8/ContentEntityTest.php
@@ -0,0 +1,375 @@
+ * @group migrate_drupal_8

On reviewing the interdiff I would have expected this to change as there's no way we'd find this and change it when we tag Drupal 9.

Attempt to create the patch that was intended in #35.

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,234 @@
   +/**
   + * Drupal content entity source from database.
   

   I think we should explain that this is the for the current version of Drupal.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntityDeriver.php
   @@ -0,0 +1,59 @@
   +
   +class ContentEntityDeriver extends DeriverBase implements ContainerDeriverInterface {
   

   Missing class doc.

This is getting really close.

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,234 @@
   + * Drupal content entity source from database.
   

   It's not really from the database...

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,234 @@
   +    if (!$this->entityType instanceof ContentEntityTypeInterface) {
   +      throw new InvalidPluginDefinitionException($plugin_id, 'The "entity" source plugin only supports content entities.');
   +    }
   

   Should we rename the plugin to content_entity, so it's clearer? (Also future-proof, in case we support config entities at some point too.)

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,234 @@
   +    if (!empty($this->configuration['bundle']) && !$this->entityType->getKey('bundle')) {
   

   Nit: We can use hasKey() instead of getKey() here.

4. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,234 @@
   +    return (string) sprintf('%s source', $this->entityType->id());
   

   I wonder if we shouldn't just use $this->entityType->getPluralLabel() here.

5. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,234 @@
   +   * Loads and yields a single entity one at a time.
   

   Should be "...yields entities, one at a time."

6. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,234 @@
   +   * Convert entity to array.
   

   Should be "Converts an entity to an array".

7. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,234 @@
   +   * Make all IDs flat values. All other values are returned as per
   

   "Makes all IDs into flat values"

8. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,234 @@
   +      if (empty($return[$id])) {
   +        throw new MigrateException(sprintf('Entity of type %s and id %s is missing its id.', $entity->getEntityType(), $entity->id()));
   +      }
   

   Does EntityMalformedException still exist? If so, that might be more appropriate here. Also, 'id' should be 'ID', and we should be calling $entity->getEntityTypeId() here. (getEntityType() returns EntityTypeInterface, not a string; the fact that the tests didn't catch this leads me to think that we need a test of this exception :)

Sorry, more shtuff...

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,232 @@
   +  /**
   +   * Static cache for bundle fields.
   +   *
   +   * @var array
   +   */
   +  protected $bundleFields = [];
   

   This doesn't appear to be used anywhere.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,232 @@
   +  /**
   +   * The entity definition.
   +   *
   +   * @var \Drupal\Core\Entity\EntityTypeInterface
   +   */
   +  protected $entityType;
   

   Nit: Can this say "The entity type definition"?

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,232 @@
   +  public function __toString() {
   +    return (string) sprintf('%s source', $this->entityType->getLabel());
   +  }
   

   I think we should simply return (string) $this->entityType->getPluralLabel() and leave it at that. The word "source" adds nothing.

4. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,232 @@
   +      foreach ($entity->getTranslationLanguages(FALSE) as $language) {
   +        yield $this->toArray($entity->getTranslation($language->getId()));
   +      }
   

   Maybe we should hide this behind a configuration option (include_translations), which is TRUE by default.

5. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,232 @@
   +   * Convert entity to array.
   

   Should be "Converts an entity to an array."

6. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,232 @@
   +   *   The entity converted to array.
   

   Should be "The entity, represented as an array."

7. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,232 @@
   +      // Force the IDs over top the previous values.
   +      unset($return[$id]);
   

   Why is the unset() necessary if we're overwriting $return[$id] anyway?

8. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,232 @@
   +    $field_definitions = $this->entityFieldManager->getBaseFieldDefinitions($this->entityType->id());
   +    $fields = [];
   +    foreach ($field_definitions as $field_name => $definition) {
   +      $fields[$field_name] = (string) $definition->getLabel();
   +    }
   +    if (!empty($this->configuration['bundle'])) {
   +      foreach ($this->entityFieldManager->getFieldDefinitions($this->entityType->id(), $this->configuration['bundle']) as $field_name => $definition) {
   +        $fields[$field_name] = (string) $definition->getLabel();
   +      }
   

   Rather than have two foreach loops, let's build a full array of $field_definitions first, by combining the base and bundle field definition arrays.

9. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,232 @@
   +  /**
   +   * Gets the field definition from a specific entity base field.
   +   *
   +   * @param string $key
   +   *   The field ID key.
   +   *
   +   * @return array
   +   *   An associative array with a structure that contains the field type, keyed
   +   *   as 'type', together with field storage settings as they are returned by
   +   *   FieldStorageDefinitionInterface::getSettings().
   +   *
   +   * @see \Drupal\migrate\Plugin\migrate\destination\EntityContentBase::getDefinitionFromEntity()
   +   */
   +  protected function getDefinitionFromEntity($key) {
   +    /** @var \Drupal\Core\Field\FieldDefinitionInterface $field_definition */
   +    $field_definition = $this->entityFieldManager->getBaseFieldDefinitions($this->entityType->id())[$key];
   +    return [
   +      'alias' => 'b',
   +      'type' => $field_definition->getType(),
   +    ] + $field_definition->getSettings();
   +  }
   

   This code might be useful to destinations as well. Perhaps it should go into a trait? I'm OK with discussing that in a follow-up, though.

10. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntityDeriver.php
    @@ -0,0 +1,62 @@
    +    // Translations don't make sense unless we have content_translation.
    +    return new static(
    +      $base_plugin_id,
    +      $container->get('entity_type.manager')
    +    );
    

    This comment either needs to be expanded or removed; nothing else in the method makes reference to content_translation.

11. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntityDeriver.php
    @@ -0,0 +1,62 @@
    +      // The source plugin only supports entities that are a content entity.
    

    I think we can remove this comment. "ContentEntityDeriver" pretty much sums it up.

12. +++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/ContentEntityTest.php
    @@ -0,0 +1,380 @@
    +  /**
    +   * The mock migration plugin.
    +   *
    +   * @var \PHPUnit_Framework_MockObject_MockObject
    +   */
    +  protected $migration;
    

    This should be dual-type hinted as both a mock object and MigrationInterface.

13. +++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/ContentEntityTest.php
    @@ -0,0 +1,380 @@
    +    $this->sourcePluginManager = $this->container->get('plugin.manager.migrate.source');
    +    $this->migration = $this->createMock(MigrationInterface::class);
    +    $idMap = $this->createMock(MigrateIdMapInterface::class);
    +    $idMap->expects(self::any())->method('getRowBySource')->willReturn(NULL);
    +    $this->migration->expects(self::any())->method('getIdMap')->willReturn($idMap);
    

    Is there any particular reason we don't use MigrationPluginManager::createStubMigration()? That would be a more "realistic" test than setting up mock objects.

14. +++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/ContentEntityTest.php
    @@ -0,0 +1,380 @@
    +  /**
    +   * Create a media type for a source plugin.
    +   *
    +   * @param string $media_source_name
    +   *   The name of the media source.
    +   *
    +   * @return \Drupal\media\MediaTypeInterface
    +   *   A media type.
    +   */
    +  protected function createMediaType($media_source_name) {
    +    $media_type = MediaType::create([
    +      'id' => 'image',
    +      'label' => 'Image',
    +      'source' => $media_source_name,
    +      'new_revision' => FALSE,
    +    ]);
    +    $media_type->save();
    +    $source_field = $media_type->getSource()->createSourceField($media_type);
    +    // The media type form creates a source field if it does not exist yet. The
    +    // same must be done in a kernel test, since it does not use that form.
    +    // @see \Drupal\media\MediaTypeForm::save()
    +    $source_field->getFieldStorageDefinition()->save();
    +    // The source field storage has been created, now the field can be saved.
    +    $source_field->save();
    +    $media_type->set('source_configuration', [
    +      'source_field' => $source_field->getName(),
    +    ])->save();
    +    return $media_type;
    +  }
    

    The Media module has a trait that we can use for this: MediaFunctionalTestCreateMediaTypeTrait. It will work in kernel tests.

Instead of creating a new issue for a 8.5.x backport, I'll just upload the two patches for the two versions here, with the only difference being how we test the taxonomy term parents. I think it's easier to follow the work on these patches if they are in the same issue.

A few minor improvements. I think this is ready for another review. Meanwhile, I'll write a change record, probably tomorrow.

I've been trying to write a change record for this issue and provide some usage examples, but there's something I don't understand in this patch, and here's a few questions to help me understand.

What does a source plugin deriver does? I know that, for example, a migration plugin deriver will generate multiple migrations plugins, and run them separately, but a deriver for a source plugin I don't understand the usage. Source plugins are used in a migration plugin, so how does one migration would use multiple derived source plugins?

And in fact, the deriver in this patch does not even seem to be working. If I create a migration with the content_entity source plugin, it will fail saying that it's missing the entity_type property, which is supposed to be the thing that is being derived.

Do we really need the deriver here? And if so, can someone clarify how it would be used?

What does a source plugin deriver does? I know that, for example, a migration plugin deriver will generate multiple migrations plugins, and run them separately, but a deriver for a source plugin I don't understand the usage.

Source plugin derivers do the same thing as migration plugin derivers, for the same purpose. As with any derived plugin, it lets us have several variants of a single plugin, each of which is only slightly different from the others. Derivatives are a core feature of the plugin system in general, to help with code reuse.

Source plugins are used in a migration plugin, so how does one migration would use multiple derived source plugins?

A migration would not use multiple source plugins, period. It doesn't matter whether or not they were derived; a migration has a single source plugin and that's that. The source plugin in use could be a variant created by a deriver, but it's still only a single variant that will be used in any given migration.

@phenaproxima, @heddn: Thanks, that makes sense.

Discussed in the weekly migrate meeting. We should add doxygen documentation with usage examples and we should remove the term parent tests in the 8.5.x backport. We should also create a follow up for the 8.5.x backport to do add a check in the source to see if it's taxonomy terms and load the parent.

Here's a first draft for the plugin documentation.

I also created the follow up #2940198: Load taxonomy term parents in ContentEntity source plugin.

Reading your CR, I guess it wouldn't hurt to add the entity_type config in the doc block, in fact, I think we need it.

Only found a few things. And only one them is not a documentation nitpick.

This patch is looking fantastic.

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,279 @@
   + * to be extended and the new class will need to load/calculated the values for
   

   Typo: "calculated" should be "calculate".

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,279 @@
   + * - entity_type: The entity type id of the entities being exported. This is
   

   "id" should be "ID".

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,279 @@
   + *   is not utilized i.e. a custom source plugin.
   

   Missing commas. Should be "...utilized, i.e., a custom..."

4. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,279 @@
   + * - bundle: (optional) String, if the entity type is bundleable, only return
   + *   entities of this bundle.
   

   We can lose "String,"

5. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,279 @@
   + * - exclude_translations: (optional) Boolean, indicates if the entity
   + *   translations should be excluded, default to FALSE.
   

   We can lose the "Boolean,", I think. "Default" should be "defaults". Personally, I'd prefer if exclude_translations was a positive switch -- include_translations, defaulting to TRUE, but that's just a preference.

6. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,279 @@
   + * This will return all nodes, from every bundle and every translation.
   

   We should mention that this does NOT load all revisions, just the default one.

7. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,279 @@
   +    if (!empty($this->configuration['bundle']) && !$this->entityType->hasKey('bundle')) {
   

   We need to look at $configuration['bundle'], not $this->configuration['bundle'], because AFAIK $this->configuration is set by the parent::__construct() call. The fact that we didn't catch this indicates that we should probably have a couple of unit tests of this constructor.

8. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,279 @@
   +  public function __toString() {
   +    return (string) $this->entityType->getPluralLabel();
   +  }
   

   Lovely!

9. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,279 @@
   +    // Retrieving fields from a non-fieldable content entity will return a
   

   Supernit: "return" should be "raise" or "throw" :)

10. +++ b/core/modules/migrate_drupal/tests/src/Kernel/Plugin/migrate/source/ContentEntityTest.php
    @@ -0,0 +1,385 @@
    +   * The vocabulary id.
    +   *
    +   * @var string
    +   */
    +  protected $vocabulary = 'fruit';
    

    "id" --> "ID"

Re #62.5: I would also prefer include_translations over exclude_translations, I don't like the double negative.

This is ready. Thanks, everyone!

From #31

And obviously is it good we have test coverage of the sources here but it would also be great to see a full migration test like we have for Drupal 6 and 7. Basically a Drupal 8 equivalent of \Drupal\Tests\migrate_drupal_ui\Functional\d7\MigrateUpgrade7Test. Not in this issue but as a follow-up.

I can't see where that follow up was created?

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,280 @@
   +    if (!empty($configuration['bundle']) && !$this->entityType->hasKey('bundle')) {
   +      throw new InvalidPluginDefinitionException($plugin_id, 'A bundle was provided but the entity type is not bundleable.');
   +    }
   

   should we also validate that the bundle is valid for the given entity type?

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,280 @@
   +      yield $this->toArray($entity);
   ...
   +          yield $this->toArray($entity->getTranslation($language->getId()));
   

   nice

   is there an issue with the amount of memory the static cache of entities on the storage handler will use?

   should we be reseting cache periodically?

3. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,280 @@
   +    $fields = [];
   +    foreach ($field_definitions as $field_name => $definition) {
   +      $fields[$field_name] = (string) $definition->getLabel();
   +    }
   

   aside:could use array_map here

4. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -0,0 +1,280 @@
   +      'alias' => 'b',
   

   What is this all about?

Follow-up created for the full migration test: #2940806: Add a full migration test for the ContentEntity source plugin

1. I added a check to see if the bundle is valid, and a corresponding test.
2. Not sure about this.
3. Changed for an array_map().
4. Removed. I think it was a leftover from before, when we used database queries instead of the Entity API.

#70.2 there's #1596472: Replace hard coded static cache of entities with cache backends and #1199866: Add an in-memory LRU cache open to improve entity static caching and memory.

One thing I spotted. It might be early in the morning, but I'm struggling a bit with exactly how translations are or are not included, could maybe use some high level documentation somewhere?

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
@@ -0,0 +1,293 @@
+   */
+  public function query() {
+    $query = $this->entityTypeManager
+      ->getStorage($this->entityType->id())
+      ->getQuery();

This should have an accessCheck(FALSE).

1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -89,18 +97,24 @@ class ContentEntity extends SourcePluginBase implements ContainerFactoryPluginIn
   +      if (!in_array($configuration['bundle'], array_keys($this->entityTypeBundleInfo->getBundleInfo($this->entityType->id())))) {
   

   Just for readability, can we expand this long if statement into a couple of lines? Also, the in_array() call should have TRUE passed as the third argument.

2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
   @@ -89,18 +97,24 @@ class ContentEntity extends SourcePluginBase implements ContainerFactoryPluginIn
   +        throw new InvalidPluginDefinitionException($plugin_id, 'The provided bundle is not valid for this entity type.');
   

   We should not throw InvalidPluginDefinitionException here, because the definition is valid; it's the configuration that isn't. InvalidArgumentException should be fine in this case.

I think all feedback has been addressed, let's get this back to RTBC so we can un-postpone the other criticals.

I agree. +1 for RTBC.

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

FYI: the changes made in #71 to throw an exception when an invalid bundle is specified breaks during rollbacks. See #3186449: Rolling back a migration implementing MigrationWithFollowUpInterface does not clear the generated follow up migrations from the cache.. Feedback welcome!