Problem/Motivation

For we need to source data from D8. We started building something ourselves in #2912348: Handle entity_references related to Drupal 6 and 7 node translations with different IDs. Then found out that https://www.drupal.org/project/migrate_drupal_d8 already has much of that functionality. Let's move it into core instead of building something new.

Proposed resolution

Discussed where this should land and it was decided in the weekly migrate meeting to land all of the code directly into migrate_drupal.

Remaining tasks

  • Do it.
  • Review it.

User interface changes

None.

API changes

Addition: A new D8 content entity source plugin and its deriver.

Data model changes

None.

CommentFileSizeAuthor
#77 interdiff_74-77.txt4.16 KBheddn
#77 2935951-77.patch29.25 KBheddn
#74 interdiff_71-74.txt762 bytesheddn
#74 2935951-74.patch28.99 KBheddn
#71 interdiff-2935951-66-70.txt5.56 KBmaxocub
#71 2935951-70.patch28.99 KBmaxocub
#66 2935951-66.patch27.8 KBheddn
#66 interdiff_64-66.txt881 bytesheddn
#64 2935951-64.patch27.78 KBheddn
#64 interdiff_59-64.txt10.17 KBheddn
#59 interdiff_58-59.txt1.35 KBheddn
#59 2935951-8.6.x-59.patch25.92 KBheddn
#59 2935951-8.5.x-58.patch25.92 KBheddn
#58 interdiff_55-57.txt1.11 KBheddn
#58 2935951-8.6.x-57.patch25.54 KBheddn
#58 2935951-8.5.x-57.patch25.62 KBheddn
#55 interdiff_54-55.txt907 bytesheddn
#55 2935951-8.6.x-55.patch25.08 KBheddn
#55 2935951-8.5.x-55.patch25.16 KBheddn
#54 interdiff-2935951-49-54-85x.txt4.36 KBmaxocub
#54 2935951-54-85x-backport.patch25.28 KBmaxocub
#54 interdiff-2935951-49-54-86x.txt2.78 KBmaxocub
#54 2935951-54-86x.patch25.19 KBmaxocub
#49 2935951-49-86x.patch24.46 KBmaxocub
#49 interdiff-2935951-47-49-86x.txt11.58 KBmaxocub
#49 2935951-49-85x-backport.patch24.68 KBmaxocub
#49 interdiff-2935951-47-49-85x.txt12.14 KBmaxocub
#47 2935951-47-85x-backport.patch24.65 KBmaxocub
#47 2935951-47-86x.patch24.44 KBmaxocub
#47 interdiff-2935951-44-47.txt1.64 KBmaxocub
#44 2935951-44.patch24.54 KBheddn
#44 interdiff_42-44.txt15.17 KBheddn
#42 interdiff_41-42.txt1.1 KBheddn
#42 2935951-42.patch25.04 KBheddn
#41 2935951-41.patch25.14 KBheddn
#41 interdiff_38-41.txt8.7 KBheddn
#38 2935951-38.patch24.99 KBheddn
#37 2935951-37.patch25 KBJo Fitzgerald
#37 interdiff-28-37.txt45.54 KBJo Fitzgerald
#35 2935951-28.patch24.12 KBheddn
#35 interdiff_28_35.txt12.51 KBheddn
#28 2935951-28.patch24.12 KBheddn
#28 interdiff_26-28.txt601 bytesheddn
#26 diff.txt117 bytesquietone
#26 2935951-26.patch24.15 KBquietone
#24 interdiff.txt1.48 KBquietone
#24 2935951-24.patch24.25 KBquietone
#22 2935951-22.patch24.57 KBheddn
#22 interdiff_20-22.txt1.63 KBheddn
#20 2935951-20.patch22.93 KBheddn
#20 interdiff_18-20.txt10.47 KBheddn
#18 interdiff_15-17.txt15.82 KBheddn
#18 2935951-17.patch21.91 KBheddn
#15 2935951-15.patch18.48 KBheddn
#12 2935951-12.patch26.79 KBheddn
#12 interdiff_10-12.txt641 bytesheddn
#10 interdiff-2935951-7-10.txt1.8 KBmaxocub
#10 2935951-10.patch26.87 KBmaxocub
#7 interdiff-2935951-4-6.txt2.53 KBmaxocub
#7 2935951-6.patch26.59 KBmaxocub
#4 interdiff_3-4.txt3.57 KBheddn
#4 2935951-4.patch26.29 KBheddn
#3 2935951-3.patch25.61 KBheddn
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

