Add deserialize for JSON/AJAX

I will work on this on my train journey.

Just posting some general progress from yesterday. Please don't take it too seriously.

I have refactored the NormalizerBase class a bit, and added some conditional stuff to it do do with format, so it can be used both generically and extended for particular formats. Not sure if this is the right approach yet though.

At the moment I tihnk I've got an abstract EntityNormalizer class, not sure we will need that either.

So a bit more like this, I should have more time later to do something more concrete.

Something more like this?

This should fix those failures.

I wasn't actually using the checkFormat method :) Doh. This should now pass fine. So, from here we are in a better position to review and decide on our path.

#9: 1892320-9.patch queued for re-testing.

Rerolled after #1903784: Move serialization to own module. I will need to change the namespaces etc... will do that when I have some time shortly.

Proper re roll.

Oops missed the json ld normalizer base.

Rerolled after the unit tests and lin's patch changing the static property, as this patch made that change too.

Broke JsonldNormalizerBase class, it wants to extend the new base class I created instead.

OK, let's postpone on that one.

The main hal patch is in, so let's get this one back on the road. This is just a reroll removing the jsonld changes, and adding the changes to hal classes instead. This doesn't yet add any of the recursive stuff.

Reroll without the conversion of $formats property from static and array. I think we should consider Just casting in the base class, see NormalizerBase::checkFormat() in this patch.

So yeah, I guess we want to work out exactly what we want to include in this patch, and how far entity NG stuff actually is.

Meh, I think it makes sense to cast these values to an array. We can then support both a single class string and an array of class/interface names.

I also added some logic to deal with the bundle and some assertions to test the denormalization. I have changed the keys that get normalized to '_entity_type' and '_bundle', to me this makes sense as it signifies it is a key that is dealt with by the serializer.

What do you think?

I thought I'd go with the underscore to separate it out from regular entity props. Mainly for bundle, as the bundle key can change depending on the entity, so thought it easier to just always have a consistent _bundle in the normalized data. So thought I would change both. What do you think?

Sorry, mixed another random patch in #31, interdiff is still ok.

+++ b/core/modules/serialization/lib/Drupal/serialization/Encoder/JsonEncoder.php
@@ -7,25 +7,34 @@
+class JsonEncoder extends BaseJsonEncoder implements EncoderInterface, DecoderInterface {

It seems to make sense to have a better classname which supports both Decoding and encoding?

+++ b/core/modules/serialization/lib/Drupal/serialization/Normalizer/EntityNormalizer.php
@@ -0,0 +1,68 @@
+  /**
+     * Implements \Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize()
+     */
...
+   * Implements \Symfony\Component\Serializer\Normalizer\DenormalizerInterface::denormalize()

@inheritdoc

+++ b/core/modules/serialization/lib/Drupal/serialization/Normalizer/EntityNormalizer.php
@@ -0,0 +1,68 @@
+   * @throws \Symfony\Component\Serializer\Exception\UnexpectedValueException

Let's explain when the exception is thrown.

+++ b/core/modules/serialization/lib/Drupal/serialization/Normalizer/EntityNormalizer.php
@@ -0,0 +1,68 @@
+    if (empty($data['_bundle'])) {
+      throw new UnexpectedValueException('Entity bundle parameter must be included.');
+    }

Do you always want to require the bundle data, even if the entity type does not use bundles?

+++ b/core/modules/serialization/lib/Drupal/serialization/Normalizer/NormalizerBase.php
@@ -24,10 +23,48 @@
+    if (isset($format) && isset($this->format) && !in_array($format, (array) $this->format)) {
+      return FALSE;
+    }
+
+    return TRUE;
+   }

Just do return !(isset($format) && isset($this->format)) etc.

+++ b/core/modules/serialization/serialization.services.yml
@@ -2,6 +2,10 @@ services:
+      - { name: normalizer, priority: 1 }

Just wondering about the priority of one.

It seems to make sense to have a better classname which supports both Decoding and encoding?

The standard seems to just be to use *Encoder for this, so not sure. We would maybe have to change all other in that case.

Just wondering about the priority of one.

Yeah, prob not needed as the default order of the services in the container will be honoured.

I think I've addressed the other points. I've moved the exception for no '_bundle' key only if the entity type supports bundles. I think that is better?

Foe access controllers we use entity_type.bundle_name, what about using the same kind of pattern here as well? If the bundle_name is optional just drop it.

We discussed this issue on the REST team meeting today. The general consensus was that application/json is understood as a Drupal-specific format where you are presumed to have prior knowledge of what you'll be getting back, which means we should be using the easier-to-serialize/deserialize raw value, not _bundle.

+++ b/core/modules/serialization/lib/Drupal/serialization/Normalizer/EntityNormalizer.php
@@ -0,0 +1,74 @@
+    $attributes['_entity_type'] = $object->entityType();
+    $attributes['_bundle'] = $object->bundle();

