To support POST/PATCH with a format, the Normalizers must implement DenormalizerInterface.
Discussion about whether or not we should support POST/PATCH in hal+json should go in #1929632: Decide how to handle POST/PATCH when using HAL. This issue is simply for implementation in case we do decide to support it.
Proposed resolution
Add denormalizers for the Typed Data interfaces we are supporting.
Because the language is identified in the field item data, we must be able to translate in the FieldItemNormalizer. However, because the language can only be managed from the entity object, the entity object must be passed in. To do this, create the 'target_instance' for both the field and field item in the calling denormalizer (e.g. entity gets the next field object and passes it into FieldNormalzier using $context['target_instance']). Then, the FieldItemNormalizer can traverse the property path back up to the entity and create a new path to a translated field item.
Comment | File | Size | Author |
---|---|---|---|
#28 | 1931976-28-minor-nits.patch | 1.96 KB | effulgentsia |
#23 | 1931976-23-hal-deserialize.patch | 28.23 KB | effulgentsia |
#23 | interdiff.txt | 2.72 KB | effulgentsia |
#20 | 1931976-20-hal-deserialize.patch | 28.47 KB | linclark |
#20 | interdiff.txt | 7.4 KB | linclark |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch just gets a start, using the TypeLinkManager to figure out which bundle the type URI corresponds to and creating the corresponding entity. This is similar to the RDF Mapping Manager we were using in the JSON-LD case.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedAdded support for marking fields for deletion.
Comment #3
Anonymous (not verified) CreditAttribution: Anonymous commentedLast patch had a minor logic error in the test. Interdiff is from #1.
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch adds support for deserializing regular fields. Because a field item is not in control of its own language, there is some really janky code.
This patch is not complete. I comment out some of the previous HAL tests and have debug messages instead of assertions for the test that isn't commented out. I just wanted to post for comparison, because I'm going to try something else in the next patch.
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedThis moves the translation logic to the FieldItemNormalizer, since field items are what contain the language information. It also adds tests for text field denormalization.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedI needed to merge due to #1921490: $supportedInterfaceOrClass should not be static.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch adds full deserialization support by adding the decoder and fixes a problem caused by entity_create automatically adding values for fields like UUID.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedReroll since info.yml got in.
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedUpdated with a few changes from #1924220: Support serialization in hal+json and some comments.
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedReroll now that HAL serialization is in. There are no outstanding dependencies for this patch.
Comment #12
klausisuperfluous white space.
I did some manual testing with REST module and creating entity_test entities, which works. So I guess we can move forward here, RTBC.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedI would like to see more review of the actual approach before this gets committed, especially since Larry raised concerns about it on the call, but I guess this can move forward if everyone else agrees.
Comment #14
webchickCrell, what say you?
Comment #15
Crell CreditAttribution: Crell commentedOnly the first comment is commit-blocking, probably. This all makes sense to the extent that I understand what all has been written. :-) I'm still behind in my understanding here.
I think we need to account for type being an array. There's no reason it should ever be multi-value, but from discussion on the HAL list it looks like we should account for *any* link being an array at any time.
If this is using the Countable interface in PHP, we should be able to do count($field), and defaulting to 0 should be handled internally there.
If that's not the case, then this patch can continue but then we need to go change Fields to use Countable properly. :-)
I'm probably late to this game and should have caught it earlier, but... Containers have no business in unit tests. Wire things up manually. Is this code not actually unit testable?
If there's some dependency that prevents it from being unit testable, we need @todos to mark that so we know how much work we have to do when we finally clean up those parts of core that prevent real unit tests.
Shouldn't this have a negative message for WHY it failed, not what it would have said if it passed? I thought that was the convention.
Comment #16
webchickComment #17
effulgentsia CreditAttribution: effulgentsia commentedI might be out of date, but last I remember (from a discussion a month or so ago) was that we wanted to structure our regular
json
to be the same as ourhal+json
, just without the_links
and_embedded
sections. And for us to have full deserialization (including entity references, andlang
on field items) working in regularjson
. Is that still the case? If so, how much overlap would there be between that code and the code here, and does it make sense to do the other one first, and have the code here extend that with just the part specific to _links (and possibly _embedded, though not sure if there's anything relevant to deserialization in _embedded, since it's a duplicate of what already exists in the "normal" part of the json)?Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commented@Crell
Good point, although we'll only be able to use one of them. I will just write it so that it chooses the first one it finds a match for.
I could have sworn this failed one of the tests before, but I tried it again and it passes now at least.
I believe other DrupalUnitTestBase tests use the container that the base class adds, I think that's how I found out about it. I'm not opposed to making this change, the only downside I see is that then the constructed Serializer can get out of sync with the Normalizers/Encoders defined in the Bundle (which happened once already in serialization.module).
There's no mention of exceptions in the SimpleTest coding standard and there doesn't seem to be one convention. For example:
Same message for pass and fail:
Different message:
However, I'm not opposed to changing it.
@effulgentsia
The divergence between the structuring of our regular JSON and hal+json was touched upon in the serialization issue. I would rather tackle hal+json first and consider whether to handle deserialization for regular JSON as a separate issue.
Comment #19
Anonymous (not verified) CreditAttribution: Anonymous commentedThis patch fixes the first two of Crell's issues and a small code style issue. I still need to fix the second two.
Comment #20
Anonymous (not verified) CreditAttribution: Anonymous commentedThis fixes the rest of the issues Crell pointed out.
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedOk. That means a fair amount of the code this patch is adding within HAL module will eventually move somewhere more reusable, but that's fine.
Other than that, after spending some time with this patch, and Lin patiently explaining some HAL concepts to me, I think this looks great other than some nits. I don't consider any of these to be commit blockers (they can be cleaned up in a follow up), but CNW to see if Lin is willing to do one more reroll to address some of them.
I think we want to use LANGUAGE_NOT_SPECIFIED here rather than language_default(). language_default() is the language the site should output things into by default, but here we're receiving input, and if the input hasn't specified the language, then that's what LANGUAGE_NOT_SPECIFIED is for. I think we can remove the comment about DrupalUnitTest, since proper language handling isn't just for unit testing.
I don't like the term "typed data ids" for referring to things like 'entity_type' and 'bundle', but looks like we already have code in HEAD (RDF module) that does this, so we can punt that discussion to a follow up.
Can we expand the comments as to why we're removing them from $data? Especially for the _embedded one, let's add a @todo pointing to #1880424: Handle entity references on import.
Setting $class here overrides the function parameter of the same name. Not a functional problem, since we no longer use the original parameter, but confusing. Perhaps calling it $field_class would be clearer?
The name 'target_instance' tripped me up, because:
1) Seeing it next to $field made me think of what $instance is within Field API, which is *the configuration* of a field specific to a particular bundle. That's not what 'instance' is referring to here though: here it just means an object we want the denormalizer we're about to call to populate rather than instantiating a new one.
2) I'm not clear what 'target' is intending to convey. Entityreference module uses the term 'target' to refer to the referenced entity, but this code has nothing to do with entity references.
Therefore, how about simply $context['object']?
"== TRUE" is redundant and can be removed.
Can we rename this method to getTranslatedFieldItem()?
Lets call this $field instead of $parent.
I think it's ok for us to know Drupal's data model of entity/field/item, and replace this with simply
$entity = $field->getParent()
. However, if there's some reason we want flexibility to accommodate other data models here, then at least let's replace themethod_exists()
withinstanceof
. The interface is Drupal\Core\TypedData\TranslatableInterface.Drupal now requires PHP 5.3.10, so this can all be replaced with
is_a($type, $this->supportedInterfaceOrClass, TRUE)
.Why are we only testing $no_field_value for not being empty? What do we actually expect it to be?
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedI've started processing these comments, but will be headed to an event soon, so just responding to a little bit now.
Using LANGUAGE_NOT_SPECIFIED makes it so that the call to
$denormalized->get('field_test_translatable_text')->getValue()
fails to return the correct value when running tests. To be honest, the intricacies of the interactions between the entity system, the multilingual system, and DrupalUnitTests are difficult for me to debug, so I don't know what is causing the underlying issue.Added comment.
Good point, changed.
I see your point, but 'object' feels to generic to me. I'll think about it some more.
Fixed.
Comment #23
effulgentsia CreditAttribution: effulgentsia commentedFixed that. The problem is just one of consistency. If langcode isn't specified, then 'en' values are translations, not default language values. So we can either change the test to identify the resource as 'en', or we can remove 'en' from the data items we want to assert as nontranslated ones. I suppose in principle we should test both variations, but here, I just arbitrarily picked the latter.
Can you post a patch for the fixes you made in #22, and then I think the rest can be a follow up.
Comment #24
webchickMy understanding is we can't call the REST web services stuff complete unless this is done, so marking this as major.
Comment #25
effulgentsia CreditAttribution: effulgentsia commentedThis was RTBC already in #12. It has only improved since then. I'm ok with my nits from #21 being addressed in a follow up patch. Getting this in will help resume progress on #1880424: Handle entity references on import. Therefore, I suggest committing #23 and then setting this issue back to needs work with normal priority.
Comment #26
webchickIt seems like the major portions of eff's review (namely the language handling stuff) were addressed here. Looks like lin meant to upload a patch in #22, but it didn't quite make it. In the interests of moving things along with web services, though, I think it makes sense to commit this and then push the rest (ha, get it?) of the more minor things to a follow-up issue.
Committed and pushed to 8.x. Thanks!
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedDoesn't this mean that we don't actually test that we can ingest the data that we export?
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedHere's @linclark's fixes from #22 extracted out of #1935538-17: Switch REST to default to HAL.
Comment #29
webchickWell that's easy enough.
Committed and pushed to 8.x. Thanks!
Comment #30
effulgentsia CreditAttribution: effulgentsia commentedThanks! Back to needs work for #27. That shouldn't be too hard: I'll try to post a patch here soon.
Comment #30.0
effulgentsia CreditAttribution: effulgentsia commentedproposed resolution.
Comment #39
Wim LeersSigh. This was filed against the wrong component. This was supposed to be a mandatory follow-up for #1924220: Support serialization in hal+json, but obviously it was never completed.
Because, yes, this is true:
It's very much broken. We're dealing with the fallout of this over at #2135829: [PP-1] EntityResource: translations support and #2904423: Translated field denormalization creates duplicate values.
Comment #46
SpokjeThe
hal
module has moved out of Drupal Core and into a Contrib Module.Moving this issue to the Contrib Module queue.