Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#38 | 1846000-38-entity-normalizer.patch | 11.84 KB | linclark |
#38 | interdiff.txt | 1.29 KB | linclark |
#35 | 1846000-35-entity-normalizer.patch | 11.78 KB | linclark |
#35 | interdiff.txt | 5.36 KB | linclark |
#27 | 18460000-27.patch | 10.33 KB | damiankloip |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedRather 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.
Comment #2
dawehnerShouldn't we just talk here about this is the common normalizer outputted as json?
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?
If we are in the context of entity, can't we document at least that here is an entity?
Nitpick: There seems to be an extra space before $data
Funny enough the adapted standard to document the bundle seems to be "/**
* $module dependency injection container.
*/" but that sounds like a bug.
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.
Yeah we don't use uppercase in classnames for Json, any special reason we don't do?
A silly question maybe: Is there a reason why to put this into system module and not core/lib?
Shouldn't we just talk here about this is the common normalizer outputted as json?
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?
If we are in the context of entity, can't we document at least that here is an entity?
Nitpick: There seems to be an extra space before $data
Funny enough the adapted standard to document the bundle seems to be "/**
* $module dependency injection container.
*/" but that sounds like a bug.
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.
Yeah we don't use uppercase in classnames for Json, any special reason we don't do?
Comment #3
damiankloip CreditAttribution: damiankloip commentedYeah 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.
Comment #4
damiankloip CreditAttribution: damiankloip commentedI think this will affect WS too.
Comment #5
dawehnerWe are now into the "Contains \$class" business.
What about an /** Implements \...NormalizerInterface::normalize() and keep the rest of the documentation, so we know it will be always an entity here?
We could probably just do an "Implements" as well. Yeah even less documentation.
It's not a problem to register them here, as it will be built into the container, so it is blazing fast.
What about an @see for the entity interface?
Comment #6
damiankloip CreditAttribution: damiankloip commentedYep, sounds good. Re rolled and added those changes.
Comment #7
dawehnerIt's looking perfect now!
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedThe 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).
Comment #9
dawehnerSo more something like that, see interdiff.
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedYeah, 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.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedThis 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.
Comment #12
damiankloip CreditAttribution: damiankloip commentedNice 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..
Comment #13
damiankloip CreditAttribution: damiankloip commentedSorry, 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.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedIt 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.
Comment #16
dawehnerIt 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.
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedJust mentioned this to damian in IRC, NormalizationAwareInterface is different than NormalizerInterface. I'm going to take a look at the failing test today.
Comment #18
fagoI 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.
Comment #19
damiankloip CreditAttribution: damiankloip commentedI 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...
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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.
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedAssigning 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.
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch removes XML.
It also:
I'm going to work on fago's suggestion next.
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedThis 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.
Comment #25
dawehnerCan't we just use one single assertEqual?
Should we maybe also check some kind of other format, and be sure it's not serialized?
If we fix that, then in a consistent way :)
It seems to be that we could convert this to DrupalUnitTestBase, i would like to do that.
Comment #26
damiankloip CreditAttribution: damiankloip commentedJust doing some stuff on this now.
Comment #27
damiankloip CreditAttribution: damiankloip commentedOk, 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)?
Comment #28
fagooh, 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 :/
Comment #30
damiankloip CreditAttribution: damiankloip commented#27: 18460000-27.patch queued for re-testing.
Comment #31
damiankloip CreditAttribution: damiankloip commentedComment #32
Anonymous (not verified) CreditAttribution: Anonymous commentedWe 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.
Comment #33
damiankloip CreditAttribution: damiankloip commentedHere is a follow up as suggested: #1854874: Add serializer support for XML
Comment #34
damiankloip CreditAttribution: damiankloip commentedAlso, created an issue, as per Fago's suggestion in #28: #1854782: Add to/setArray() methods to TypedData ListInterface
Comment #35
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch adds a ListNormalizer. It also takes the Serializer setup that Damian added to setUp and moves it to testNormalize().
Comment #36
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #37
damiankloip CreditAttribution: damiankloip commentedNice, this is looking awesome now!
We should declare the array first here. Also, should this be called attributes? not just generic 'data' or something (Just putting it out there)?
Same here. I guess this could be changed if we had the getArray method on the ListInterface too (See #34)?
Comment #38
Anonymous (not verified) CreditAttribution: Anonymous commentedJust added declaration of $attributes.
Comment #39
damiankloip CreditAttribution: damiankloip commentedSweet. This looks good.
Comment #40
moshe weitzman CreditAttribution: moshe weitzman commentedAgreed. This one is ready.
Comment #41
fago>Sweet. This looks good.
Indeed - it's great to see how simple the typed data normalizers end up being! So yeah, that's ready.
Comment #42
Dries CreditAttribution: Dries commentedReviewed and looks like another good interim step. Committed to 8.x. Thanks.
Comment #43
xjmEdit: Or not. :)
Comment #44
xjmxpost
Comment #46
Anonymous (not verified) CreditAttribution: Anonymous commentedmoving to our shiny new component.