Error when saving a denormalized entity with text fields with "processed" property

I would RTBC but I feel we need someone from the parent issue to take a look at this so we understand the previous change a bit better, I've pinged @WimLeers on IRC.

Thanks for reporting this! And thank you even more for providing a failing test!!! 🎉❤️👍

Digging in.

Rather than the expected/hoped 30-90 minutes, this took more like 6 hours to unravel. 😱

I initially thought this was related to #2957385: FieldItemNormalizer never calls @DataType-level normalizer service' ::denormalize() method. But given the patch (with failing test + fix), I started doubting that.

So I dug in.

The interesting thing is that … it does not make sense to send processed: it's a read-only computed field! However, we totally support the case of users doing this:

1. GET /something?_format=json
   
   {
     "title": {
       "value": "First post!"
     },
     "body": {
       "value": "Hello _world_!",
       "format": "markdown",
       "processed": "<p>Hello <em>world</em>!</p>",
     }
   }
   

2. mutate JSON: set [title][value] to e.g. foobar

3. POST /something?_format=json
   
   {
     "title": {
       "value": "foobar"
     },
     "body": {
       "value": "Hello _world_!",
       "format": "markdown",
       "processed": "<p>Hello <em>world</em>!</p>",
     }
   }
   

Because body is unchanged, thanks to #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior, we'll just ignore body, and not even bother denormalizing it, because it's unchanged! And we do have automated test coverage that iterates over all fields in a given entity type's test entity and verifies it can be included in a PATCH request.

Of course, the question is what happens when you modify the value property of a text field. At that point, the field is being changed. And yes, body's processed is only being exposed since #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata, which meant that only since then a PATCH request would potentially include a processed property.

But again:

The interesting thing is that … it does not make sense to send processed: it's a read-only computed field!

And that's where #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata made a mistake: it started normalizing this computed field … but did not ensure it would be ignored during denormalization! We don't have test coverage for this. All existing tests that are PATCHing a text field are doing so explicitly with only the value and format properties. Because those already existed.

Ironically, I even noticed this gap in the test coverage myself 1.5 years ago in #2626924-51: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata:

We should document why there is no denormalize() method, and we should have test coverage ensuring that sending processed as part of a PATCH or POST request results in an error.

Also see #2626924-184: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata, in response to a review, about this very subject, and for which I opened #2921890: Computed properties' classes' ::setValue() method should throw ReadOnlyException.

@amateescu closed #2921890 at #2921890-10: Computed properties' classes' ::setValue() method should throw ReadOnlyException. But I think that's because I had gotten the scope/intent wrong: I wanted to make all computed fields read-only. But I should've made it about ensuring read-only computed