heddn created an issue. See original summary.

heddn’s picture

Issue summary: View changes

Also this is critical because it blocks finishing a critical.

heddn’s picture

Status: Active » Needs review
FileSize
25.61 KB

Term test will fail. We also need to support translatable fields. But I want to see what else fails.

heddn’s picture

We need to reload the entity to fix the term tests.

heddn’s picture

Assigned: heddn » Unassigned

Unassigning for now.

The last submitted patch, 3: 2935951-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

maxocub’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
FileSize
26.59 KB
2.53 KB

This should add support for translations.

Needs tests for translations, so back to work.

heddn’s picture

heddn’s picture

maxocub’s picture

Status: Needs work » Needs review
FileSize
26.87 KB
1.8 KB

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).

Status: Needs review » Needs work

The last submitted patch, 10: 2935951-10.patch, failed testing. View results

heddn’s picture

Hopefully fixes the last failure here.

quietone’s picture

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?

phenaproxima’s picture

Status: Needs review » Needs work
  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!

heddn’s picture

Title: Move migrate_drupal_d8 into migrate_drupal » Copy migrate source plugin from migrate_drupal_d8 into migrate_drupal
Status: Needs work » Needs review
FileSize
18.48 KB

Linking to #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration, since that blocks full taxonomy term support.

No interdiff as there's enough changes it wouldn't make sense. This still doesn't support computed fields or custom fields, but you can easily grok the code and add these via a prepareRow. This also doesn't have any deriver yet.

Status: Needs review » Needs work

The last submitted patch, 15: 2935951-15.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
Issue tags: +Needs change record

OK, this passes tests locally. It has a deriver and should be ready for adding tests for i18n. Leaving the tests tag so we don't forget to do that. Also tagging for a change record so we don't forget to do that.

heddn’s picture

FileSize
21.91 KB
15.82 KB
phenaproxima’s picture

Status: Needs review » Needs work

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.

heddn’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
10.47 KB
22.93 KB

So, this adds i18n export by default. And addresses most of the feedback in #19.

1, 2, 13, 14 => this is intentional. We want this source to be usable outside of a deriver where someone extends it and add their own custom queries. Using the deriver id would make that more troublesome.
3 => edge case handling is helpful. I suggest we leave it.
4, 5, 6, 8, 9, 10, 11, 13 => fixed
7 => It might not be ideal, but its the only way I know of to return the data in a valid manner. I'm open to suggestions though.
9 => How can we calculate a valid count that includes translations? I'm not sure.

Status: Needs review » Needs work

The last submitted patch, 20: 2935951-20.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

heddn’s picture

Status: Needs work » Needs review
FileSize
1.63 KB
24.57 KB

This will probably still fail. But I couldn't get xdebug to pause so I could figure out what was going on. It seems to me like the entity schema isn't getting completely installed. But that doesn't make sense since I'm now explicitly installing it. Putting this aside for now. Maybe someone else will figure out what is wrong.

Status: Needs review » Needs work

The last submitted patch, 22: 2935951-22.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

quietone’s picture

Status: Needs work » Needs review
FileSize
24.25 KB
1.48 KB

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

maxocub’s picture

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

quietone’s picture

FileSize
24.15 KB
117 bytes

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

maxocub’s picture

Status: Needs review » Needs work
+++ 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.

heddn’s picture

Status: Needs work » Needs review
FileSize
601 bytes
24.12 KB
alexpott’s picture

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

heddn’s picture

I've opened #2937045: source plugin source_module testing seems incomplete as a follow-up for why #27 wasn't caught in tests.

alexpott’s picture

  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.

phenaproxima’s picture

