Problem/Motivation

This was found in #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field.

If a migration source references an entity ID that does not exist, an appropriate entity will be stubbed. If that entity is a content entity with a required field with a default value, the stubbed entity will not have that default value applied to the field.

That is because the default value is incorrectly nested in another array, so during entity-saving the value ultimately gets lost.

Proposed resolution

Remove the unnecessary and incorrect nesting.

CommentFileSizeAuthor
#9 2923915-9.patch4.78 KBtstoeckler
#9 2923915-5-9-interdiff.txt3.34 KBtstoeckler
#5 2923915-3.patch3.68 KBtstoeckler
#5 2923915-3--tests-only.patch2.58 KBtstoeckler
#5 2923915-interdiff-2-3.txt1.12 KBtstoeckler
#2 2923915-2.patch3.41 KBtstoeckler
#2 2923915-2--tests-only.patch2.58 KBtstoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented
Status: Active » Needs review
FileSize
2923915-2--tests-only.patch2.58 KB
2923915-2.patch3.41 KB

Here we go.

The last submitted patch, 2: 2923915-2--tests-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 2: 2923915-2.patch, failed testing. View results

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented
Status: Needs work » Needs review
FileSize
2923915-interdiff-2-3.txt1.12 KB
2923915-3--tests-only.patch2.58 KB
2923915-3.patch3.68 KB

Self-review:

  1. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentBaseTest.php
@@ -265,4 +269,21 @@ public function testEmptyDestinations() {
+    foreach (EntityTestMul::loadMultiple($ids) as $entity) {

    Seems kind of pointless to go into a loop here, EntityTestMul::load(reset($ids)) would make more sense. 

  2. +++ b/core/modules/migrate/tests/src/Kernel/MigrateEntityContentBaseTest.php
@@ -265,4 +269,21 @@ public function testEmptyDestinations() {
+      $this->assertSame('this is a default value', $entity->get('required_default_field')->value);

    For extra credit, fetch the default value from the field definition.

Fixed the above and fixed up the test a bit more, as well.

EDIT: Apparently this does not apply to 8.4.x. But attaching 8.5.x patches for now. Will reroll for both branches when this is RTBC.

The last submitted patch, 5: 2923915-3--tests-only.patch, failed testing. View results

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
Assigned: Unassigned » heddn

Assigning to myself to review for the week up coming.

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented
Assigned: heddn » Unassigned
Status: Needs review » Needs work 
+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
@@ -218,7 +218,7 @@ protected function processStubRow(Row $row) {
+          $values = $default_value;

Can we add a test for a field with a cardinality of > 1? I'd like to see what happens when we provide multiple default values. Does it need to be an array then?

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler at bio.logis Genetic Information Management GmbH commented
Version: 8.4.x-dev » 8.5.x-dev
Status: Needs work » Needs review
FileSize
2923915-5-9-interdiff.txt3.34 KB
2923915-9.patch4.78 KB

Sure, no problem. Added another test. Actually it will work just fine with multiple values as well, so the test change is the only needed change.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Looks good to me :)

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC, Acro Commerce commented

+1 from me too. Thanks for the added test coverage.

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.

  • catch committed 9714796 on 8.6.x 
    Issue #2923915 by tstoeckler, heddn: Stubbing content entities with...

  • catch committed 8023a1c on 8.5.x 
    Issue #2923915 by tstoeckler, heddn: Stubbing content entities with...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
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!

Status: Fixed » Closed (fixed)

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