Problem/motivation

entity_id/type are required properties for a comment. So there are chances that weird things will start happening in migrations in the future, if we leave those fields empty for stub comments.

What prompted this:

#2318875: Redo CommentStatisticsInterface is going to change the function signature of CommentStatisticsInterface::update(), so Drupal\comment\Entity\Comment::postSave was going to contain this code:

    // Update the {comment_entity_statistics} table
    \Drupal::service('comment.statistics')->update($this->getCommentedEntity(), array($this->getFieldName()));

...and d6\MigrateCommentTest / MigrateDrupal6Test started failing with:
Argument 1 passed to Drupal\comment\CommentStatistics::update() must implement interface Drupal\Core\Entity\ContentEntityInterface, null given

Proposed resolution

The proposed patch was initially dubbed a 'cheat', because there is no clean way to derive which types of entities have already been imported. This is still true, but the approach has approval from Migrate people and the refined patch should work with > 99.9% of migrations in practice.

(Note: First I thought "let's make a property/method that specifies which destination fields must be filled for a stub entity". But while coding I ran into a wall: I don't know how to cleanly derive the source values connected to those destination fields.)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

roderik’s picture

Issue summary: View changes
dawehner’s picture

Category: Feature request » Bug report
Priority: Normal » Major

Note: At the moment it already breaks in case you have RDF module enabled.
Maybe you can use this in order to provide a test case?

andypost’s picture

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityComment.php
@@ -87,4 +101,28 @@ public function import(Row $row, array $old_destination_id_values = array()) {
+          $this->stubCommentedEntityId = $result[0]['nid_min'];
+          $this->stubCommentedEntityType = 'node';

Take a look at Comment::preCreate() we require comment_type, so entity type and field name should create stubs?

dawehner’s picture

FileSize
3.73 KB
2.62 KB

Take a look at Comment::preCreate() we require comment_type, so entity type and field name should create stubs?

I think adding a comment_type is fine for now?

In general the patch works fine, though I dont' really have a good idea how to test it. Let's see whether RDF fails.

Status: Needs review » Needs work

The last submitted patch, 4: migrate-2340401-4.patch, failed testing.

roderik’s picture

Status: Needs work » Needs review
FileSize
2.51 KB
2.42 KB

We're already adding comment_type (the bundle name) in Drupal\migrate\Plugin\migrate\destination\Entity::getEntity() at the moment, so that's covered.

Only one test failure, it seems the insert 'duplicate key in node table' is caused not by adding rdf, but by the other change made in testComments(). I'm not sure; will just undo the change and let the testbot tell me.

I don't know how to test it 'properly' either. But when this goes in, I can reverse the 2nd code block in #2318875-13: Redo CommentStatisticsInterface, and that will be an implicit test because it will generate a "you are not allowed to pass NULL into \Drupal::service('comment.statistics')->update()" error when the stub comment gets inserted. Would that do, for now?

roderik’s picture

Summarizing the test failure in #4: @dawehner so moving

    \Drupal::service('comment.manager')->addDefaultField('node', 'story');
    /** @var \Drupal\migrate\entity\Migration $migration */
    $migration = entity_load('migration', 'd6_comment');

    $dumps = array(
      $this->getDumpDirectory() . '/Drupal6Node.php',
      $this->getDumpDirectory() . '/Drupal6CommentVariable.php',
      $this->getDumpDirectory() . '/Drupal6Comment.php',
    );
    $this->prepare($migration, $dumps);
    $executable = new MigrateExecutable($migration, $this);
    $executable->import();

from setUp() into testComments(), gives an Integrity constraint violation: 1062 Duplicate entry '1' for key 'PRIMARY': INSERT INTO {node} ". I don't know why but I'm not sure it's worth for me to investigate because it doesn't seem to be important for this issue.

So, I think your comment/new patch implicitly validates my 'cheat' (a term I used in the issue summary) and this is good for inclusion?
That is, if the note made about an 'implicit test' of this patch in #6 (last paragraph) is OK.

andypost’s picture

It looks like quick hack but the cause is comment entity save, will try to dig in

  1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityComment.php
    @@ -87,4 +101,28 @@ public function import(Row $row, array $old_destination_id_values = array()) {
     
    +  protected function getEntity(Row $row, array $old_destination_id_values) {
    +    if ($row->stub()) {
    

    needs doc block

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityComment.php
    @@ -87,4 +101,28 @@ public function import(Row $row, array $old_destination_id_values = array()) {
    +        // Since it's hard to derive the 'right' source values for this stub
    +        // record in a migration agnostic way, and since the entity will be
    +        // overwritten anyway... just try to pick a node.
    +        $result = \Drupal::entityQueryAggregate('node')
    +          ->aggregate('nid', 'MIN')
    

    fragile...

andypost’s picture

migration should set "comment type" (there's only one for d6&7 core's shipped)

  public static function preCreate(EntityStorageInterface $storage, array &$values) {
    if (empty($values['comment_type']) && !empty($values['field_name']) && !empty($values['entity_type'])) {
      $field_storage = FieldStorageConfig::loadByName($values['entity_type'], $values['field_name']);
      $values['comment_type'] = $field_storage->getSetting('comment_type');
    }
  }

Also comment type entity could not be skipped so because it's THE bundle of comment entity

roderik’s picture

FileSize
2.54 KB
679 bytes

8.1 added doc block

8.2 correct, it's fragile. This is why I name it a 'cheat' and I will leave it to more experienced migrate-people to judge if this is even allowed. I didn't know a better way than this.

9.

migration should set "comment type"

This (setting the bundle) is done in the parent. Drupal\migrate\Plugin\migrate\destination\Entity::getEntity():

  protected function getEntity(Row $row, array $old_destination_id_values) {
    $entity_id = $old_destination_id_values ? reset($old_destination_id_values) : $row->getDestinationProperty($this->getKey('id'));
    if (!empty($entity_id) && ($entity = $this->storage->load($entity_id))) {
      $this->updateEntity($entity, $row);
    }
    else {
      $values = $row->getDestination();
      // Stubs might not have the bundle specified.
      if ($row->stub()) {
        $bundle_key = $this->getKey('bundle');
        if ($bundle_key && !isset($values[$bundle_key])) {
          $values[$bundle_key] = reset($this->bundles);
        }
      }
      $entity = $this->storage->create($values);
      $entity->enforceIsNew();
    }
    return $entity;
  }

(If I did not understand something in #9, please tell me.)

andypost’s picture

Assigned: Unassigned » benjy

Let's get feedback from migrate pov.
RTBC +1 from me

benjy’s picture

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityComment.php
@@ -87,4 +101,31 @@ public function import(Row $row, array $old_destination_id_values = array()) {
+        $result = \Drupal::entityQueryAggregate('node')
+          ->aggregate('nid', 'MIN')
+          ->execute();
+        if ($result) {
+          $this->stubCommentedEntityId = $result[0]['nid_min'];
+          $this->stubCommentedEntityType = 'node';
+        }

I see where you're coming from with this, the node migration has to run before comment and if there are no nodes there will be no comments so this should always work. One question, Is there any problems if the node id selected is of a bundle that doesn't have a comment field attached?

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityComment.php
@@ -87,4 +101,31 @@ public function import(Row $row, array $old_destination_id_values = array()) {
+          $this->stubCommentedEntityType = 'node';
...
+        $row->setDestinationProperty('entity_type', $this->stubCommentedEntityType);

This isn't really doing anything since it's set as a constant in the migration anyway but I understand it would break if the entity type wasn't node. We should probably pass this stuff in as configuration because it would good if it was possible to make the entity type relevant but we could do that in a follow-up.

I can reverse the 2nd code block in #2318875-13: Redo CommentStatisticsInterface: Redo CommentStatisticsInterface, and that will be an implicit test because it will generate a "you are not allowed to pass NULL into \Drupal::service('comment.statistics')->update()" error when the stub comment gets inserted. Would that do, for now?

Yes, I think that is fine. It's not for Migrate to test Comment's API's and I presume you already have the appropriate test coverage. We could have added unit tests for the comment destination but very few other destinations have coverage so I don't think it would be fair to hold this issue up on that.

andypost’s picture

Comments uses "comment_type" as bundle so #9 explains the case when no bundle set (really bad because we need to find node with fields)

Is there any problems if the node id selected is of a bundle that doesn't have a comment field attached?

no comment no problem. Comments are displayed only with "comment field formatter" so when comment is bound to node without comment-field it will not shown

benjy’s picture

Assigned: benjy » Unassigned

In #12, I was referring to the entity type the comment is attached too, nevertheless, I think this issue is ready.

I also ran MigrateCommentTest locally with nothing other than the RDF module enabled and the test does fail. Maybe a comment above $modules explaining why it's there would be good though?

Then, RTBC for me.

roderik’s picture

OK, I hope this works for a comment.

Re #14 as far as I know a comment does not normally refer back to its commented entity's field configuration, by itself. So indeed, if a comment is attached to an entity without a comment field, that shouldn't be fatal.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

That looks OK to me apart from it should have been "RDF" but I'm not gonna hold it up for that.

andypost’s picture

Assigned: Unassigned » larowlan

Let's get @larowlan review

larowlan’s picture

Why does it store the entity type as a class property, seems hardwiring would be fine, it's always node.
$this->stubCommentedEntityType = 'node';

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Reviewed & tested by the community » Needs review
andypost’s picture

@larowlan That's right for d6 but d7 can carry different fields per node type

andypost’s picture

In #20 I mean a commentType but stubCommentedEntityType is always "node" so should be moved to constants.
Once someone will try to migrate some entities to comments for that purpose he will need to hack core or override this EntityComment plugin... dx--
I'm sure that parent node(entity) migration should run before the comment and when there's no parent at the time of comment import then this row should be simply skipped

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityComment.php
@@ -87,4 +101,31 @@ public function import(Row $row, array $old_destination_id_values = array()) {
+  protected function getEntity(Row $row, array $old_destination_id_values) {
+    if ($row->stub()) {
...
+      if (empty($this->stubCommentedEntityId)) {

I really can't get the reason to import comments that have no parent node imported.
Suppose better to throw exception

+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateCommentTest.php
@@ -19,7 +19,9 @@
-  static $modules = array('node', 'comment');
+  // rdf module serves as an implicit test that comments' entity_id is set;

s/rdf/Rdf

benjy’s picture

Regarding #18, we may as well remove it entirely, it's set in the migration YAML as a constant anyway.

source:
  plugin: d6_comment
  constants:
    entity_type: node
...
entity_type: 'constants/entity_type'

I did also mention in #12, that we might want to move this to configuration for the destination in a follow-up. That would be better still.

roderik’s picture

@benjy could you poke at this (extra) approach? I wanted to derive the "node" constant from the migration definition, but I'm not sure if there is a cleaner method than just going through the definitions as I did.

...in #18 vs #12 we have "hardwiring 'node' would be fine" vs "we could add a configuration option in a followup", but in practice, ~99% of migration definitions will define the entity type as a constant (in the source) already. We can refine the current approach by going with that. Points:

  • We could define (in a followup) extra configuration, but this would only be necessary for migrations which did not define entity_type as a constant, in situations where no nodes are imported/present. That is a very small number of sites.
  • ...and I'm not sure whether @benjy meant defining an extra configuration value (for the destination), but if so, many people wouldn't really understand why they had to define it (in two places). So the fallback to 'node' is still in there.
  • latest code also makes clearer why we don't hardcode "node" as an enitity type: the class should also work with migrations that only migrate non-node entities with comments.

Also: throwing exception now, as noted by andypost/#21. With a MigrateSkipRowException you can get loads of error messages instead of one, but it seems the best solution.

roderik’s picture

Fix unnecessary line

andypost’s picture

@roderik Thanx for working on this! The reasons for "node" makes sense only in context of migration from previous version of core. That means that comments before d8 are "sourced" from nodes only. But destination could be any

benjy’s picture

Status: Needs review » Needs work

Few small things and then I think this is looking good.

  1. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityComment.php
    @@ -87,4 +102,59 @@ public function import(Row $row, array $old_destination_id_values = array()) {
    +          if (isset($this->migration->process['entity_type'])
    +              && is_string($this->migration->process['entity_type'])
    +              && strpos($this->migration->process['entity_type'], 'constants/')
    +                 == 0) {
    

    Can we put all this up on one line?

  2. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityComment.php
    @@ -87,4 +102,59 @@ public function import(Row $row, array $old_destination_id_values = array()) {
    +              $this->stubCommentedEntityType =
    +                $this->migration->source['constants'][$constant];
    

    1 line

  3. +++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityComment.php
    @@ -87,4 +102,59 @@ public function import(Row $row, array $old_destination_id_values = array()) {
    +          $result = \Drupal::entityQueryAggregate($this->stubCommentedEntityType)
    

    Can we inject this?

benjy’s picture

Status: Needs work » Needs review
FileSize
6.24 KB
4.35 KB

OK, I re-rolled this myself and made the changes I suggested in #26 because we now have a new fatal coming out of the RDF module that the tests didn't catch but this patch fixes.

New test coming soon.

benjy’s picture

FileSize
6.23 KB
1.78 KB

Fixed QueryFactory. We don't need a new test it was the same issue mentioned by dawehner relating to the RDF module, enabling that module in the test gives us an implicit test.

The last submitted patch, 27: 2340401-27.patch, failed testing.

chx’s picture

Why $this->stubCommentedEntityId and $this->stubCommentedEntityType instead of $stub_comment_entity_id. Why do we change the object state instead of just local variables?

benjy’s picture

It looks like this was done to prevent having to recalculate the entity_id for every row, eg, we only need to know one valid id for which we can use in stubbing and that doesn't change.

benjy’s picture

FileSize
5.34 KB
3.79 KB

I simplified the code a little. I removed the clever way we were determining the entity_type field and simply just presumed it was at 'constants/entity_type'. I also removed the class property so we just recalculate the value each time, it's practically free anyway.

chx’s picture

Status: Needs review » Needs work

I rarely push back on migrate patches but this is unacceptable. It flies against the migrate architecture completely: the only way data moves from the source to the destination is via the pipelines described in process. As far as I can see the stub row goes through normal process $migrate_executable->processRow($stub_row, $process); so there is no reason for this sort of violation.

benjy’s picture

Status: Needs work » Needs review
FileSize
5.16 KB
1.03 KB

I completely agree with #33, I should have spotted that myself :/

I've removed the fallback to node, I don't think we need that in core, see what the tests say.

Status: Needs review » Needs work

The last submitted patch, 34: 2340401-34.patch, failed testing.

chx’s picture

Let's put the stub entity handling in a separate method. It only needs $row anyways. Let's use a simple conditionless entity query. Despite the comment says we don't care about which entity id it is and we really don't we still pick the minimal. Let's just do the equivalent of SELECT nid FROM node LIMIT 1 and then let the database pick whatever nid it wants. I feel like stubCommentedEntityId should be stubCommentedEntityIds an array keyed by entity type. Destination plugins are very generic; just because Drupal 6 happens to only use a single entity type, and node at that, once we get to Drupal 8 sources this will be problematic.

benjy’s picture

Status: Needs work » Needs review
FileSize
5.48 KB
3.87 KB

Thanks for the feedback. New patch attached.

As far as I can see the stub row goes through normal process $migrate_executable->processRow($stub_row, $process); so there is no reason for this sort of violation.

I missed this earlier when we discussed but the stub_row only contains the source ids. See the Migration process plugin. Relevant code:

      // Only keep the process necessary to produce the destination ID.
      $process = array_intersect_key($migration->get('process'), $destination_plugin->getIds());
      $source_ids = $migration->getSourcePlugin()->getIds();
      $values = array();
      foreach (array_keys($source_ids) as $index => $source_id) {
        $values[$source_id] = $source_id_values[$migration->id()][$index];
      }
      $stub_row = new Row($values, $source_ids);
      $stub_row->stub(TRUE);
      // Do a normal migration with the stub row.
      $migrate_executable->processRow($stub_row, $process);

I've left it as $this->getDestinationProperty() so the tests fail and put this back to NW.

Status: Needs review » Needs work

The last submitted patch, 37: 2340401-37.patch, failed testing.

chx’s picture

Ah. That's a bug then. We should do $stub_row = new Row($values, $source_ids + $migration->get('source')); so that the constants are in.

benjy’s picture

Status: Needs work » Needs review
FileSize
6.5 KB
1.02 KB

Hmm, I thought it was the values and the process array that would need the values. See attached interdiff?

Also, not sure if we have an explicit test for these changes, we do have an implicit test from the EntityComment destination.

chx’s picture

benjy’s picture

Status: Needs review » Postponed
benjy’s picture

Status: Postponed » Needs work
phenaproxima’s picture

This affects the Drupal 6 and 7 upgrade path as described in #2558923: Comment migrations produce fatal errors when RDF module is enabled (which is a workaround), which is Migrate critical. Therefore I think this should be Migrate critical as well.

mikeryan’s picture

Status: Needs work » Needs review
Issue tags: +Needs reroll
FileSize
814 bytes

Rerolling, starting with a fail patch.

mikeryan’s picture

This is just a straight reroll of the last patch here, doesn't actually work at the moment.

Status: Needs review » Needs work

The last submitted patch, 45: fill_commented_entity-2340401-45-FAIL.patch, failed testing.

mikeryan’s picture

Here we go... A few notes:

  • The Entity destination base class had a processStubValues() while EntityComment had processStubRow(), both with the intent of making the stub more complete but called in different places with different signatures. Now, there's a single processStubRow() in Entity, overridden in EntityComment for comment-specific stuff, forming a pattern other modules can follow to fill in stubs.
  • processStubValues() was stripping out all non-ID values for no reason I could see - the new base processStubRow() does not do that.
  • Once I got past the initial RDF error that's caused so pain, I needed to add a little more RDF setup to the test so it would proceed silently from there.
  • created/changed also needed to be set in the stub.
mikeryan’s picture

+++ b/core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
@@ -9,8 +9,11 @@
+use Drupal\migrate\MigrateSkipRowException;

Oops, should have removed this - I switched to MigrateException instead of MigrateSkipRowException, more appropriate in the error case. I'll hold out till I see what other edits folks may have...

Also wanted to point out that, obviously, using RDF to test-by-side-effect is a little hinky. The alternative would be to add a test module implementing hook_comment_storage_load() checking for a valid entity_id - do we want to go there?

The last submitted patch, 45: fill_commented_entity-2340401-45-FAIL.patch, failed testing.

phenaproxima’s picture

Status: Needs review » Needs work

Apart from these minor quibbles, I think this looks great!

  1. +++ b/core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
    @@ -30,6 +33,20 @@ class EntityComment extends EntityContentBase {
    +   * An array of entity Ids for the 'commented entity' keyed by entity type.
    

    Nit: s/Ids/IDs

  2. +++ b/core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
    @@ -85,4 +106,38 @@ public function import(Row $row, array $old_destination_id_values = array()) {
    +      $entity_type = $this->entityManager->getDefinition($stub_commented_entity_type);
    +      if ($id_key = $entity_type->getKey('id')) {
    

    Why do we need to check if $id_key is valid? All entity types should have an ID key, so we can always assume that getKey('id') will return something.

  3. +++ b/core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
    @@ -85,4 +106,38 @@ public function import(Row $row, array $old_destination_id_values = array()) {
    +        if ($result) {
    +          $this->stubCommentedEntityIds[$stub_commented_entity_type] = array_pop($result);
    +          $row->setSourceProperty($id_key, $this->stubCommentedEntityIds[$stub_commented_entity_type]);
    +        }
    

    IMO, we should throw the MigrateException if $result is empty, rather than repeating the check of $this->stubCommentedEntityIds below.

  4. +++ b/core/modules/comment/src/Plugin/migrate/destination/EntityComment.php
    @@ -85,4 +106,38 @@ public function import(Row $row, array $old_destination_id_values = array()) {
    +      throw new MigrateException(t('Could not derive commented entity for comment %id', ['%id' => implode(':', $row->getSourceIdValues())]), MigrationInterface::MESSAGE_ERROR);
    

    Not sure about the use of the word "derive" here. Maybe "find" or "query"?

  5. +++ b/core/modules/comment/src/Tests/Migrate/d6/MigrateCommentTest.php
    @@ -19,7 +19,10 @@ class MigrateCommentTest extends MigrateDrupal6TestBase {
    +  // its hook_comment_storage_load() references $comment->getCommentedEntity().
    

    Nit: s/references/calls

  6. +++ b/core/modules/migrate/src/Plugin/migrate/destination/Entity.php
    @@ -139,21 +138,14 @@ protected function getEntityId(Row $row) {
    +    if ($bundle_key && empty($row->getDestinationProperty($bundle_key))) {
    

    Nit: Can we use $row->hasDestinationProperty() here?

mikeryan’s picture

Status: Needs work » Needs review
FileSize
7.4 KB
3.23 KB

All done, except:

Nit: s/references/calls

Changed to "references a stubbed comment.".

Nit: Can we use $row->hasDestinationProperty() here?

No, we can't, that fails. I think there is actually an empty bundle coming in, which we want to replace...

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Party on, Wayne.

  • webchick committed 24e346c on 8.0.x
    Issue #2340401 by benjy, roderik, mikeryan, dawehner, andypost, chx,...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok cool. It was explained to me that the reason we need to do this is comments are dependent on nodes, and so the stub entity needs to be complete. It looks like chx's concerns above were addressed by the latest patch, and this makes it much easier to test the migrations because you don't need to disable RDF module first.

Committed and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.