Status: Needs review » Needs work

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.

alexpott’s picture

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.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

heddn’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Needs work » Needs review
FileSize
12.51 KB
24.12 KB

I fixed all the comments from #31 and #34.

For #32:
1-5 fixed
6: How would we know at this point if translations are turned on for an entity? I'd like to know, because then we could do that same logic and still call count when the entity is not translatable. See #2937166: Improve count() for ContentEntity source plugin. However, I think translations is at a bundle level, not a entity-wide setting. So we don't really know since the generic support must handle both. I'd be willing to punt this to a follow-up. The only concern is with BC if we do that, since it could mean that the mapping table would need to be rebuilt to /not/ include a langcode. What is the hurt if we do include langcode? Then we naturally support multi-lingual if someone ever turns it on...
7-12 fixed

I also opened up #2937166: Improve count() for ContentEntity source plugin as a follow-up to better calculate counts.

alexpott’s picture

Status: Needs review » Needs work

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.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
45.54 KB
25 KB

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

heddn’s picture

FileSize
24.99 KB

Here's the patch that was intended in #35. Th lone diff I'm seeing is a response to #36, where it uses 'migrate_drupal' for the test group. Sorry for the snafu.

$ diff 2935951-37.patch 2935951-38.patch
334c334
< index 0000000..35819b4
---
> index 0000000..f290c19
361c361
< + * @group migrate_drupal_8
---
> + * @group migrate_drupal
alexpott’s picture

  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.

phenaproxima’s picture

Status: Needs review » Needs work

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 :)

heddn’s picture

Status: Needs work » Needs review
FileSize
8.7 KB
25.14 KB

#39 and #40 addressed. Except for adding tests for 40.8.

I'm not sure how to test that. I did some investigation into typed data and I really don't think it is possible for that exception to ever get reached. I'm almost inclined to remove the check and exception. @phenaproxima, you asked for it originally. Is it possible for TypedData to not return a double array nested response?

heddn’s picture

FileSize
25.04 KB
1.1 KB

I did some more digging into situations where the exception might occur. I don't think that if the backend is sql-based, it is very realistic. The ids are going to be required to be either primary key or unique keys by SqlContentEntityStorageSchema. This means that it would be impossible to store the data in the database for later use by this source plugin without some value.

I've also now switched to type hint on ItemList->first()->getString(). Which makes it more clear we are returning a single value from the first element in the entity's id property. And if that should fail for some reason, then we'll throw the appropriate entity API errors/warnings/exceptions; which will just naturally get bubbled up.

phenaproxima’s picture

Status: Needs review » Needs work

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.

heddn’s picture

Version: 8.5.x-dev » 8.6.x-dev
Status: Needs work » Needs review
FileSize
15.17 KB
24.54 KB

I've added #2937782: Create trait for getDefinitionFromEntity to address #43.9. The rest of the points are addressed. I also added a parent check on terms since[#2543726] is now committed. That makes this a 8.6 thing only thing right now. We'll have to do a special backport patch to remove those test checks on 8.5.

Tests pass locally.

Status: Needs review » Needs work

The last submitted patch, 44: 2935951-44.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

heddn’s picture

Status: Needs work » Needs review

Seems it wanted to test 8.5, even though I switched to 8.6. So, back to NR.

maxocub’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 47: 2935951-47-85x-backport.patch, failed testing. View results

maxocub’s picture

maxocub’s picture

Status: Needs review » Needs work

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?

phenaproxima’s picture

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.

heddn’s picture

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.

To use it, you have to use it like the destination plugin and its deriver. For the user entity, it would look like: content_entity:user, where 'user' is the entity type id. Now, why does it complain about a missing entity_type? That's because the plugin is built with extensibility in mind. Not everyone who uses the source plugin will use it by passing in the entity_type. Because they will want to extend from this class and include additional fields or data. So, for that purpose, we automatically map the entity_type in the deriver, but give it as an argument/configuration if you decided to extend and then cannot take advantage of the deriver.

maxocub’s picture

@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.

maxocub’s picture

Here's a first draft for the plugin documentation.

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