What's tricky about this is that there's no way to reproduce this problem except for doing normalize(denormalize(decode($serialized_entity)). i.e. only if you normalize a denormalized entity, rather than a loaded entity, you can reproduce this. Which is exactly what your test does.

The easy solution would be to add a TextItemNormalizer whose denormalize() method would ignore the processed property that it receives. But that's not the correct solution. That's putting the logic in core's Serialization system, but which means it'll only work for core's rest.module and the https://www.drupal.org/project/relaxed module you maintain. It won't work for https://www.drupal.org/project/jsonapi or https://www.drupal.org/project/graphql. It'll be bad for the API-First ecosystem of modules — as mentioned in https://wimleers.com/blog/api-first-drupal-two-big-maintainability-miles... for example.

The correct solution is to fix this in the Field/Typed Data API. Then it'll work everywhere. That's why I wanted to do something like this:

diff --git a/core/lib/Drupal/Core/TypedData/TypedData.php b/core/lib/Drupal/Core/TypedData/TypedData.php
@@ -102,6 +103,9 @@ public function getValue() {
    * {@inheritdoc}
    */
   public function setValue($value, $notify = TRUE) {
+    $data_definition = $this->getDataDefinition();
+    if ($data_definition->isReadOnly() && $data_definition->isComputed()) {
+      throw new ReadOnlyException(sprintf('Setting a value for %s is not allowed: it is read-only and computed.', $this->getPropertyPath()));
+    }
     $this->value = $value;
     // Notify the parent of any changes.
     if ($notify && isset($this->parent)) {

…but unfortunately that is not possible, because setValue() is also used by Entity::create(), even if you just specify the value and format properties for a text field. So somewhere along the way, a processed property value is being generated to be set. So we need to dig even deeper in Entity/Field API, to avoid doing this for read-only computed properties!

And that's how I arrived in \Drupal\Core\TypedData\Plugin\DataType\Map::setValue(), where this happens:

    // Update any existing property objects.
    foreach ($this->properties as $name => $property) {
      $value = isset($values[$name]) ? $values[$name] : NULL;
      $property->setValue($value, FALSE);
      // Remove the value from $this->values to ensure it does not contain any
      // value for computed properties.
      unset($this->values[$name]);
    }

Despite $values === ['value' => 'something', 'format' => 'something'], this code insists on updating existing properties, even if it doesn't make sense to set a value on a read-only computed property! (The point being that it is … computed.) AND despite those last 3 lines wrt computed properties, a fix that landed in December 2014, in #2137309: Typed data does not handle set() and onChange() consistently. Obviously we haven't yet had the opportunity to pay attention to read-only computed fields (that's what this very issue is revealing), so I think it's fair to say that that code needs a small update: it should not update read-only computed properties.

By fixing it there, we don't even need to modify FieldItemNormalizer::denormalize(), which #2921890-2: Computed properties' classes' ::setValue() method should throw ReadOnlyException did need to change!

Whew, that was a super long comment, but hopefully others (and especially the Entity/Field/Typed Data API maintainers who'll need to review this) find it a helpful guide at hopefully arriving at the same conclusions.

Next up: patches!

First, ensure that we throw an explicit failure when trying to set a value for a read-only computed property.

1. Mark text fields' processed property read-only (it's already marked as computed). This should already have been the case; we're just updating the metadata to match reality.
2. Make TypedData::setValue() throw an exception when setting a value for a read-only computed property.
3. The previous point means we can remove TextProcessed::setValue(), because it's an override of TypedData::setValue(), and is no longer necessary (we need to either remove it, or add the same lines to it)

Then, to add test coverage:

1. Let the Comment REST test send a processed property in PATCH and POST requests, so that we're actually testing this.

This patch should FAIL.

(We should bring back the failing test that @jeqq wrote later. Right now I want this to be as simple and focused as possible, to make it easier for Entity/Field/Typed Data API maintainers to review.)

#6 should fail with errors like this:

1) Drupal\Tests\comment\Functional\Rest\CommentJsonBasicAuthTest::testPatch
Drupal\Core\TypedData\Exception\ReadOnlyException: Setting a value for comment_body.0.processed is not allowed: it is read-only and computed.

The solution is to implement the change to Map::setValue() that I described near the end of #5.

Isn't this a BC break? What if there's a contrib or custom module which is using "processed" as non-read-only field?

+++ b/core/modules/text/src/Plugin/Field/FieldType/TextItemBase.php
@@ -28,6 +28,7 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
+      ->setReadOnly(TRUE)

+++ b/core/modules/text/src/TextProcessed.php
@@ -68,17 +68,6 @@ public function getValue() {
-  public function setValue($value, $notify = TRUE) {
-    $this->processed = $value;
-    // Notify the parent of any changes.
-    if ($notify && isset($this->parent)) {
-      $this->parent->onChange($this->name);
-    }
-  }

#7 should fail with errors like this:

There was 1 failure:

1) Drupal\Tests\comment\Functional\Rest\CommentJsonBasicAuthTest::testPatch
Failed asserting that an array has the subset Array &0 (
    0 => Array &1 (
        'value' => 'Llamas are awesome.'
        'format' => 'plain_text'
        'processed' => '<p>Llamas are <script>alert("evil")</script> awesome.</p>'
    )
).
--- Expected
+++ Actual
@@ @@
-            [processed] => <p>Llamas are <script>alert("evil")</script> awesome.</p>

/Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:1188
/Users/wim.leers/Work/d8/core/modules/comment/tests/src/Functional/Rest/CommentJsonBasicAuthTest.php:35

because the test coverage that #2882717: EntityResourceTestBase: assert that PATCHed and POSTed entities contain the intended values introduced made assumptions that no longer hold true; since now request bodies may include read-only computed properties. processed isn't stored, so we shouldn't be asserting that it is stored to verify that the data in our request bodies is actually stored: we should omit read-only computed properties from that expectation!

So, we update those assertions to take this into account and … now the patch should be green! ✅

#8:

Isn't this a BC break? What if there's a contrib or custom module which is using "processed" as non-read-only field?

That's impossible. You can't write to it today either. You could theoretically set a value and have it exist for the remainder of the life of that entity object, but that's it. It wouldn't be saved. It couldn't be saved. And it would be SUPER INSECURE to do so anyway — because that's what the filter.module's filter plugins are for!

I'm glad you made that necessary scrutinary remark — somebody had to make it. It's something we need to discuss and question. But for this particular property, this is not a BC break IMO. It may be for some others.

If this issue gets blocked on that BC concern, I'll eat my hat. 🎩 🤠

Re: #10: ok, sounds reasonable, just wanted to check! 😉

#11: 👍

The failures are due to \Drupal\text\Plugin\Field\FieldType\TextItemBase::onChange():

  public function onChange($property_name, $notify = TRUE) {
    // Unset processed properties that are affected by the change.
    foreach ($this->definition->getPropertyDefinitions() as $property => $definition) {
      if ($definition->getClass() == '\Drupal\text\TextProcessed') {
        if ($property_name == 'format' || ($definition->getSetting('text source') == $property_name)) {
          $this->writePropertyValue($property, NULL);
        }
      }
    }
    parent::onChange($property_name, $notify);
  }

i.e. that write.

That write's intent is to "reset" the computed value of the processed property, because the data it's based upon has been modified.

But why do we need to write that; it's computed, so why do we need to write anything? Because those computed values are currently cached (#2083785: Add support for determining which field properties are cacheable added that): they're stored in-memory on the entity object, so essentially they're statically cached. Hence any writes to their dependencies need to invalidate the computed value.

IOW: it's the static cache that gets in the way.

So we need to figure out a way to support this from a Field/Typed Data API POV.

For now, just disabling TextItemBase::onChange(). Let's see what happens. What is relying on this?

@Wim Leers great investigating as usual!

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -1233,6 +1229,55 @@ public function testPatch() {
+   * @param $field_name

Super Nit, @param type needed

The fix looks good.

Just saw the \Drupal\text\Plugin\Field\FieldType\TextItemBase::onChange write error you found 😉

@Wim Leers
Is it save to use your patch while theres still some failing tests?

@Wim Leers

I would also like to know if patch #15 would be safe to use on a production site. The error is popping up when using the REST API to create content. It still seems to create the content, but the error reported in this issue occurs on the target site. If I apply patch #15 the issue goes away and I get a 201 response code from the target site.

@Lee You can use this patch https://www.drupal.org/files/issues/2018-05-15/text-processed-fix.patch from the creator of the issue. It worked well.

@Patrick Thanks I had missed that one. It works perfectly for me.

Neither patch has been thoroughly reviewed and committed, so I wouldn't recommend either at this time.

Let's get this patch to green!

14 to 2 failures, much better :)

+++ b/core/modules/text/src/Plugin/Field/FieldType/TextItemBase.php
@@ -60,7 +61,8 @@ public function onChange($property_name, $notify = TRUE) {
       if ($definition->getClass() == '\Drupal\text\TextProcessed') {
         if ($property_name == 'format' || ($definition->getSetting('text source') == $property_name)) {
-          $this->writePropertyValue($property, NULL);
+          // @todo figure out another way; this need support from Field/Typed Data APIs.
+//          $this->writePropertyValue($property, NULL);
         }

The remaining failures are caused by this change. See #15 for detailed explanation. Static caching is getting in the way.

That's the last problem to solve; I'm leaving that for an Entity/Field/Typed Data maintainer.

Now also reported by a JSON API user: #2984466: [PP-1] Sending a text field's "processed" property breaks POST or PATCH requests.

1. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Map.php
   @@ -79,6 +79,10 @@ public function setValue($values, $notify = TRUE) {
   +      if ($data_definition->isReadOnly() && $data_definition->isComputed()) {
   
   +++ b/core/lib/Drupal/Core/TypedData/TypedData.php
   @@ -102,6 +103,10 @@ public function getValue() {
   +    if ($data_definition->isReadOnly() && $data_definition->isComputed()) {
   

   I'm wondering whether it would be worth documenting these lines with a rational.

2. +++ b/core/lib/Drupal/Core/TypedData/TypedData.php
   @@ -102,6 +103,10 @@ public function getValue() {
   diff --git a/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php b/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php
   
   diff --git a/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php b/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php
   index 7d1abdb..aad76a0 100644
   
   index 7d1abdb..aad76a0 100644
   --- a/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php
   
   --- a/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php
   +++ b/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php
   
   +++ b/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php
   +++ b/core/modules/comment/tests/src/Functional/Rest/CommentResourceTestBase.php
   @@ -238,6 +238,7 @@ protected function getNormalizedPostEntity() {
   
   @@ -238,6 +238,7 @@ protected function getNormalizedPostEntity() {
            [
              'value' => 'Llamas are awesome.',
              'format' => 'plain_text',
   +          'processed' => '<p>Llamas are <script>alert("evil")</script> awesome.</p>',
            ],
          ],
        ];
   

   It feels odd to test this just on comments. Maybe we could add a text field to entity_test entities?

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -1233,6 +1229,55 @@ public function testPatch() {
   +    /* @var \Drupal\Core\TypedData\DataDefinitionInterface[] $property_definitions */
   +    $property_definitions = $field->getItemDefinition()->getPropertyDefinitions();
   +    foreach ($property_definitions as $property_name => $property_definition) {
   +      if ($property_definition->isReadOnly() && $property_definition->isComputed()) {
   +        $read_only_computed_property_names[] = $property_name;
   +      }
   +    }
   +
   +    return $read_only_computed_property_names;
   

   Nitpick: Any reason to not use a filter + map here?

4. +++ b/core/modules/text/src/Plugin/Field/FieldType/TextItemBase.php
   @@ -60,7 +61,8 @@ public function onChange($property_name, $notify = TRUE) {
        foreach ($this->definition->getPropertyDefinitions() as $property => $definition) {
          if ($definition->getClass() == '\Drupal\text\TextProcessed') {
            if ($property_name == 'format' || ($definition->getSetting('text source') == $property_name)) {
   -          $this->writePropertyValue($property, NULL);
   +          // @todo figure out another way; this need support from Field/Typed Data APIs.
   +//          $this->writePropertyValue($property, NULL);
            }
   

   Couldn't we check for readOnly here too?

1. 👍 ✔️
2. Sure! But I'd first like a +1 on the overall approach (and have green tests) before working on expanding test coverage. And most importantly:
   there's one @todo yet to be figured out.
3. Because an array_map() can't access the keys :(
4. This is not about not writing, this is about resetting a statically cached computed value, see #15 for details.

In addition to #29.4 pointing to #15:

+++ b/core/modules/text/src/Plugin/Field/FieldType/TextItemBase.php
@@ -60,7 +61,8 @@ public function onChange($property_name, $notify = TRUE) {
+//          $this->writePropertyValue($property, NULL);

Uncomment this, then run \Drupal\Tests\text\Kernel\TextWithSummaryItemTest::testCrudAndUpdate(). That makes it easy to reproduce the problem.

Added to #3002188: REST: top priorities for Drupal 8.7.x.

Hey,

I see that this issue was added to be fixed at 8.7.x, but I don't think it was, at recent release of 8.7.
Nor I see an issue for this on 8.8.x.

Anyone can update where this stands?

Thanks,

It looks like the bot that automatically changes the target version didn't fire off.

I confirm this issue still exists in Drupal 8.7.3

I just came across this same issue with Drupal 8.7.6, while trying to use the default_content module.

- deleted -

Hello,
Facing this issue on a fresh D8.9.14 migrated from D6.
Error: Call to a member function getProcessedText() on string in Drupal\text\TextProcessed->getValue() (line 43 of core/modules/text/src/TextProcessed.php).

If someone found a way to fix it.
Thanks

Error still persists in D9.4. As this is rather old and still not fixed, gues I'm doing something wrong when working with entity serialization in general. Is the serialization API not meant to be built for saving denormalized entities?

Triaging my component queue by tagging #29 and trying to continue on.

Test only shows the same error as the summary https://git.drupalcode.org/issue/drupal-2972988/-/jobs/9414259

99% of the fix was already implemented in the previous patches but I ended up reverting some of it and doing an unset in Map vs fixing the spots where tests failed.

Disclosure Claude was use to help with the test.

MR looks good to me, but I have not manually reproduced the bug to ensure the fix.

Another pair of eyes before RTBC would be nice.

We're modifying a core library, but aren't adding explicit test coverage for this change to the library's tests. Coverage has only been added to a downstream module. @dawehner effectively had this same concern in #28 and @wim leers agreed to it in #29 pending approval of the approach. But that coverage never got added. The suggestion was made to add a text field to entity_test, which from what I can tell is exactly the test in the MR does. So maybe all that needs to happen is moving the test.