Migrating more than 1 entity reference field instance fails

Test only so setting back to needs work.

Pls make sure to move status to needs review after patch

Reroll and update of the patch in #19. The patch in #19 added a new test, which has been removed and the assertions moved to the relevant existing tests, MigrateField, MigrateFieldInstance and MigrateNode.

Previous patch no longer to applied

So I have rerolled previous patch. Kindly check it.

Waiting for test cases. I am working for test cases.

@durgeshs, thanks for the reroll. There is no need to output the results of a git pull and git apply, we can see from the test run that the patch does not apply. To aid the reviewer please provide at least an interdiff or if that fails a diff, See Creating an interdiff in the handbook.

@quietone,

I have added interdiff and new patch with resolve test case under core/modules/field/tests/src/Kernel/Migrate/d7.

Found two out of scope changes that were introduced by me earlier on.

1. +++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceTest.php
   @@ -51,7 +51,7 @@ protected function setUp(): void {
   +    [$expected_entity_type, $expected_bundle, $expected_name] = explode('.', $id);
   

   out of scope

2. +++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldTest.php
   @@ -53,7 +53,7 @@ protected function setUp(): void {
   +    [$expected_entity_type, $expected_name] = explode('.', $id);
   

   Out of scope

3. +++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldTest.php
   @@ -102,7 +102,7 @@
   +    $this->assertEntity('node.field_reference_3', 'entity_reference', TRUE, 1);
   

   Good. I checked the field definition and it does have a cardinality of 1.

4. +++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceTest.php
   @@ -211,7 +212,7 @@ public function testTextFieldInstances() {
   +    $this->assertCount(9, $errors);
   

   Why has the error count increased? Is is supposed to? What is the error message?

@quietone,

I have done changes as per #37, kindly review it.

@durgeshs, thanks for fixing the out of scope changes I introduced.

+++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceTest.php
@@ -226,6 +226,8 @@
+    $message = 'New object was not saved, no error provided';

This will allow the test to pass but I don't think the test itself is correct. This issue is to migrate more that 1 entity reference field so getting a new errors seems wrong. The changes here that allow the test to pass need to be explained.

Settings to NW for #37.4 and the above change to the test.

@quietone, Thanks for feedback.

Sure, I will check and resolve it.

@quietone,

I have checked code and found that there are getting one extra message when migrate more than 1 entity reference field. This message occurring because of the following code
import(Row $row, array $destination_id_values),
$destination_id_values is getting blank array for first entity reference filed.
Please feel free to let me know if there are any best solution.

Meanwhile in the current patch, I have added a proper comment for this.

@durgeshs, Yes, that is not unexpected, it will be failing somewhere in the import process. Just have to find out where. Maybe I can do some debugging on this in the next couple of days.

+++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceTest.php
@@ -211,7 +212,7 @@ public function testTextFieldInstances() {
+    $this->assertCount(9, $errors);

@@ -225,6 +226,11 @@ public function testTextFieldInstances() {
+    // Due to migrate more than 1 entity reference field so getting a new errors
+    // message. Because of in the import(Row $row, array $destination_id_values) ¶
+    // $destination_id_values is getting blank array for first entity.
+    $message = 'New object was not saved, no error provided';
+    $this->assertEquals($errors[8], $message);

The comment before the tests on the error messages states that it is for text field instances not entity reference field. So, these changes need to be removed. Sorry about that.

+++ b/core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceTest.php
@@ -106,6 +106,7 @@ public function testFieldInstances() {
+    $this->assertEntity('node.article.field_reference_3', 'Reference 3', 'entity_reference', FALSE, TRUE);

The fixture add two instances of the field_reference_2 and we are only checking that one on the article is migrated. A similar test needs to be added for the forum bundle. Please add it a few lines down where the other 'forum' bundle field instances are tested.

@quietone, Thanks for the feedback, I have addressed those two comments of #43 and will be waiting for your debugging input.

I manually tested today and wasn't able to produce an error. I followed the steps 1, 2 and 3 in the ISS then I used the UI to edit node 2 and add a reference. There were no errors in the log.

@mradcliffe, is this still a problem?

Is this issue still a problem?

There has been no activity here for 7 months.

Since we need more information to move forward with this issue, I am keeping the status at Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Based on #45, it looks as though this issue was solved by some other changes in Drupal.

I tried to confirm this. I applied the test-only patch from #18 to commit 45b25f08b7 (2019-12-13). I installed PHP 7.2 and did composer install (with Composer 1.10.13) to get PHPUnit 6.5.14. After this experiment in time travel, I got the same test failure reported by the testbot in #18: "Failed asserting that an array has the key 'field_reference_3'."

Then, with some difficulty, I rerolled the test-only patch so that it applies to 9.3.x. With PHP 7.3 and PHPUnit 8.5.15, I get the same failure.

Part of the reroll is removing the reference to NodeCommentCombinationTrait, which was removed in #3111344: Remove deprecated code from migrate module. The test-only patch did not use the method defined in that trait, so no related changes were needed.

There are a lot of things I have not done: I have not looked at the test to decide what it does. Maybe it is bogus. Maybe it is not testing what the issue summary says. I have not compared the failing test to the tests added by @quietone in #28 and later patches. I have not done any manual testing.

I did check that the patch in #18 is a test-only version of the patch in #19.

I am removing an unhelpful screenshot from the issue summary.

I think we have a failing test, so I am setting the status back to NW.

P.S. It is also quite possible that I made a mistake while rerolling the patch. But then it would be quite a coincidence that I get the same test failure.

Rerolled #43 test-only patch