heddn’s picture

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/ContentEntity.php
@@ -13,7 +13,31 @@
+ * This will return all nodes, from every bundles (if this entity type is
+ * bundleable) and every translations (if this entity type is translatable).

Nodes are bundleable and translatable, so these phrases aren't needed. Patch with fixes attached. Otherwise, I think all the feedback here is addressed and we're ready for a final review.

Only a single interdiff uploaded, because the changes between 8.5 and 8.6 are identical. Remember, the only difference between 8.5 and 8.6 is in test coverage and the interdiff of those changes is in #54.

heddn’s picture

Issue tags: -Needs change record

I also wonder if we should add details about the entity_type config (required) config option. I stumbled on that question while writing the CR.

maxocub’s picture

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.

heddn’s picture

I've added more details about setting the entity_type and why you ever might want to extend and build your own custom source plugin. This should address my question in #56. /Now/ we are ready for a final review.

heddn’s picture

While reviewing #2630732-36: Implement Entity::fields() for migration destinations, I stumbled upon the fact that you can create a content entity that is non-fieldable. This would be very hard to do, because ContentEntityBase is /the/ default class to extend when working with entities. All core entities extend from it. And it implements ContentEntityInterface, which implements FieldableEntityInterface. And its actually pretty mind-boggling to consider creating content entities that aren't fieldable. But for completeness sake, I've added a check.

The last submitted patch, 59: 2935951-8.5.x-58.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 59: 2935951-8.6.x-59.patch, failed testing. View results

phenaproxima’s picture

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"

maxocub’s picture

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

heddn’s picture

Status: Needs work » Needs review
FileSize
10.17 KB
27.78 KB

I've dropped the 8.5 patch in this post. Once we get closer to RTBC, I'll re-add it. It's just too hard keeping all of everything in sync for 8.5.

All feedback in #62 should be addressed.

Status: Needs review » Needs work

The last submitted patch, 64: 2935951-64.patch, failed testing. View results

heddn’s picture

Status: Needs work » Needs review
FileSize
881 bytes
27.8 KB

I think we ran into https://github.com/doctrine/common/issues/744 for those failures.

This should fix things.

heddn’s picture

This was fixed upstream already, but we're still using 2.6.2 of doctrine/common, which doesn't have the fixes.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

This is ready. Thanks, everyone!

heddn’s picture

Let's leave this open after the 8.6.x commit for the follow-up commit to 8.5.x? Its only a couple lines and will be way easier to re-roll in here than opening a follow-up.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs followup

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?

maxocub’s picture

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.
heddn’s picture

Status: Needs review » Reviewed & tested by the community

#70.2: The migrate executable has memoryExceeded(). If we consume more than 85% of available memory, it will stop the migration cleanly. So, I think that is covered already. One can always re-run the migration from that point and pick up from where it stopped off.

If this come back green, this should be good to go again.

catch’s picture

#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).

heddn’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
28.99 KB
762 bytes

re: translations
There is a flag include_translations. If true (the default), yieldEntities calls getTranslationLanguages and returns all non-default languages. The default language will always be the first returned, followed by any additional languages.

There's docs on include_translations in the doxygen. Is more needed?

Attached is the requested accessCheck logic change.

heddn’s picture

Also re 71: and the full test... I thought the full migration tests would be all the follow-up issues for i18n this issue is blocking. Of course, we can always add more tests, but those might be enough to give us full coverage.

phenaproxima’s picture

  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.

heddn’s picture

FileSize
29.25 KB
4.16 KB

#77 addressed. Although I'm not as sure we should do a strict match. The entity query won't be that strict itself on /most/ sql implementations.

maxocub’s picture

Status: Needs review » Reviewed & tested by the community

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

phenaproxima’s picture

I agree. +1 for RTBC.

  • catch committed ef5b216 on 8.6.x
    Issue #2935951 by heddn, maxocub, quietone, Jo Fitzgerald, phenaproxima...
catch’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

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

  • catch committed eddad1b on 8.5.x
    Issue #2935951 by heddn, maxocub, quietone, Jo Fitzgerald, phenaproxima...