Entity denormalization fails to retrieve bundle

After investigating a bit this appears to only be occurring with 'json' serialisation and not 'hal_json'. Patch with a test and a bit more tidy up.

The test looks good and just one more minor nit with the code...

+++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
@@ -43,22 +44,34 @@ public function __construct(EntityManagerInterface $entity_manager) {
+    if ($entity_type instanceof EntityTypeInterface && $entity_type->hasKey('bundle')) {

The instanceof check here is superfluous because we've just checked it above.

The only problem now is the _restSubmittedFields property.

I think this is good to go and we have the followup #2456257: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work to try to remove _restSubmittedFields which is not the fault of this patch. I can't rtbc since I contributed to the patch.

1. +++ b/core/modules/rest/src/Tests/NodeTest.php
   @@ -90,4 +90,40 @@ public function testNodes() {
   +    $this->assertEqual($node->title->value, $title);
   

   We should check the type here too.

2. +++ b/core/modules/rest/src/Tests/NodeTest.php
   @@ -90,4 +90,40 @@ public function testNodes() {
   +    $this->assertHeader('Location', $node->url('canonical', ['absolute' => TRUE]));
   

   This seems like odd behaviour - not a problem with this patch though.

3. +++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
   @@ -43,22 +44,34 @@ public function __construct(EntityManagerInterface $entity_manager) {
   +    if (!$entity_type instanceof EntityTypeInterface) {
   

   I don't see a need to check the interface here, EntityManager::getDefinition won't return random objects. So this could just check emptiness? However, we currently won't get here anyway as EntityManager::getDefinition() will throw a PluginNotFoundException if the type is not found before here. Unless we add FALSE for the second ($exception_on_invalid) parameter.

4. +++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
   @@ -43,22 +44,34 @@ public function __construct(EntityManagerInterface $entity_manager) {
   +      $data[$bundle_key] = !empty($data[$bundle_key][0]['target_id']) ? $data[$bundle_key][0]['target_id'] : $data[$bundle_key][0]['value'];
   

   This still seems kind of fragile to me, we should be able to have a solution using FieldItemInterface::mainPropertyName()?

5. +++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
   @@ -43,22 +44,34 @@ public function __construct(EntityManagerInterface $entity_manager) {
   +    $entity->_restSubmittedFields = array_keys($data);
   

   If we are keeping this as part of this issue (sigh) then we at least need a @todo linking to the other issue :)

We also need to add to/amend the test coverage in EntityNormalizerTest probably?

Just took a look at the newer approach in #2456257: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work, no more _restSubmittedFields - I love it!

+++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
@@ -43,22 +43,45 @@ public function __construct(EntityManagerInterface $entity_manager) {
+      if (!is_string($data[$bundle_key])) {
+        throw new UnexpectedValueException('A valid bundle is required for denormalization.');
+      }

Is this testable?

Patch fixes @todo.

We can test that in the unit tests, which need to be amended and extended anyway I think.

1. +++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
   @@ -61,17 +60,23 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
   -      $data[$bundle_key] = $data[$bundle_key][0][$key_id] ?: $data[$bundle_key];
   

   That would throw a notice.

2. +++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
   @@ -61,17 +60,23 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
   +      if (!is_string($data[$bundle_key]) || !in_array($data[$bundle_key], $bundle_types)) {
   

   Added checking against available bundles too. That also makes the exception message make a bit more sense.

+++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
@@ -72,10 +72,11 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
+      $bundle_types = ($bundle_entity_type !== 'bundle') ? $this->entityManager->getStorage($bundle_entity_type)->getQuery()->execute() : [];

Core is doing that in other places too. Having the bundle entity type defaulting to 'bundle' seems....silly.

Very. good thing we have #2346857: Set default property bundle_entity_type in EntityType to NULL :)

YES! That is very good, thanks Berdir!

We can wait on that one now I think and tidy that part I don't like up a bit. Also,not being able to deserialze entities is bigger than 'normal'.

1. +++ b/core/modules/rest/src/Tests/NodeTest.php
   @@ -42,7 +42,13 @@ protected function enableNodeConfiguration($method, $operation) {
   +   * Serializes and post a node to create it.
   

   This line needs a tweak.

2. +++ b/core/modules/rest/src/Tests/NodeTest.php
   @@ -50,9 +56,15 @@ protected function postNode($data) {
   +   * Testes the title on a newly created node.
   

   Tests*

Otherwise, those changes look ok to me.

+++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
@@ -43,22 +43,55 @@ public function __construct(EntityManagerInterface $entity_manager) {
+      $bundle_types = ($bundle_entity_type !== 'bundle') ? $this->entityManager->getStorage($bundle_entity_type)->getQuery()->execute() : [];

We can now simplify this again based on #2346857: Set default property bundle_entity_type in EntityType to NULL

1. +++ b/core/modules/rest/src/Tests/NodeTest.php
   @@ -90,4 +124,80 @@ public function testNodes() {
   +    // testing
   +    $this->postNode($data);
   

   That comment is a bit pointless

2. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -388,4 +394,27 @@ protected function removeNodeFieldsForNonAdminUsers(NodeInterface $node) {
   +   * Check to see if the http request response body is identical to the expected
   

   Nitpick: HTTP instead of http

3. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -388,4 +394,27 @@ protected function removeNodeFieldsForNonAdminUsers(NodeInterface $node) {
   +    return $this->assertIdentical($expected, $this->responseBody, $message ? $message : SafeMarkup::format('Response body @expected (expected) is equal to @response (actual).', array('@expected' => var_export($expected, TRUE), '@response' => var_export($this->responseBody, TRUE))), $group);
   

   Let's not introduce a new Safemarkup::format call but use strtr instead

4. +++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
   @@ -43,22 +43,54 @@ public function __construct(EntityManagerInterface $entity_manager) {
   +      throw new UnexpectedValueException('A valid entity type is required for denormalization.');
   

   I'm curious whether we should be more explicit, so something like: The specified entity type $entity_type does not exist, but is required for denormalization.

5. +++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
   @@ -43,22 +43,54 @@ public function __construct(EntityManagerInterface $entity_manager) {
   +      $key_id = 'value';
   +      // Get the ID key from the base field definition for the bundle key.
   +      if (isset($base_field_definitions[$bundle_key])) {
   +        $key_id = $base_field_definitions[$bundle_key]->getFieldStorageDefinition()->getMainPropertyName();
   +      }
   

   If we want, we could move that to a ternary as well

6. +++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
   @@ -43,22 +43,54 @@ public function __construct(EntityManagerInterface $entity_manager) {
   +      $data[$bundle_key] = isset($data[$bundle_key][0][$key_id])
   +        ? $data[$bundle_key][0][$key_id]
   +        : (isset($data[$bundle_key]) ? $data[$bundle_key] : NULL);
   

   It is a bit odd to break this ternary into multiple lines

7. +++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
   @@ -43,22 +43,54 @@ public function __construct(EntityManagerInterface $entity_manager) {
   +      if (!is_string($data[$bundle_key]) || ($bundle_types && !in_array($data[$bundle_key], $bundle_types))) {
   +        throw new UnexpectedValueException(sprintf('"%s" is not a valid bundle type for denormalization.', $data[$bundle_key]));
   

   Do you mind splitting up the exception into one which makes it clear that the data does not contain bundle information + one that the specified bundle type does not exist?

Thanks Daniel, here are some changes based on those comments.

Thank you @Jaesin and @damiankloip

The patch has changed a lot since my last involvement. Committed fe81cfa and pushed to 8.0.x. Thanks!

diff --git a/core/modules/rest/src/Tests/RESTTestBase.php b/core/modules/rest/src/Tests/RESTTestBase.php
index 6a461ab..5e84827 100644
--- a/core/modules/rest/src/Tests/RESTTestBase.php
+++ b/core/modules/rest/src/Tests/RESTTestBase.php
@@ -411,7 +411,7 @@ protected function removeNodeFieldsForNonAdminUsers(NodeInterface $node) {
    *   translate this string. Defaults to 'Other'; most tests do not override
    *   this default.
    *
-   * @return
+   * @return bool
    *   TRUE if the assertion succeeded, FALSE otherwise.
    */
   protected function assertResponseBody($expected, $message = '', $group = 'REST Response') {

Fixed on commit.