Posted by damiankloip on November 20, 2012 at 5:50pm
11 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | serialization.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | VDC, Web services |
Issue Summary
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| d8.serializer-json-xml.patch | 4.31 KB | Idle | PASSED: [[SimpleTest]]: [MySQL] 48,338 pass(es). | View details |
Comments
#1
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.
#2
+++ 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?
#3
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.
#4
I think this will affect WS too.
#5
+++ 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?
#6
Yep, sounds good. Re rolled and added those changes.
#7
It's looking perfect now!
#8
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).
#9
So more something like that, see interdiff.
#10
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.
#11
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.
#12
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..
#13
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.
#14
The last submitted patch, 1846000-13.patch, failed testing.
#15
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.
#16
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.
#17
Just mentioned this to damian in IRC, NormalizationAwareInterface is different than NormalizerInterface. I'm going to take a look at the failing test today.
#18
+ /**+ * 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!Primitive.php/class/Primitive/8. 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.
#19
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...
#20
The last submitted patch, 1846000-19.patch, failed testing.
#21
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.
#22
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.
#23
This patch removes XML.
It also:
I'm going to work on fago's suggestion next.
#24
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.
#25
+++ 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.
#26
Just doing some stuff on this now.
#27
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..?
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)?
#28
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 :/
#29
The last submitted patch, 18460000-27.patch, failed testing.
#30
#27: 18460000-27.patch queued for re-testing.
#31
#32
We don't want to use the container in the Normalize test. We do want to use it in the Serialize test.
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.
I didn't know whether DrupalUnitTest would work with Entity, but if we can make it work, great!
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.
#33
Here is a follow up as suggested: #1854874: Add serializer support for XML
#34
Also, created an issue, as per Fago's suggestion in #28: #1854782: Add get/setArray() methods to TypedData ListInterface
#35
This patch adds a ListNormalizer. It also takes the Serializer setup that Damian added to setUp and moves it to testNormalize().
#36
#37
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)?
#38
Just added declaration of $attributes.
#39
Sweet. This looks good.
#40
Agreed. This one is ready.
#41
>Sweet. This looks good.
Indeed - it's great to see how simple the typed data normalizers end up being! So yeah, that's ready.
#42
Reviewed and looks like another good interim step. Committed to 8.x. Thanks.
#43
Edit: Or not. :)
#44
xpost
#45
Automatically closed -- issue fixed for 2 weeks with no activity.
#46
moving to our shiny new component.