Drupal\Core\Entity\EntityInterface\ContentEntityStorageBase::doCreate() assumes that the bundle is a string

What exactly did you do?

Blind guess is something with the bundle is not right.

Everything is a field.

Yes, I'm pretty sure that it's the bundle detection that is flawed. ContentEntityStorageBase::doCreate() assumes that it's just a string. We can try to make that code more generic, but the easiest solution for you is probably just to special case that.

if ($node->getEntityType()->hasKey('bundle')) {
  $data[$node->getEntityType()->getKey('bundle')] = $node->bundle();
}

Untested, but that should do the trick and is generic.

That's not generic enough, the bundle is not always in a target_id property. EntityTest for example doesn't use a reference field, so it is in 'value'.

The best way might be to just reset() in a loop until we no longer have an array.

We also need tests.

Btw, your code is flawed for the same reason. Mine works no matter how the bundle is stored, because it uses the API.

Also, you could also use the serialization module for this instead of your own implementation. See \Drupal\hal\Tests\EntityTest for examples. That allows entity and field types to customize the process.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -62,7 +62,14 @@ protected function doCreate(array $values) {
   +      if (is_array($bundle)) {
   +        do {
   +          $bundle = reset($bundle);
   +        } while (is_array($bundle));
   +      }
   

   That's a creative way of writing a normal while loop as a do/while.. :)

   We also need a comment here, something like this:

   The bundle could be a string or it might a field based structure. We can rely on it being a single string, so remove all wrapping arrays until we have that string.

   Yes, I I think we need explicit tests for this.

2. +++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
   @@ -50,14 +50,6 @@ public function denormalize($data, $class, $format = NULL, array $context = arra
   -    // The bundle property behaves differently from other entity properties.
   -    // i.e. the nested structure with a 'value' key does not work.
   -    if ($entity_type->hasKey('bundle')) {
   -      $bundle_key = $entity_type->getKey('bundle');
   -      $type = $data[$bundle_key][0]['value'];
   -      $data[$bundle_key] = $type;
   -    }
   -
   

   Grml. json serialization is crap.

   I recommend to use hal+json from the hal.module instead. Doesn't mean that we shouldn't fix this, of course.

Rerolled, fixed the comment wrapping and adjusted the wording a bit.

Other than that - a bit ugly indeed, but no better suggestion on my side :-/

Patch no longer applies.

Rerolled patch #23

+++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
@@ -50,14 +50,6 @@ public function denormalize($data, $class, $format = NULL, array $context = arra
-    // The bundle property behaves differently from other entity properties.
-    // i.e. the nested structure with a 'value' key does not work.
-    if ($entity_type->hasKey('bundle')) {
-      $bundle_key = $entity_type->getKey('bundle');
-      $type = $data[$bundle_key][0]['value'];
-      $data[$bundle_key] = $type;
-    }

lost in re-roll

FWIW, I don't think this is a particularly good idea. If we want to support multiple possible inputs we should do so more explicitly. The current patch allows, for example Node::create('type' => [[[[[[[[[['article']]]]]]]]]]). These are the sort of things that - in my opinion - come to bite us years from now because this will hide some other obscure bug that would generate obscenely nested arrays.

Edit: fix example code

OK, sorry, maybe that was unclear.

I'm totally fine with supporting all of the below:

$property_value = 'article';
Node::create('type' => $property_value);
$field_item_value = ['value' => 'article'];
Node::create('type' => $field_item_value);
$field_item_list = [['value' => 'article']];
Node::create('type' => $field_item_list);

which - unless I am mistaken - is all the variants we support for all other fields, as well. But we IMO we should support those explicitly and not arbitrarily deeply nested arrays, which is what the current patch does.

But of course me having that opinion does not mean that it has to done that way :-)

If you click on the test results->view results on dispatcher->console log->console log full, there you will see the culprit(s):
https://dispatcher.drupalci.org/job/default/84494/consoleFull

00:17:26.259 FATAL Drupal\Tests\Core\Entity\ContentEntityStorageBaseTest: test runner returned a non-zero error code (2).

00:17:46.192 PHP Fatal error:  Call to a member function create() on a non-object in /var/www/html/core/modules/serialization/src/Normalizer/EntityNormalizer.php on line 88
00:17:46.209 Drupal\Tests\serialization\Unit\Normalizer\EntityNormalizerT   0 passes   1 fails      

00:17:46.561 FATAL Drupal\Tests\serialization\Unit\Normalizer\EntityNormalizerTest: test runner returned a non-zero error code (255).
00:17:46.563 Drupal\Tests\serialization\Unit\Normalizer\EntityNormalizerT   0 passes   1 fails     

Those are all essentially segmentation faults or something like it. Whatever you are doing in that test, its causing phpunit to crash, hard.

Sorry that the error is "PHPunit Test failed to complete " -> thats actually in core and is what the testbots tell you: http://cgit.drupalcode.org/drupal/tree/core/modules/simpletest/simpletes...

The core committers and Field API maintainers discussed this issue today. We decided to defer triage since we did not really have consensus on whether the issue was a bug vs. a task and whether it was major vs. normal. Both Entity system maintainers considered it a bug despite being developer-facing only, and emphasized that it was a very common usecase for the API, so leaving at major to revisit later.

This once fixed a save error in Paragraphs module which I can't reproduce at the moment (will post if it comes up again). Must have been a very specific case. I can't apply the patch to Drupal 8.3 so could anybody clarify what's the status of this? Is it somehow fixed from. How serious is this bug? Thank you!

I just bumped into this problem while working on another patch :/ It would be nice to have it fixed, major or not.

Here's a new patch, written from scratch to take into account @tstoeckler's feedback from #37.

Looks good to go

1.   /**
      * @covers ::create
      */
     public function testReCreate() {
       $storage = $this->container->get('entity_type.manager')->getStorage('entity_test');
   
       $values =  $storage->create(['type' => 'test_bundle'])->toArray();
       $entity = $storage->create($values);
       $this->assertEquals('test_bundle', $entity->bundle());
     }
   

   I think it is worthwhile to have a test something like the above to prove that we don't regress the reported bug in some other way.

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -85,7 +85,22 @@ protected function doCreate(array $values) {
   +      // Normalize the bundle value.
   +      $bundle_value = $values[$this->bundleKey];
   +      if (!is_array($bundle_value)) {
   +        // The bundle value is a scalar, use it as-is.
   +        $bundle = $bundle_value;
   +      }
   +      elseif (is_numeric(array_keys($bundle_value)[0])) {
   +        // The bundle value is a field item list array, keyed by delta.
   +        $bundle = reset($bundle_value[0]);
   +      }
   +      else {
   +        // The bundle value is a field item array, keyed by the field's main
   +        // property name.
   +        $bundle = reset($bundle_value);
   +      }
   

   Is it worth having a static method that does this in a more generic way. I.e. do we do something similar anywhere else?

Thanks @alexpott for the review :)

Re #50:

1. That's a very good idea, added the test to the patch.

2. We have \Drupal\Core\Field\FieldInputValueNormalizerTrait::normalizeValue() with a purpose similar to the code from this patch, but that helper does a lot more work than we need because it is normalizing a field value in an array of field properties keyed by delta, and we just need the scalar value instead. The code here is also more performant because it doesn't require any knowledge about the definition of the bundle field in order to get its main property name.

However, to answer your initial question, FieldInputValueNormalizerTrait::normalizeValue() is the method that should be used in most cases.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -85,7 +85,22 @@ protected function doCreate(array $values) {
+      // Normalize the bundle value.

Let's add a code comment here to point out this is an optimised version of \Drupal\Core\Field\FieldInputValueNormalizerTrait::normalizeValue() so people don't do copy pasta unnecessarily.

Sure thing, expanded the comment :)

Committed and pushed c594d13e25 to 8.7.x and 44c3648423 to 8.6.x. Thanks!

Backported to 8.6.x because this is a bugfix that does not have an API impact. The original code path of providing a scalar value is unaffected.