Problem/Motivation

Discovered in #2626924-180: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata.

Nothing is throwing \Drupal\Core\TypedData\Exception\ReadOnlyException, even though it was added for a reason in #1696640: Implement API to unify entity properties and fields. It was present in the very first patch there: #1696640-3: Implement API to unify entity properties and fields. It's just that nothing has used it so far!

Drupal 8 core has several computed property typed data classes:

  1. \Drupal\text\TextProcessed
  2. \Drupal\datetime\DateTimeComputed

All of them have a setValue() method that actually sets a value, instead of throwing \Drupal\Core\TypedData\Exception\ReadOnlyException. Because the docs of \Drupal\Core\TypedData\TypedDataInterface::setValue() say this:

   * @throws \Drupal\Core\TypedData\Exception\ReadOnlyException
   *   If the data is read-only.

Which of course is pretty bad.

It also means that when you do a PATCH or POST request via the core REST module (or the contrib JSON API or GraphQL modules), you don't get an error message, but you should!

Proposed resolution

  1. Fix ::setValue() method of:
    1. \Drupal\text\TextProcessed
    2. \Drupal\datetime\DateTimeComputed
  2. Fix \Drupal\serialization\Normalizer\FieldItemNormalizer::denormalize() to only set non-computed properties' values

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#6 2921890-6.patch3.18 KBdagmar
#2 2921890-2.patch3.53 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 2921890-2.patch3.53 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Needs tests

This will need explicit REST test coverage.

Status: Needs review » Needs work

The last submitted patch, 2: 2921890-2.patch, failed testing. View results

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
commented
Status: Needs work » Needs review
StatusFileSize
new 2921890-6.patch3.18 KB

Re-rolled. #2 didn't apply.

Status: Needs review » Needs work

The last submitted patch, 6: 2921890-6.patch, failed testing. View results

amateescu’s picture

amateescu commented

Quoting the issue title:

Computed properties' classes' ::setValue() method should throw ReadOnlyException

Why is that? Computed does not mean read only..

yched’s picture

yched commented

"Computed does not mean read only"
Indeed. I think I remember struggling a bit with @fago and @Berdir so that the 'target_id' (stored) and 'entity' (computed) properties of an entity_ref field can be set both ways :

// setting 'target_id' updates 'entity'
$node1->field_ref->target_id = $node2->id();
$node1->field_ref->entity === $node2 // TRUE

// setting 'entity' updates 'target_id'
$node1->field_ref->entity = $node3;
$node1->field_ref->target_id === $node3->id(); // TRUE

IIRC that was related to the "autocreate taxo tags" feature, and that was kind of a pain to make it work :-)

amateescu’s picture

amateescu commented
Status: Needs work » Closed (works as designed)
Issue tags: -Needs tests

The parent issue arrived at the same conclusion in the meantime :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2972988: Error when saving a denormalized entity with text fields with "processed" property

See #2972988-5: Error when saving a denormalized entity with text fields with "processed" property — this really is a problem. But I should've made this issue not about computed properties, but read-only computed properties.

Anyway, I hope to now see you all over at #2972988: Error when saving a denormalized entity with text fields with "processed" property!