Spin off from #2426509: ContentEntityBase::__set() messes with values that happen to be TypedData
ContentEntityBase::__set(), ItemList::set() and FieldItemBase::__set() all have special logic to downcast assigned values that happen to be a TypedDataInterface to their ->getValue() array before assigning it.
The exact intent of that at each level is not fully clear.
- In ContentEntityBase::__set(), this allows easy copying of field values :
$e1->field = $e2->field; // does: $e1->field->setValue($e2->field->getValue())
(we copy the values, but don't actually reuse the same FieldItemList object)
--> @fago suggested we limit that behavior to FieldItemListInterface instead of "anything that is a TypedData".
Anything else gets passed as is to ItemList->setValue(), and if not an array, gets assigned to the first property of the first item.
- In ItemList::offsetSet()/set(), this allows easy copying of item values:
$e1->field[0] = $e2->field[1]; // does: $e1->field[0]->setValue($e2->field[1]->getValue())
--> Similarly, do we want FieldItemList to override that and limit that behavior to FieldItemInterface ?
- In FieldItemBase::__set(), well, it's not clear to me what this one is for, nor what it allows or forbids :-)
--> Do we want to keep it ?
--> If we do, then by the same reasoning than #2426509: ContentEntityBase::__set() messes with values that happen to be TypedData, it seems that behavior should only be applied to the "official properties" in the FieldItem, not to arbitrary runtime properties
--> What is done there is "downcast if TypedDataInterface **and not EntityInterface**". That part about EntityInterface is moot now, since Entities ar not TypedData anymore.
Comment | File | Size | Author |
---|---|---|---|
#18 | 2430843-nr-bot.txt | 150 bytes | needs-review-queue-bot |
#3 | interdiff.txt | 665 bytes | yched |
#3 | 2426509-TypedData_downcast-4.patch | 1.71 KB | yched |
#1 | 2426509-TypedData_downcast-1.patch | 5.44 KB | yched |
#1 | 2426509-TypedData_downcast-1-do-not-test.patch | 1.06 KB | yched |
Comments
Comment #1
yched CreditAttribution: yched commentedBaby steps - try the first part (limit the downcast in ContentEntityBase::__set() to FieldItemListInterface instead of anything that's a TypedData), and see what tests say.
Patch is rolled on top of #2426509: ContentEntityBase::__set() messes with values that happen to be TypedData, the actual change is in the -do-not-test.patch
Comment #2
yched CreditAttribution: yched commented(as a side note, this finally removes "use TypedDataInterface" in ContentEntityBase, which definitely feels like a yay)
Comment #3
yched CreditAttribution: yched commentedOK, green.
Then, trying to remove the downcast behavior in FieldItemBase::__set(), see what it does.
(#2426509: ContentEntityBase::__set() messes with values that happen to be TypedData landed meanwhile, so patch is directly against HEAD this time)
Comment #4
yched CreditAttribution: yched commentedOops, wrong nid in the patch names. Will fix that for later patches.
Comment #12
andypostComment #18
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.