Now that we have serializer in core, and being used for JSON-LD, it makes sense to provide support for JSON and XML too. I think all we should need to do is register the encoders and normalizers for each in the container. I have included a very basic generic normalizer that returns the getProperties() method from an entityNG class. We then get support for these formats anytime we use serializer, such as : #1819760: Add a REST export display plugin and serializer integration., which uses a generic serializer plugin.

Initial patch attached.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Status: Needs review » Needs work

Rather than having separate Normalizers for xml and json, we could simply have one Normalizer class that does not check the format. If someone wants to provide a format specific normalizer that overrides the default one (as JSON-LD would, for example), that format specific normalizer would simply be given a higher priority when registering.

dawehner’s picture

Status: Needs work » Needs review
+++ b/core/modules/system/lib/Drupal/system/JsonNormalizer.phpundefined
@@ -0,0 +1,23 @@
+ * Converts the Drupal entity object structure to a normalized array form for
+ * JSON.
...
+class JsonNormalizer extends NormalizerBase {

Shouldn't we just talk here about this is the common normalizer outputted as json?

+++ b/core/modules/system/lib/Drupal/system/NormalizerBase.phpundefined
@@ -0,0 +1,54 @@
+use Drupal\Core\Entity\EntityNG;
...
+abstract class NormalizerBase implements NormalizerInterface {

Maybe a dump question, but you talk about the entity object but the name of the class is really generic. Maybe EntityNormalizerBase makes more sense?

+++ b/core/modules/system/lib/Drupal/system/NormalizerBase.phpundefined
@@ -0,0 +1,54 @@
+   * @param object $object
+   *   Object to normalize.

If we are in the context of entity, can't we document at least that here is an entity?

+++ b/core/modules/system/lib/Drupal/system/NormalizerBase.phpundefined
@@ -0,0 +1,54 @@
+   * @param mixed  $data

Nitpick: There seems to be an extra space before $data

+++ b/core/modules/system/lib/Drupal/system/SystemBundle.phpundefined
@@ -0,0 +1,30 @@
+/**
+ * System bundle container.
+ */

Funny enough the adapted standard to document the bundle seems to be "/**
* $module dependency injection container.
*/" but that sounds like a bug.

+++ b/core/modules/system/lib/Drupal/system/SystemBundle.phpundefined
@@ -0,0 +1,30 @@
+    $options = array('priority' => 5);

If we change the priority giving the reason seems to be helpful. Just from that i wouldn't have any clue why it's set to 5.

+++ b/core/modules/system/lib/Drupal/system/SystemBundle.phpundefined
--- /dev/null
+++ b/core/modules/system/lib/Drupal/system/XmlNormalizer.phpundefined

Yeah we don't use uppercase in classnames for Json, any special reason we don't do?

--- /dev/null
+++ b/core/modules/system/lib/Drupal/system/JsonNormalizer.phpundefined

A silly question maybe: Is there a reason why to put this into system module and not core/lib?

+++ b/core/modules/system/lib/Drupal/system/JsonNormalizer.phpundefined
@@ -0,0 +1,23 @@
+ * Converts the Drupal entity object structure to a normalized array form for
+ * JSON.
...
+class JsonNormalizer extends NormalizerBase {

Shouldn't we just talk here about this is the common normalizer outputted as json?

+++ b/core/modules/system/lib/Drupal/system/NormalizerBase.phpundefined
@@ -0,0 +1,54 @@
+use Drupal\Core\Entity\EntityNG;
...
+abstract class NormalizerBase implements NormalizerInterface {

Maybe a dump question, but you talk about the entity object but the name of the class is really generic. Maybe EntityNormalizerBase makes more sense?

+++ b/core/modules/system/lib/Drupal/system/NormalizerBase.phpundefined
@@ -0,0 +1,54 @@
+   * @param object $object
+   *   Object to normalize.

If we are in the context of entity, can't we document at least that here is an entity?

+++ b/core/modules/system/lib/Drupal/system/NormalizerBase.phpundefined
@@ -0,0 +1,54 @@
+   * @param mixed  $data

Nitpick: There seems to be an extra space before $data

+++ b/core/modules/system/lib/Drupal/system/SystemBundle.phpundefined
@@ -0,0 +1,30 @@
+/**
+ * System bundle container.
+ */

Funny enough the adapted standard to document the bundle seems to be "/**
* $module dependency injection container.
*/" but that sounds like a bug.

+++ b/core/modules/system/lib/Drupal/system/SystemBundle.phpundefined
@@ -0,0 +1,30 @@
+    $options = array('priority' => 5);

If we change the priority giving the reason seems to be helpful. Just from that i wouldn't have any clue why it's set to 5.

+++ b/core/modules/system/lib/Drupal/system/SystemBundle.phpundefined
--- /dev/null
+++ b/core/modules/system/lib/Drupal/system/XmlNormalizer.phpundefined

Yeah we don't use uppercase in classnames for Json, any special reason we don't do?

damiankloip’s picture

FileSize
2.58 KB

$module dependency injection container.

Yeah that seems wrong to me too, That's why I changed it :) But then I did see that it is doing it in other places. It's not really a DIC, it's just adding to it, right?

I've removed the JSON and XML normalizers, so we just have EntityNormalizer. So I guess we should move this to entity namespace too? and maybe register these in the CoreBundle class, along with the actual serializer?

So, something more like this? It's getting smaller, which I like.

damiankloip’s picture

Issue tags: +Web services

I think this will affect WS too.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityNormalizer.phpundefined
@@ -0,0 +1,50 @@
+ * Definition of Drupal\Core\Entity\EntityNormalizer.

We are now into the "Contains \$class" business.

+++ b/core/lib/Drupal/Core/Entity/EntityNormalizer.phpundefined
@@ -0,0 +1,50 @@
+  /**
+   * Normalizes an object into a set of arrays/scalars.

What about an /** Implements \...NormalizerInterface::normalize() and keep the rest of the documentation, so we know it will be always an entity here?

+++ b/core/lib/Drupal/Core/Entity/EntityNormalizer.phpundefined
@@ -0,0 +1,50 @@
+   * Checks whether the data and format are supported by this normalizer.
...
+  public function supportsNormalization($data, $format = NULL) {

We could probably just do an "Implements" as well. Yeah even less documentation.

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -154,6 +154,11 @@ public function build(ContainerBuilder $container) {
+    $container->register('serializer.normalizer.xml', 'Drupal\Core\Entity\EntityNormalizer')->addTag('normalizer');
+    $container->register('serializer.encoder.xml', 'Symfony\Component\Serializer\Encoder\XmlEncoder')->addTag('encoder');

It's not a problem to register them here, as it will be built into the container, so it is blazing fast.

+++ b/core/lib/Drupal/Core/Entity/EntityNormalizer.phpundefined
@@ -0,0 +1,50 @@
+ * The format is not checked here, but assumed it will be supported.
+ */

What about an @see for the entity interface?

damiankloip’s picture

FileSize
2.37 KB

Yep, sounds good. Re rolled and added those changes.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

It's looking perfect now!

Anonymous’s picture

Status: Reviewed & tested by the community » Needs work

The normalizer should only be registered once. Since this normalizer will default to lowest priority, anyone who wants to override the normalizer can just add their own with a higher priority. They don't need to rely on the format specific string (e.g. serializer.normalizer.xml).

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.25 KB
1014 bytes

So more something like that, see interdiff.

Anonymous’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Yeah, that sounds good. It strikes me that it might be a good idea to have tests for this. I've written tests for JSON-LD (which were recently restructured in the patch in #1838596: Add deserialize for JSON-LD). We should probably use the same structure for all serializer tests.

Marking as needs work for tests. If anyone else disagrees and thinks the tests can be a followup, I'm fine with that, just create the follow up issue. If no one else adds tests before tonight, I'll try to make time.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
5.25 KB

This patch moves the EntityNormalizer out of the Entity directory and into a new Serialization directory. It also modifies the normalizer to use getPropertyValues().

I've added a test for the normalize function. It needs to be expanded, but just posting what I have for now.

damiankloip’s picture

FileSize
4.61 KB
6.98 KB

Nice work Lin.

I don't think we need to add more tests for this? The assertEqual will fail if any values are different or any array keys don't match/correspond too.

I have added a test for the actual serialization too. Then we can test that we have the available formats registered in the container etc..

damiankloip’s picture

FileSize
6.89 KB

Sorry, this patch. interdiff still pretty much applies. I left in a line getting the serializer in the EntityNormalizer test that I didn't mean to.

Status: Needs review » Needs work

The last submitted patch, 1846000-13.patch, failed testing.

Anonymous’s picture

The assertEqual will fail if any values are different or any array keys don't match/correspond too.

It will also fail if the data has changed order. Since order doesn't matter, I think it would be preferable to check each key individually and then array_diff the expected keys and normalized keys to ensure that no other data items were added. However, I won't hold up the patch on that if others don't want to.

dawehner’s picture

Status: Needs work » Needs review
FileSize
776 bytes
7.65 KB

It feels wrong that xmlencoder defines itself as implemting normalizerInterface. Why should xml know how to normalize the data?
Here is a patch which fixes the patch, though i'm still not sure whether this is the route to go, lin
probably knows way better, whether this makes sense.

Anonymous’s picture

Status: Needs review » Needs work

Just mentioned this to damian in IRC, NormalizationAwareInterface is different than NormalizerInterface. I'm going to take a look at the failing test today.

fago’s picture

Status: Needs work » Needs review
+  /**
+   * Implements \Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize().
+   */
+  public function normalize($object, $format = NULL) {
+    return $object->getPropertyValues();
+  }

I think that could be done better. With EntityNG, all fields and field values are based upon defined data types - as they are aregisted by the TypedData API. TypedData consists of complex data, lists and simple values, as outlined in the not-yet completed docs: http://drupal.org/node/1794140
So, the simple data types may have a mapping to a aset pre-defined primitive types as seen in http://api.drupal.org/api/drupal/core!lib!Drupal!Core!TypedData!Primitiv.... So the idea behind that mapping is that any-module defined data type, that e.g. maps to a date can be interprated as date. Thus, serializers can pick that up and serialize any type data (including entities) based upon that. Define how you are serializing
- complexdata structrues
- lists
- primitives
and ignore the rest. Then your serializer will work with any entity and any field type out of the box. Still, we can add special handling on top of that for, e.g. entity references if we want to.

damiankloip’s picture

FileSize
7.99 KB

I think we also need to support the 'ajax' content type that is used by Drupal (returned by ContentNegotiation). I have added a DrupalJsonEncoder class that supports json and ajax types. I have tested this with #1819760: Add a REST export display plugin and serializer integration. and seems to do the trick.

Fago; Sorry, I'm really not familiar with the Property API (yet!) So I would have to look into this. If you have an idea of how you think this should work, you know alot more about entities properties than me, so any help etc... is most welcome. If you have time obviously :)

Lin, over to you for the test failure...

Status: Needs review » Needs work

The last submitted patch, 1846000-19.patch, failed testing.

Anonymous’s picture

I took a closer look at the XMLEncoder. It turns out NormalizationAwareInterface works differently than I assumed from the name. It actually skips the normalizer and handles all normalization internally. If we want to use any of the functions from the TypedData API, we shouldn't register XmlEncoder as the encoder or inherit from it.

I believe we could use composition to get the desired functionality. We would implement a custom encoder (which would not be NormalizationAware). The data would go through a normalizer to be turned into the proper array structure. Then it would be passed into our custom encoder, which would instantiate an instance of XmlEncoder and shunt all the work off to that.

For now, I recommend focussing this issue on JSON and AJAX.

Anonymous’s picture

Assigned: Unassigned » linclark

Assigning this to myself to remove the XML and also to try to implement what fago suggests. I think I know what his comment means, he'll have to correct me if I'm wrong.

Anonymous’s picture

Title: Add serializer support for JSON and XML » Add serializer support for JSON and AJAX
FileSize
7.37 KB
7.3 KB

This patch removes XML.

It also:

  • renames DrupalJsonEncoder to JsonEncoder
  • merges the tests into one file
  • moves test entity creation into setUp()
  • Adds a test for ajax
  • Removes the second registration of the JSON encoder

I'm going to work on fago's suggestion next.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
7.43 KB
9.77 KB

This patch splits the functionality into two Normalizers, ComplexData and TypedData. It also fixes the Normalizer test which wasn't working because assertEqual was returning TRUE for a comparison between an array and null. I've expanded that test as I had explained in an early comment.

Ideally, we wouldn't use the serializer from the DIC in the Normalizer test, but would construct our own. I'm too tired for that now, but can switch it over in the next patch.

The interdiff is from 23.

dawehner’s picture

+++ b/core/modules/system/lib/Drupal/system/Tests/Serialization/EntitySerializationTest.phpundefined
@@ -0,0 +1,116 @@
+    foreach (array_keys($expected) as $fieldName) {
+      $this->assertEqual($expected[$fieldName], $normalized[$fieldName], "ComplexDataNormalizer produces expected array for $fieldName.");
+    }
+    $this->assertEqual(array_keys($expected), array_keys($normalized), 'No unexpected data is added to the normalized array.');

Can't we just use one single assertEqual?

+++ b/core/modules/system/lib/Drupal/system/Tests/Serialization/EntitySerializationTest.phpundefined
@@ -0,0 +1,116 @@
+    // Test 'json'.
...
+    // Test 'ajax'.

Should we maybe also check some kind of other format, and be sure it's not serialized?

+++ b/core/modules/system/lib/Drupal/system/Tests/Serialization/SerializationTest.phpundefined
@@ -2,7 +2,7 @@
+ * Definition of Drupal\system\Tests\Serialization\SerializationTest.

If we fix that, then in a consistent way :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Serialization/EntitySerializationTest.phpundefined
@@ -0,0 +1,116 @@
+class EntitySerializationTest extends WebTestBase {

It seems to be that we could convert this to DrupalUnitTestBase, i would like to do that.

damiankloip’s picture

Assigned: linclark » damiankloip

Just doing some stuff on this now.

damiankloip’s picture

FileSize
5.07 KB
10.33 KB

Ok, removed the use of the container in the tests, but I've just been thinking, don't we actually want the tests to encompass the use of the container to check the encoder and normalizers are registered properly etc..?

  • Changed the file docblocks to be consistent
  • Create a Serializer object, pass in our encoders/decoders and store this on the test class

I think what Lin is saying above that in the array of values, array() and NULL would not fail. So that's why each element is checked individually.

We could test that a random format doesn't work, I think this would throw a serializer exception though? So we would need to catch it, then use that. Is that something we do in tests?

If you want to convert this to DrupalUnitTestBase, go for it! Will this work ok with the CacheDecorator on the plugin manager (TypedData)?

fago’s picture

oh, great to see you going for typedData normalizer!

Not sure how this weighting thing works, but I think we should make sure that the general type data normalizer has lowest priority - else it will take over handling lists and complex data also. That said, I think there should be also a list-normalizer. Yes, getValue() gives you an array for entities - but that really is the "internal" list representation what might be something different in other use-cases.
Actually, I think the list interface misses something like setArray() and getArray() for that use case, right now you'd have to use ArrayAccess or iterating over the object to build your array yourself :/

Status: Needs review » Needs work
Issue tags: -Needs tests, -Web services, -VDC

The last submitted patch, 18460000-27.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +Web services, +VDC

#27: 18460000-27.patch queued for re-testing.

damiankloip’s picture

Assigned: damiankloip » Unassigned
Anonymous’s picture

Assigned: Unassigned » linclark

Ok, removed the use of the container in the tests, but I've just been thinking, don't we actually want the tests to encompass the use of the container to check the encoder and normalizers are registered properly etc..?

We don't want to use the container in the Normalize test. We do want to use it in the Serialize test.

We could test that a random format doesn't work, I think this would throw a serializer exception though? So we would need to catch it, then use that. Is that something we do in tests?

Since it is a simple array test and we aren't doing any overriding between different Normalizers, I don't think we need to test this.

If you want to convert this to DrupalUnitTestBase, go for it! Will this work ok with the CacheDecorator on the plugin manager (TypedData)?

I didn't know whether DrupalUnitTest would work with Entity, but if we can make it work, great!

Not sure how this weighting thing works, but I think we should make sure that the general type data normalizer has lowest priority - else it will take over handling lists and complex data also. That said, I think there should be also a list-normalizer.

Yeah, I was thinking about this after I posted last night. The ordering is correct for the two that are used (the first matching Normalizer is used). However, there does need to be a ListNormalizer in between them.

Will post a patch shortly.

damiankloip’s picture

Here is a follow up as suggested: #1854874: Add serializer support for XML

damiankloip’s picture

Also, created an issue, as per Fago's suggestion in #28: #1854782: Add to/setArray() methods to TypedData ListInterface

Anonymous’s picture

This patch adds a ListNormalizer. It also takes the Serializer setup that Damian added to setUp and moves it to testNormalize().

Anonymous’s picture

Assigned: linclark » Unassigned
damiankloip’s picture

Issue tags: -Needs tests

Nice, this is looking awesome now!

+++ b/core/lib/Drupal/Core/Serialization/ComplexDataNormalizer.phpundefined
@@ -0,0 +1,42 @@
+    foreach ($object as $name => $field) {
+      $attributes[$name] = $this->serializer->normalize($field, $format);
+    }
+    return $attributes;

We should declare the array first here. Also, should this be called attributes? not just generic 'data' or something (Just putting it out there)?

+++ b/core/lib/Drupal/Core/Serialization/ListNormalizer.phpundefined
@@ -0,0 +1,41 @@
+    foreach ($object as $fieldItem) {
+      $attributes[] = $this->serializer->normalize($fieldItem, $format);
+    }
+    return $attributes;

Same here. I guess this could be changed if we had the getArray method on the ListInterface too (See #34)?

Anonymous’s picture

Just added declaration of $attributes.

damiankloip’s picture

Sweet. This looks good.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Agreed. This one is ready.

fago’s picture

>Sweet. This looks good.
Indeed - it's great to see how simple the typed data normalizers end up being! So yeah, that's ready.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Reviewed and looks like another good interim step. Committed to 8.x. Thanks.

xjm’s picture

Status: Fixed » Reviewed & tested by the community

Edit: Or not. :)

xjm’s picture

Status: Reviewed & tested by the community » Fixed

xpost

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Component: other » serialization.module

moving to our shiny new component.