Problem/Motivation
The HAL and core Entity Reference field item normalizers will discard data passed to constructValue
if an entity is referenced by its UUID. The biggest impact of this is that normalizing then denormalizing an entity with an Image field will discard the alt, title, width, and height properties.
Proposed resolution
We should accept valid properties in data passed to constructValue
for these normalizers, so that users can normalize and denormalize the same structure without data loss.
Remaining tasks
Review the patch.
User interface changes
None.
API changes
None.
Data model changes
None.
Original issue summary
While working on the test coverage for #1927648-494: Allow creation of file entities from binary data via REST requests.3, I discovered that denormalizing file fields in HAL explicitly omits any values I send for a @FieldType=file
field.
\Drupal\hal\Normalizer\EntityReferenceItemNormalizer
inherits \Drupal\hal\Normalizer\FieldItemNormalizer::denormalize()
, which does $field_item->setValue($this->constructValue($data, $context));
. $data
here does contain 'description' => 'something'
.
Then we go into constructValue()
… which EntityReferenceItemNormalizer
overrides to this:
protected function constructValue($data, $context) {
$field_item = $context['target_instance'];
$field_definition = $field_item->getFieldDefinition();
$target_type = $field_definition->getSetting('target_type');
$id = $this->entityResolver->resolve($this, $data, $target_type);
if (isset($id)) {
return ['target_id' => $id];
}
return NULL;
}
… in other words: if the entity that the data is referencing exists, then we will set one property and one property only: target_id
. Which means that the display
and description
that FileItem extends EntityReferenceItem
is adding will never get set. Because EntityReferenceItemNormalizer
doesn't work for any subclasses. Same for ImageItem extends FileItem extends ReferenceItem
: its alt
, title
, width
and height
properties are impossible to set.
Comment | File | Size | Author |
---|---|---|---|
#16 | interdiff-2935738-14-16.txt | 681 bytes | samuel.mortenson |
#16 | 2935738-16.patch | 5.36 KB | samuel.mortenson |
Comments
Comment #2
BerdirKind of expected that, that's why I mentioned we should test that :)
That's why file_entity has http://cgit.drupalcode.org/file_entity/tree/src/Normalizer/FileItemNorma... for a long time, but it's fairly ugly.
Comment #3
BerdirThat said, I don't think this needs to block testing in the other issue, we simply can't test setting/getting the description yet.
Comment #4
Wim LeersAgreed, and that's what I did there! :)
.
Comment #5
samuel.mortensonI ran into something similar when just trying to normalize and denormalize an existing entity, but with the "json" format.
If you install the Standard profile, then create an Article with an image uploaded and alt text filled in, you should be able to replicate this locally (I'm using Drush's PHP console for this):
Comment #6
Wim LeersHm, so perhaps this is not limited to HAL's entity reference field normalizer.
The IS says it's caused by
hal
'sconstructValue()
override:And
serialization
's looks different:In the example above, you would be sending
target_uuid
. Could youunset($normalized['field_image']['target_uuid']);
and try again?Comment #7
samuel.mortenson@wim-leers Normalizing the Node, then unsetting target_uuid, then denormalizing properly set the field_image's alt text. Nice!
Comment #8
Wim Leersindeed, but that means both have bugs, yet in different ways… 🤦♂️
I think it makes sense to fix both in the same/single issue, because half the effort is the test coverage, and the test coverage can be shared. The only thing that's going to be different is the fixes!
Comment #9
kpv CreditAttribution: kpv at DrupalBASE commentedFor text fields you can set both
{ "field_text": {"value": "Dramallama"}, }
and
{ "field_text": [{"value": "Dramallama"}], }
when setting data for a request.
For file fields you should always pass it as an array, e.g.:
{ "field_file": [{"target_id": 42}], }
(Found when implementing #1927648-546: Allow creation of file entities from binary data via REST requests)
Comment #10
Wim Leers#9: yep, there's lots of weirdness in there :( Thanks for explicitly reporting another example!
Comment #11
samuel.mortensonThis may be overly simplistic but I found that merging in the data retained alt text (and other column) values. Would like to see if approach makes sense before writing tests.
Comment #13
samuel.mortensonAdded tests, made it so that only properties defined in the field item are set.
Edit: Should say that I fixed tests, I did not add new tests yet.
Comment #14
samuel.mortensonAdded test coverage, removed nitpick. Also updated the issue summary.
Comment #15
BerdirWe also have a @todo in \Drupal\Tests\rest\Functional\FileUploadResourceTestBase::testPostFileUpload() waiting for this to be resolved. If this indeed is enough to fix it, then we should be able to remove that todo and complete the test to make sure we can deal with a file/image field for both normalization and denormalization now.
Comment #16
samuel.mortenson@Berdir I removed the todo and the early return, and it looks like tests pass! Let's see what the test bot thinks.
Comment #17
Wim LeersGreat, we have proof that the properties that
class FileItem extends EntityReferenceItem
add work as expected! We have strict integration test coverage thanks to #1927648: Allow creation of file entities from binary data via REST requests, which is also the issue that originally triggered the creation of this issue.Since that works, we can reasonably assume that
class ImageItem extends FileItem
also works.For once, an API-First normalization issue is simple! Let's do this! 🤘
Comment #18
alexpottCrediting @Berdir and @Wim Leers for reviews.
Comment #19
alexpottCommitted a46c461 and pushed to 8.6.x. Thanks!
Comment #21
Wim Leers🎉
Comment #22
samuel.mortensonWow! Thanks everyone.