Problem/Motivation
Just before the committed #2824851 patch was reverted due to a security vulnerability that it exposed in another subsystem, @cburschka wrote this at #2824851-110: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior:
The PathItem::class case in getModifiedEntityForPatchTesting isn't right:
case PathItem::class: // PathItem::generateSampleValue() doesn't set a PID, which causes // PathItem::postSave() to fail. Keep the PID (and other properties), // just modify the alias. $value = $field->getValue(); $value['alias'] = str_replace(' ', '-', strtolower((new Random())->sentences(3))); $field->setValue($value); break;::getValue() returns the list of items, not the first item. We'd have to set $value[0]['alias'] here to work correctly. Current data sent to ::setValue()
array(2) { [0]=> array(4) { ["pid"]=> string(1) "1" ["source"]=> string(7) "/node/1" ["alias"]=> string(6) "/llama" ["langcode"]=> string(2) "en" } ["alias"]=> string(40) "cogo-luctus-mauris-nostrud-tation-valde." }(The PATCH still gets rejected as designed. That's why it didn't cause a test failure.)
Can we do this as a follow-up, or does it need a separate issue?
I am currently working on porting REST's integration test coverage to JSON API in #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only, and this has come back to bite us.
The only reason it isn't a problem in core is that core's normalizers happily attempt to normalize whatever data they're given, including this case, where we're setting a second item on a single-cardinality field.
The PATCH request only fails because core perceives this as attempting to add a second item to this field, which qualifies as a change in value, and hence the edit field access check for the path field fails.
Proposed resolution
Rather than relying on this edge case, we should make this behave as @cburschka proposed. Because in the future, core may very well prevent adding a second item to a single-cardinality field … and in fact I'm surprised it doesn't already. The ->setValue() call quoted above should throw an exception, but doesn't.
Remaining tasks
None.
User interface changes
None.
API changes
None.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 2939802-2.patch | 1 KB | wim leers |
Comments
Comment #2
wim leersRe-uploading @cburschka's patch from #2824851-112: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior. I only renamed it.
Comment #3
wim leersManually tested, works fine.
Please credit @cburschka!
Comment #4
wim leersComment #6
catchComment #7
catchCommitted/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!