I was on the call that #37 refers to, and agree that we shouldn't need _bundle. Also, I don't see why we need _entity_type in the data: shouldn't that be in the $context instead, when deserializing? E.g., if deserialization is triggered from rest.module, the URI defines the entity type; if it's triggered some other way, whatever that way is should know the entity type and pass that in.

Also, what is the use case we're adding deserialization of json for to begin with? Client-side apps (backbone or phone) are probably better off with HAL. On our call, we identified migrate/deploy as the most likely good use case for round tripping straight json, but for that we need #2060677: Add target_type, target_uuid to serialized output of entity reference fields in non-HAL formats.

Its a good point about the entity type, we should have that from the context. However, I think having the bundle is totally valid. You would pass that into entity_create anyway? So we should have that.

It seems totally incomplete to not be able to consume json IMO. People could use this for their own web services a lot. I don't think we need to wait for that issue here either?

However, I think having the bundle is totally valid. You would pass that into entity_create anyway?

But you don't pass _bundle into entity_create(). If you're creating a node, you call entity_create('node', array('type' => 'article', ...)) and if you're creating a taxonomy term, you call entity_create('taxonomy_term', array('vid' => 'tags', ...)). So we don't need _bundle as a special extra key in the serialized output; the 'type', 'vid', or whatever other key is the bundle key is already in the output and entity_create() knows how to deal with it.

It seems totally incomplete to not be able to consume json IMO. People could use this for their own web services a lot. I don't think we need to wait for that issue here either?

I'm ok with proceeding with this issue independantly of #2060677: Add target_type, target_uuid to serialized output of entity reference fields in non-HAL formats, just to keep the scope of each issue well focused. But I'm also curious if anyone here can think of a use case where deserializing json containing ER fields, without uuids on those fields, would ever make sense.

But you don't pass _bundle into entity_create()

Sorry, I wasn't clear before, I'm talking about changing the bundle key back to its original key, and not using _bundle at all. I know entity_create will not know what to do with _bundle :)

But I'm also curious if anyone here can think of a use case where deserializing json containing ER fields, without uuids on those fields, would ever make sense.

Why would every case have to depend on an entity reference field? I don't see how that's a requirement for serializing/unserializing stuff.

We should also consider handling the bundle property in entityNG the same as other properties. Currently, because the bundle get's passed into the entityNG constructor this has to be a raw value, and not a nested array like everything else.

Sorry, forgot to change the tests too.

@@ -47,28 +41,23 @@ public function normalize($object, $format = NULL, array $context = array()) {
+    return \Drupal::entityManager()->getStorageController($context['entity_type'])->create($data);

Don't we have the entity_manager already as local variable? Maybe it is worth to switch here to an injected version but I don't care that much.

Yep, I forgot to change that! We could go with the setter approach, as that Lin started using that in some of the hal stuff.

#45: 1892320-45.patch queued for re-testing.

I don't see the advantage of using a setter than just constructor based dependency injection ... can you please clarify why?

Errm, nope :) No reason beyond bringing it inline with what we have already.

I prefer constructor injection, If anything maybe other serializer related classes should all make sure they do this too?

You are the man of empty patches today!

HA, yes I am! I'm not sure what's happening to me...

interdiff in #49 is still good.

EDIT: Something is going wrong with file uploads. This patch I am looking at definitely has stuff in!!

@@ -0,0 +1,79 @@
+   * Implements \Symfony\Component\Serializer\Normalizer\DenormalizerInterface::denormalize()

So we use Implements if something additional is done than we can cover by @inheritdoc?

@@ -0,0 +1,79 @@
+      unset($data[$bundle_key]);
+      $data[$bundle_key] = $type;

Is there a reason why we can't just do $data[$bundle_key] = $type and skip the unsetting?

@@ -24,10 +23,44 @@
+   * This class doesn't implement DenormalizerInterface, but most of its child
+   * classes do, so this method is implemented at this level to reduce code
+   * duplication.

