Problem/Motivation
We want to be able to respond to GET requests on entity pages like node/1 with a JSON-LD representation of the node.
Proposed resolution
We can leverage Symfony's Serializer component to do this. We simply need to provide a Normalizer and Encoder for JSON-LD.
The Normalizer receives an entity and processes it into the correct object/array structure for JSON-LD. The encoder then takes that normalized object and spits it out as JSON. Both Normalizers and Encoders can specify what they are able to process. The JSON-LD N/E should only be used when the object is a subclass of Entity (or EntityNG for now) and the requested format is 'jsonld'.
This activity diagram demonstrates how it could work. The section in purple is addressed in this issue. The section in green is the duty of the Symfony Serializer, to be added with #1810472: Add Symfony's Serializer component to core despite Symfony potentially releasing BC-breaking updates after 2.3.. Sections in white are still in discussion and outside the scope of this issue.
Comment | File | Size | Author |
---|---|---|---|
#60 | 1811510_60_jsonld_mimetype.patch | 1.43 KB | scor |
#57 | 1811510-57-jsonld-entity.patch | 18.67 KB | linclark |
#57 | interdiff.txt | 1.35 KB | linclark |
#54 | 1811510-54-jsonld-entity.patch | 17.32 KB | linclark |
#54 | interdiff.txt | 2.91 KB | linclark |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch covers the purple parts of the diagram above, enabling basic serialization using a Normalizer and Encoder. It will only respond if the format requested is jsonld and the object for serialization is an Entity.
To enable testing of this patch, I have added a route and controller, and have also added a URI callback to entity_test. These should not be evaluated since routing/controllers should not be a part of serialization. I've broken out the code that should be evaluated in the __for-review patch. Just to show what stopgap code is necessary for testing, I've broken that out into a __temp patch. The __for-testing patch includes both, along with the Serializer patch from #1810472.
Because core entities have not been converted to EntityNG yet, this patch uses the entity_test entity.
To test this patch:
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedI removed the @context handling, which will be tackled in #1794958: Generate the @context for JSON-LD.
Comment #4
tstoecklerThe patch was named "for-review" so I took a stab at it :-)
Disclaimer: I know absolutely nothing about JSON-LD and about the new property API, so it might be that this makes absolutely no sense.
Don't really know what the use-case would be, so maybe the answer to this question is "No", but shouldn't this also keep the 'options' from $uri_info and just merge the 'absolute' => TRUE in?
I guess this should use static:: instead of self::
First of all,
values
is protected, so no idea how that can work :-)Secondly, how does this relate to translations? I've yet to grasp the new entity property API, but I see a method called
getTranslatedField()
in EntityNG and then also the following @todo on EntityNG::$values:@todo: Add methods for getting original fields and for determining changes.
Is that why you are using $values?
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedWe want a canonical URI for the entity here, so I don't think we'd want to use any options that are passed in. For example, we don't want the language to be included in the URI.
I used self:: because Symfony's JsonNormalizer and Encoder use self::, but I can switch to static::. It would actually be better if Symfony used static:: there, because then the method could be removed on the Encoder. I might post an issue on Symfony.
I didn't check the visibility, but it definitely works. RE: translations... they are included in the field structure in the same way for both Drupal and JSON-LD.
Comment #6
Crell CreditAttribution: Crell commentedPlease do. :-) In practice there's little reason to use self:: anymore as of PHP 5.3; usually static:: is what you mean.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedThis just has some small changes, self changed to static and code style.
I will be posting a patch with actual changes soon, so going to switch this to needs work for now.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedBased on the discussion in #1797210: Decide how to negotiate between the 2 JSON-LD serializations, we will need to have two different JSON-LD serializations; one for content staging, and another that provides data in a way that external consumers expect it. We will negotiate between these two by using different mime types:
This patch provides Normalizers, Encoders, and EntityWrappers for both of those serializations. The Drupal-specific Jsonld classes extend from the default Jsonld classes.
JsonldEntityWrapper should probably be extracted out into an interface. However, I'm going to let the patch mature more before doing that.
To test this:
If you want to toggle between vnd.drupal.ld+json and the (incomplete) ld+json, go to JsonldController and change 'drupal_jsonld' to 'jsonld'. Eventually, this string will be a variable provided by the content negotiation framework.
Comment #9
moshe weitzman CreditAttribution: moshe weitzman commentedI finally got this patch applied and enabled the json_ld and entity_test modules. The patch works, and is nice and small.
Comment #10
Crell CreditAttribution: Crell commented"Drupal" is not needed in the class name.
"Drupal" is not needed in the class name.
camelCase, please.
This is the sexy part. :-)
We should be able to use a JsonResponse here, no?
I like how straightforward this looks so far. That means we must be doing something right.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for the reviews :)
This is because that's how the Entity Field API handles those properties. The options I can see are either 1) fixing that in Entity Field API (which I think is preferable), or 2) we could handle it in this module by having a defined list of properties which should be extracted from their language wrappers.
I was expecting this would be brought up. Using "Drupal" in a class name is against coding standards. However, in this case DrupalJsonldEncoder corresponds to the media type vnd.drupal.ld+json. I can't think of a more suitable name (either for the media type or the class). However, I'm happy to change this if someone has a suggestion. Would VndDrupalJsonld be better for the class name?
I tried that earlier and it actually double encodes the json, so I don't think Serializer was meant to work with JsonResponse... it seems like they are two ways of doing the same thing, basically.
Also, the JsonldController will go away once the REST module adds its controllers. Because we want the controller to be format agnostic, it will not have any knowledge of which response class it should use. However, that does touch on the issue of how we are going to figure out which Content-type header to use. The JsonldController is hard coded to pass the json Content-type header into the Response, but that's going to need to be handled differently once REST module is handling the controller.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch adds some of the things requested in #9.
RE: README... I might be missing something. I checked and it doesn't look like other core modules have readme files.
RE: api docs... There isn't yet anything that can be documented in jsonld.api.php, since there are no hooks. I believe I followed all the doc block standards. Let me know if I'm missing something.
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedOh right. No README for core modules ... How about a unit test which exercises the JSON-LD Normalizer+Serializer? Hopefully we could use UnitTestBase and not WebTestBase
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedThis line feels wrong and doesn't enable distinction between translatable and non-translatable fields (per #1346214-36: [meta] Unified Entity Field API, we now call everything fields, not properties). I don't know what the right API is yet. Maybe fago can shed some light.
Otherwise, I think this patch is great, and have nothing to add to what Moshe and Crell already pointed out.
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedOops. xpost. I reviewed #8 rather than #12.
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedI added tests for the normalizer. I don't think we need tests for the encoder at this point, since it is simply using Symfony's JsonEncoder behaviors, but could add those tests if necessary.
I haven't addressed the issue in #14 about using the values array. I thought that the values array would include any language handling we would need.
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedThe patch had an unnecessary dependency on entity_test, which I removed.
I also removed the whitespace issues there were in the last chunk.
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commented#19: 1811510-19-jsonld-entity__for-review.patch queued for re-testing.
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedThe same exception has been thrown twice:
I don't understand how this could be related to the changes made in this patch. Does anyone else know?
Comment #24
effulgentsia CreditAttribution: effulgentsia commented#19: 1811510-19-jsonld-entity__for-review.patch queued for re-testing.
Comment #25
effulgentsia CreditAttribution: effulgentsia commentedLocally, I'm getting a different exception thrown in the same test:
Base table or view not found: 1146 Table 'd8.simpletest360676system' doesn't exist"
. But I'm getting that same exception on HEAD. So I queued a bot retest to see what comes back.Comment #26
Anonymous (not verified) CreditAttribution: Anonymous commentedThere is a bug in Entity Field API which will prevent us from getting the appropriate language handled values as inquired about in #14.
fago said "that method [Entity::getTranslation] does not apply strict mode, so that's a bug". He agreed to file an issue about it and will hopefully post here (or ping me) when the issue is created.
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedThe test failure is very likely not due to any direct problem with this patch, but rather with simpletest and the DIC. #1708692: Bundle services aren't available in the request that enables the module may solve it, though it would be nice to figure out why exactly the introduction of this patch is triggering it. My guess is that jsonld.module is the disabled module directly preceding layout.module alphabetically (language.module not being disabled by default), but that's just a guess right now, it could be something else.
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedThe first set of tests I did were too abstract for tests. After comparing with both core tests and Symfony's Normalizer tests, I decided to go more procedural. The attached interdiff is against the patch in #12. It is not a diff against the first set of tests.
Comment #31
fagoad ##2: I've created #1822706: EntityTranslation fails to consistenly apply strict mode.
Still, you should be able to work around that rather easily. E.g. this (untested) code shold do it:
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous commentedThe problem with that is that non-translatable values such as UUID are still language tagged. It gives me the same value as the ->values property.
Comment #33
effulgentsia CreditAttribution: effulgentsia commentedOn a phone call, Crell had suggested that since the JSON output is intended for machines, not humans, then language tagging everything for consistency might not be a bad thing.
Alternatively, I could see the JSON output maybe not language tagging the fields that are 'entity keys'. Not sure if the responsibility for that should be the entity API, or if the JSON normalizer should call entity_get_info() and compare each field against the 'entity keys' list.
Comment #34
Anonymous (not verified) CreditAttribution: Anonymous commentedI'm less knowledgable about standards concerning multilingual, so this could very well be wrong...
Using 'und' in general will most likely be against the spirit, if not the letter, of language maps in JSON-LD. Language map keys are expected to follow the IETF's best current practice doc BCP 47. AFAIK, there is no defined 'und'. While you can use private use subtags, the BCP doc says:
If we DID go ahead and mint a private use subtag, it would be something like x-und.
If we don't use und, I believe the JSON above would look as follows:
Comment #35
fagoMaybe it would make sense to follow the structure the new entity API uses? E.g. like that:
This way we've a the same structure for translatable and untranslatable fields and you don't have to care about language by default, as you just go with the default language. I'm not sure whether this would map well to json-ld though?
Comment #36
Anonymous (not verified) CreditAttribution: Anonymous commentedThat would be a very unusual way to model the data. If we were going with something so unusual, I'm not sure that we get any advantage from using JSON-LD.
Rather than using 'und' in the JSON-LD to mean that it is in the default language, could we use the language code for the default language? When a production site is consuming the data from staging, wouldn't it know what the default language is? Then it could access the values using the language code.
Comment #37
effulgentsia CreditAttribution: effulgentsia commentedI don't necessarily think we want to do #35, but if we did, why is it so unusual? The idea is, here's "an entity in its default language" as a resource, and here's "a translation of the entity to language X" as a resource, and then ideally, having a way to properly connect the translation to the entity so that the entity and all translations could be inlined in the same JSON response. I'm sure there's systems out there that model translations of documents as separate documents. Again, I don't know what the pros and cons of #35 are, but I'd be surprised if it's flat out wrong.
Yes, we can and should. Not sure where the responsibility for this belongs (inside the Entity API or inside the normalizer), but it should be easy.
Not automatically it wouldn't (it could be a new node, and each node can have its own default language), but we can make it part of the JSON. Is there any already existing standard for where to put that info?
Comment #38
Anonymous (not verified) CreditAttribution: Anonymous commentedIt's not that it is wrong, it's that it is unusual. I could explain this more, but I will be posting a patch that shows how we could do it in a more common way, so I'd prefer to see whether that suits us. After reviewing the patch, if people are still interested in the less common approach I can explain what makes it idiosyncratic.
The language that is set for an entity is a property of the entity, and is currently exposed. As far as exposing the site's default language, I don't believe that is necessary for our use case.
I need to work on the tests a bit more, but should be posting a patch later tonight.
Comment #39
Anonymous (not verified) CreditAttribution: Anonymous commentedI added translation handling to DrupalJsonldEntityWrapper::getProperties(). I changed the tests to account for this.
I also created a jsonld_test entity type. I assumed this was necessary to get translation handling. It is not, but I figured it was worth keeping. It is probably better not to depend on an external module's test entity anyways.
Assuming you have an entity with the following:
The JSON-LD would look like the following:
To Crell's point about ease of processing, I believe it will be pretty easy to transform this back to our internal structure using a simple processing routine on Denormalization. The @context keeps track of which properties contain language maps and which do not. For any that are not language maps, we can simply nest the value inside an array keyed by 'und'. That should return us to the array structure we would expect.
Currently, two of the new tests should fail.
"Translatable field with multiple language values is nested correctly.", based on #1823584: EntityNG::language() returns FALSEThere are also a number of exceptions, "Trying to get property of non-object" and "Undefined index: translatable". It was odd, because the code triggering those exceptions is producing the expected results.
EDIT: It turns out that one of those fails is because the entity_test module (which I based jsonld_test on) has a langcode field which needs special property handling. I will add this back into jsonld_test (or switch back to entity_test) to fix this.
Comment #41
Anonymous (not verified) CreditAttribution: Anonymous commentedSo after switching back to entity_test, I'm still getting FALSE for $this->entity->language().
I think I'm going to need help from someone who knows the entity translation system better than I do. The parts that need work are DrupalJsonldEntityWrapper and the testNormalize function in DrupalJsonldNormalizerTest.
Comment #42
fagoI'm not sure it's really so unusual, but anyway I'd be interested in the explanation as it it might at least partly apply to the entity system as well.
@ease-of-processing:
Doesn't that apply to the client side the very same way?
We already use the 'entity_test' entity type for a lots of different entity related tests, so I think it would be fine to continue this for json-ld / entity serialization. Entity translation does also.
Having a single entity type saves us from maintaining lots of different test entities also and ensures that a single implementation can serve all the different components at the same time.
So should this default to request content language maybe? But then it sounds like being another use-case as it could potentially add-in a translation then.
Thus, if the entity is marked as language neutral it should be probably output that way also. That's something that might be interesting to the client.
translatable is optional, thus use empty() to check it.
I'm not sure where the problem lies. Is the structure you've read out from the entity not as intended?
That should not happen. Maybe that requires language module or something to work? entity translation tests seem to enable 'locale' module, so maybe we've got an unwanted dependency there.
Comment #43
Anonymous (not verified) CreditAttribution: Anonymous commentedI'm at a one day conference, so I cant respond fully ATM regarding the data model.
Many clients will request a single language version of the entity. I suggest that we serve that without language maps.
For those that are processing the full multilingual version, they can choose to use the same simple processing algorithm.
The nice thing about this is that all of the information you need to figure out the relationship the properties have to the entity is directly in the data. With the other way, you need to have out-of-band knowledge that the lang_de property is a special property that contains other values of its sibling properties.
Comment #44
effulgentsia CreditAttribution: effulgentsia commentedWhy would we have any entity with a default language of 'und' (LANGUAGE_NOT_SPECIFIED)? Language neutral entities should have a default language of 'zxx' (LANGUAGE_NOT_APPLICABLE), which is a standard langcode that means that, so doesn't run into the problem of #34. Perhaps the only entities with 'und' are ones that were updated from D7? It's an interesting question what to do with those, since we don't yet know that they are language neutral, all we know is that we don't know their language.
Comment #45
Anonymous (not verified) CreditAttribution: Anonymous commentedAs I mentioned, multilingual is not something that I've familiarized myself with before. So I found IANA's language subtag registry and it turns out that there is actually an 'und' in the registry. It is one of the 4 tags marked as "special" (as is zxx), but that shouldn't be a problem.
For this issue, I'm going to go back to using 'und' for non-translatable fields. I plan to use the langcode of the default language (entity default if set, or site default if not) when I find 'und' in translatable fields.
We may want to consider further how we want to use 'und' for non-translatable fields. There are some values, such as UUID, which simply are not linguistic content, and could use the zxx tag. There would also be fields on single language sites that aren't technically translatable (because the site isn't multilingual) but are clearly translatable content, and could use the default langcode of the site. AFAIK, we don't currently have a way of distinguishing these, but could consider introducing one in a follow up issue.
Comment #46
Anonymous (not verified) CreditAttribution: Anonymous commentedSo it turns out I have to manually add German as a language before using it as the default language for an entity.
Setting an entity's language to an invalid language (as I was doing) should probably result in an exception, I have filed an issue for this, #1824964: Setting entity language to an invalid language should throw exception.
Comment #47
Anonymous (not verified) CreditAttribution: Anonymous commentedThe attached patch:
The interdiff is from #39, except that for ease of reviewing, it does not show the removal of the jsonld_test entity.
There will still be one test failure, as described in #39. "Translatable field with single language value is nested correctly.", which I assume fails because of #1822706: EntityTranslation fails to consistenly apply strict mode.
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commentedIt seems that the behavior I thought was part of the bug in #1822706: EntityTranslation fails to consistenly apply strict mode is instead the expected behavior. Even if a field hasn't been given a value in a particular translation, it will still be a part of the translation's property set.
This now checks whether $translation['field_name']->getValue() returns an empty array. If there is no value for the field in the language, the language is not shown in the JSON-LD.
Comment #50
fagoYou can use isEmpty() for that, which will also work with field items containg an empty value, e.g. array('value' => '').
Comment #51
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks, I've updated the patch to use Field::isEmpty().
Comment #52
Crell CreditAttribution: Crell commentedConvention elsewhere has been to alias the parent class to SymfonyJsonEncoder or BaseJsonEncoder or something, then just name ours JsonldEncoder. Don't repeat the "Drupal" string in the class name.
Is there somewhere else we can get language_default from other than variable_get? Please? :-( If this is coming from the Container than we can set something up in the container config, probably...
Why is_subclass_of? instanceOf is preferred, and works for base classes and interfaces, too.
:-( Why can't this be a unit test?
Comment #53
effulgentsia CreditAttribution: effulgentsia commentedPer #11, why is it bad for Drupal to be in the class name if it corresponds to Drupal being in the mime type?
Comment #54
Anonymous (not verified) CreditAttribution: Anonymous commentedAs @effulgentsia pointed out, I discussed this in #11. We are creating two classes here, the JsonldEncoder and the DrupalJsonldEncoder, one per mime type.
Since there is currently no difference in the behaviors between the two, we probably could just collapse them into one and check against the array of supported formats in supportsEncoding. However, there definitely need to be two normalizers, so if the issue is having "Drupal" in the class name, the comments from #11 still stand. I'm open to changing it if there is a suggestion for a better descriptor.
It turns out there is, language_default()... multilingual is not my jam :-/
But instead of using that, I was able to rewrite the language handling code so that default language is no longer needed, plus it is more concise. I'm not sure whether this new code will break once #1822706: EntityTranslation fails to consistenly apply strict mode is fixed... I don't understand how that is supposed to work and/or how it is breaking.
Fixed in patch.
I'd be happy to be shown wrong, but I don't think we can use the entity system fully enough in a UnitTest. We definitely need to use the entity system for the normalizer tests... the main function of the normalizer is to convert from the entity object structure to the array. Thus, we want these tests to detect whether changes in the entity system affect the JSON-LD output.
We could use UnitTest for encoder tests, but I haven't included those since all behaviors are simply inherited from JsonEncoder.
Comment #55
moshe weitzman CreditAttribution: moshe weitzman commentedI reviewed the code and the tests and they look good. This is slowing down rest module and web services in general. Lets commit and iterate as we need to.
Comment #56
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for the review, Moshe.
For REST to use this, #1814864: Provide way to register serialization classes will need to be committed and this module will need to register its Normalizers and Encoders. I'd like to add that support before this patch is committed.
I work on that now and post a patch.
Comment #57
Anonymous (not verified) CreditAttribution: Anonymous commentedThis adds integration with the serializer service. It depends on #1814864: Provide way to register serialization classes being committed.
I would feel comfortable committing this and iterating in follow up issues.
Comment #58
moshe weitzman CreditAttribution: moshe weitzman commentedThis one is ready too.
We discussed that sometimes caller wants rendered text fields and sometimes not. At BADCamp sprint, we thought that application/ld_json would send rendered and application/vnd.drupal.ld+json would send raw text plus format. Alex and Lin discussed the mechanism for rendering text values and I heard a lot of TypedData and Normalizer words but can't clearly recount them here. Anyway, thats a followup.
Comment #59
webchickAwesome work! This is SO EXCITING!!
We are over thresholds by a couple of major bugs right now, but I feel that it's strategically important to commit this while there are people sprinting at BADCamp, so...
Committed and pushed to 8.x. Thanks!
Comment #60
scor CreditAttribution: scor commentedminor fix in the help text: JSON-LD MIME type is
application/ld+json
.Comment #61
klausiConfirmed that the latest patch only changes one character with git diff --color-words. Good to go.
Comment #62
webchickCommitted and pushed the follow-up to 8.x as well.
Comment #63
Anonymous (not verified) CreditAttribution: Anonymous commentedI've added a followup at #1832840: Enable fieldtype-specific JSON-LD normalization, which will allow us to properly support entity references, and also to do things like sanitize the values of a text field when application/ld+json is requested.