That is kind of odd :(

So we use Implements if something additional is done than we can cover by @inheritdoc?

Yes, at the moment the comment is like this for the @throws part.

That is kind of odd :(

Yes, I guess it is a bit but I'm not sure where else this could live.

Sorry Lin, I think I totally missed your review there, I think I posted the patch before updating this issue.

This is a rather long and complicated conditional. Can we possibly move the checks to variables that communicate what they are checking?

Fair point, there was some eye bleeding stuff in there; I have refactored checkFormat() to be much easier to read, as that was doing some double false positive logic, which is not easy for anyone to read. I have also moved the callables that were being used in the array_filter checks to variables. This should make it both easier to read and self documents what the check is.

Also, if we're switching to add an EntityNormalizer, we should remove whichever normalizer was handling things at the entity level before. Otherwise we create too many potential code paths.

Before it was using the ComplexDataNormalizer, which I think we should keep really?

Rolled and made a few changes to the logic in the NormalizerBase class.

1. +++ b/core/modules/serialization/lib/Drupal/serialization/Normalizer/EntityNormalizer.php
   @@ -0,0 +1,78 @@
   +   * Implements \Symfony\Component\Serializer\Normalizer\DenormalizerInterface::denormalize()
   
   +++ b/core/modules/serialization/lib/Drupal/serialization/Normalizer/NormalizerBase.php
   @@ -24,10 +23,62 @@
   +   * Implements \Symfony\Component\Serializer\Normalizer\DenormalizerInterface::supportsDenormalization()
   

   What about using @inheritdoc here?

2. +++ b/core/modules/serialization/lib/Drupal/serialization/Normalizer/NormalizerBase.php
   @@ -24,10 +23,62 @@
   +    $reflection = new \ReflectionClass($type);
   ...
   +      return (class_exists($name) || interface_exists($name)) && $reflection->isSubclassOf($name);
   

   I am wondering whether is_subclass_of just works here as well.

3. +++ b/core/modules/serialization/serialization.services.yml
   @@ -2,6 +2,11 @@ services:
   +    arguments: ['@plugin.manager.entity']
   

   We switched to entity.manager quite a while ago.

Thank you! Also, the test was failing as the old patch still had EntityNG, which doesn't exist anymore :)

+++ b/core/modules/serialization/lib/Drupal/serialization/Normalizer/NormalizerBase.php
@@ -19,15 +18,65 @@
+  /**
+   * Implements \Symfony\Component\Serializer\Normalizer\DenormalizerInterface::supportsDenormalization()
+   *
+   * This class doesn't implement DenormalizerInterface, but most of its child
+   * classes do, so this method is implemented at this level to reduce code
+   * duplication.
+   */

That one is still in.

I think we want to keep that comment tbh.

+++ b/core/modules/serialization/lib/Drupal/serialization/Normalizer/EntityNormalizer.php
@@ -0,0 +1,75 @@
+    // The bundle property behaves differently from other entity properties.
+    // i.e. the nested structure with a 'value' key does not work.
+    if (!empty($definition['entity_keys']['bundle'])) {
+      $bundle_key = $definition['entity_keys']['bundle'];
+      $type = $data[$bundle_key][0]['value'];
+      $data[$bundle_key] = $type;
+    }
+

Are we doing this in multiple places?

Seems like create()/the upcoming entity factory should better support this by default and not require custom code here.

Also, there's a third option to consider, another level, keyed by the language code, which is how the values are coming from the database for example.

That again reminds me, how is serialization + entity translations going to work? Seems like everything should either be for a specific language and define that (then it needs to handle per-language changes correctly) or return/accept all languages somehow..

Are we doing this in multiple places?

I think just here. The entity dernormalization in hal module does something a bit different. If this gets fixed in entities in general, happy to change this too :)

Also, regarding the translations, this is also down to the entities, as we are just creating instances with the data. Do these need separate issues?

This issue is still good to go IMO.

Lin, I think the ComplexDataNormalizer will serve as a good base implementation anyway? But this is used by content entities and field items, so most things are covered I guess.

I vote it stays :)

60: 1892320-60.patch queued for re-testing.

60: 1892320-60.patch queued for re-testing.

Rerolled and made the EntityType class changes.

Sorry for my review.

1. +++ b/core/modules/serialization/lib/Drupal/serialization/Normalizer/EntityNormalizer.php
   @@ -0,0 +1,75 @@
   +   * @var \Drupal\Core\Entity\EntityManager
   ...
   +   * @param \Drupal\Core\Entity\EntityManager $entity_manager
   ...
   +  public function __construct(EntityManager $entity_manager) {
   

   We do have interfaces for that.

2. +++ b/core/modules/serialization/lib/Drupal/serialization/Normalizer/EntityNormalizer.php
   @@ -0,0 +1,75 @@
   +  protected $supportedInterfaceOrClass = array('Drupal\Core\Entity\Entity');
   

   Are you sure we should not use some interfaces?

Yes, and yes!

Are you sure we should not use some interfaces?

So could we?

Yes we can :) I'll reroll in the morning.

Sorry, forgot about this one!

Thank you!

bumping to major, this is incomplete functionality. As well as people building stuff on REST, consuming JSON will probably be the most common implementation of that.

bumping to major, this is incomplete functionality.

As well as people building stuff on REST; consuming JSON will probably be the most common implementation of that.

Agreed with this being major. It'd also be great to have in the next alpha release, so folks can continue testing the new REST functionality in Drupal 8.

I looked through and couldn't see anything to complain about. Stuff is tested, documented, etc. Great!

Committed and pushed to 8.x. Thanks!

Whenever you try to deserialize a ContentEntity, you get this error:

exception 'Drupal\Core\Entity\EntityStorageException' with message 'Missing bundle for entity type node' in core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:63

The code responsible for that error is here:

// 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;
}

Since the code assumes that the bundle is in value key rather than the target_id key a ContentEntity cannot be deserialized.

I've opened #2443165: Drupal\Core\Entity\EntityInterface\ContentEntityStorageBase::doCreate() assumes that the bundle is a string to fix the problem. :)