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

Without this, it can can indeed be painful/difficult to consume text data entered in Drupal.

\Drupal\text\Plugin\Field\FieldType\TextItemBase::propertyDefinitions() says:

  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
    $properties['value'] = DataDefinition::create('string')
      ->setLabel(t('Text'))
      ->setRequired(TRUE);

    $properties['format'] = DataDefinition::create('filter_format')
      ->setLabel(t('Text format'));

    $properties['processed'] = DataDefinition::create('string')
      ->setLabel(t('Processed text'))
      ->setDescription(t('The text with the text format applied.'))
      ->setComputed(TRUE)
      ->setClass('\Drupal\text\TextProcessed')
      ->setSetting('text source', 'value');

    return $properties;
  }

So, just exposing that computed processed property would fix this.

Yay, thanks for taking this on @frob! I will happily provide reviews! Let's do this :)

I think the REST module will be improved by this change.

In terms of review as something that extends ServiceProviderBase and NormalizerBase

It follows a well established pattern -and everything looks good.

As for my changes I have resolved only trivial things.

Hmm to answer the testing question from #9

For general information \Drupal\hal\Tests\FileNormalizeTest provides a good patterns for such a test harness, but the question is should we

I guess if some cunning hackers can find a dangerous input which the system accidentally manipulates into bad output.... then potentially we should test.
I am afraid I don't have enough system oversight to have a opinion.

+++ b/core/modules/text/src/TextItemServiceProvider.php
@@ -0,0 +1,32 @@
+    // Check for installed REST & HAL modules, as HAL requires REST.
+    if (isset($modules['serialization'])) {

The comment and the if-test don't match.

So the only big thing missing here, is test coverage! Great work :)

1. +++ b/core/modules/text/src/Normalizer/TextItemNormalizer.php
   @@ -0,0 +1,32 @@
   + * Converts typed data objects to arrays.
   

   This comment needs to be adjusted to match what this class actually does.

2. +++ b/core/modules/text/src/Normalizer/TextItemNormalizer.php
   @@ -0,0 +1,32 @@
   +  public function normalize($object, $format = NULL, array $context = array()) {
   

   Nit: s/array()/[]/

3. +++ b/core/modules/text/src/Normalizer/TextItemNormalizer.php
   @@ -0,0 +1,32 @@
   +    $attributes->processed = check_markup($object->value, $object->format);
   

   What's missing here is the third parameter. The entity has a langcode, we should pass that there.

4. +++ b/core/modules/text/src/TextItemServiceProvider.php
   @@ -0,0 +1,32 @@
   +    // Check for installed serialization module.
   ...
   +      // Add a normalizer service for text fields.
   

   I think these comments can just be omitted, I think both speak for themselves.

Do you have an example or doc page to go off of for the test coverage of this?

Yes!

Look at:

• integration test: \Drupal\serialization\Tests\NormalizerTestBase (and the tests that extend this base class)
• unit test: \Drupal\Tests\serialization\Unit\Normalizer\ListNormalizerTest tests \Drupal\serialization\Normalizer\ListNormalizer

Closed #2756667: Rendered content from REST /GET as a duplicate of this.

In most recent patch i noticed a minor typo: parkup

I decided to review the patch but ended up with all kind of points, so this patch makes it actually working as well as adding a test.

1. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,31 @@
   +    $attributes = $object->getValue();
   

   I don't think this is right, I think this should extend the ComplexDataNormalizer and get the values that way (from the parent::normalize() method) then add this computed property.

2. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,31 @@
   +    $attributes['processed'] = (string) $object->processed;
   

   This could be passed to the serializer too, functionally there is little point. Thoughts?

3. +++ b/core/modules/text/src/TextServiceProvider.php
   @@ -0,0 +1,27 @@
   + * Service Provider for Text Items.
   

   "Service provider for text module" ?

4. +++ b/core/modules/text/src/TextServiceProvider.php
   @@ -0,0 +1,27 @@
   +      $container->setDefinition('serializer.normalizer.text', $service_definition);
   

   text_item I think is better.

5. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -0,0 +1,65 @@
   +class TextItemBaseNormalizerTest extends KernelTestBase {
   

   It's sad this needs to be a kernel test. I am not even going to ask, I know you would have made it a unit test if that was not a problem...

In an ideal word this would be handled in a generic way, so a field item normalizer could just expose computed properties. I don;t think we are able to do that in the current world order though :/

Here is a new one. Thank you @damiankloip for a, as always, proper review.

Any time, Daniel!

+++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
@@ -21,10 +21,8 @@ class TextItemBaseNormalizer extends NormalizerBase {
+    $attributes['processed'] = $this->serializer->normalize($object, $format, $context);

This needs to normalize $object->processed?

Well my IDE never failed, but it also didn't stopped in its infinite loop.

Let's see whether this fixes the problem

I've been playing with this patch. It seems the issue with the failing tests is due the line:

$service_definition->addTag('normalizer', ['priority' => 20]);

If I remove that line, the failing tests work again, but the new one fails.

To be honest I don't know why changing this from 20 to 1 makes the failing tests and the new one added by @dawehner work. Let see what the testbot says.

Interesting!

I'm wondering whether we could provide some list of all normalizers and their priorities to understand the order of them.

Some other findings. This patch should modify also EntitySerializationTest:

http://cgit.drupalcode.org/drupal/tree/core/modules/serialization/src/Te...

     'non_rev_field' => array(),
      'field_test_text' => array(
        array(
          'value' => $this->values['field_test_text']['value'],
          'format' => $this->values['field_test_text']['format'],
        ),
      ),

And http://cgit.drupalcode.org/drupal/tree/core/modules/serialization/src/Te...

<?php
$expected = array(
      'id' => '<id><value>' . $this->entity->id() . '</value></id>',
      'uuid' => '<uuid><value>' . $this->entity->uuid() . '</value></uuid>',
      'langcode' => '<langcode><value>en</value></langcode>',
      'name' => '<name><value>' . $this->values['name'] . '</value></name>',
      'type' => '<type><value>entity_test_mulrev</value></type>',
      'created' => '<created><value>' . $this->entity->created->value . '</value></created>',
      'user_id' => '<user_id><target_id>' . $this->user->id() . '</target_id><target_type>' . $this->user->getEntityTypeId() . '</target_type><target_uuid>' . $this->user->uuid() . '</target_uuid><url>' . $this->user->url() . '</url></user_id>',
      'revision_id' => '<revision_id><value>' . $this->entity->getRevisionId() . '</value></revision_id>',
      'default_langcode' => '<default_langcode><value>1</value></default_langcode>',
      'non_rev_field' => '<non_rev_field/>',
      'field_test_text' => '<field_test_text><value>' . $this->values['field_test_text']['value'] . '</value><format>' . $this->values['field_test_text']['format'] . '</format></field_test_text>',
    );
?>

Which doesn't include the new processed value.

To be honest I'm not sure whether we should expose the processed value all the time. It could easily double the payload of those requests.
Is there any way how we can opt in things ... ?

@frob
Well the additional payload could be an issue we introduce. Ideally one doesn't introduce bugs as part of tasks :) Feel free to simply tell me that this is not really an issue in the first place.

Just wondering, could we use the approach from #16 and the test from #36?

Never mind, we tried #16 using the entity embed module and didnt work.

The approach that fix the issue is #36, we should find the way to fix the tests.

#46: "payload" != "server load". Payload == the data you send. So dawehner was saying that this means a lot more data to send (via the network).

You're right though that the processed value is cached, because GET requests are cached by Dynamic Page Cache.

Fixed EntitySerializationTest as mentioned in #39.

Also fixed namespace in \Drupal\Tests\text\Kernel\Normalizer\TextItemBaseNormalizerTest
and 2 coding standard issues.

Gave it another look over. Just another small coding standard nit.

1. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,27 @@
   +  public function normalize($object, $format = NULL, array $context = []) {
   

   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.

2. +++ b/core/modules/text/src/TextServiceProvider.php
   @@ -0,0 +1,27 @@
   +    if (isset($modules['serialization'])) {
   

   I'm not sure why it even needs to be conditional, I think it can just be defined in text.services.yml: there's no harm at all in having unused services.

3. +++ b/core/modules/text/src/TextServiceProvider.php
   @@ -0,0 +1,27 @@
   +      $service_definition->addTag('normalizer', ['priority' => 1]);
   

   Why priority 1?

4. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -0,0 +1,66 @@
   +  public function testNormalize() {
   +    /** @var \Symfony\Component\Serializer\Serializer $serializer */
   +    $serializer = \Drupal::service('serializer');
   +
   +    $entity = EntityTest::create([
   +      'field_text' => 'text item',
   +    ]);
   +    $entity->save();
   +
   +    $data = $serializer->normalize($entity);
   +    $this->assertEquals([
   +      'value' => 'text item',
   +      'processed' => "<p>text item</p>\n",
   +      'format' => NULL,
   +    ], $data['field_text'][0]);
   +  }
   

   This is testing the case of format = NULL (I did not even know this was possible?). It should also test a case of a format being defined.

@Wim Leers thanks for review.

1. Added comment.
Remaining task

and we should have test coverage ensuring that sending processed as part of a PATCH or POST request results in an error.

2. moved to text.services.yml:
3. It will only work for me if there is a priority . Haven't had a chance to see what it needs to be ahead of but 100 works.
4. Created a format and testing for that.

This is testing the case of format = NULL (I did not even know this was possible?).

Debugged and it falls back to plain_text but you can't specify plain_text explicitly.

Changing to "Needs Review" to trigger tests. but still have a remaining test under 1.

Changing priority to 10. This should fix a lot (all?) of the fails.

1. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,31 @@
   + * Adds processed markup from text fields to normalizer data.
   

   Hm, "markup" made me frown here. Do we want s/markup/text/?

2. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,31 @@
   + * This class does not implement DenormalizerInterface because the 'proccessed'
   + * attribute represents text processed by the filter system therefore it cannot
   + * be provided when creating new entities.
   

   Great! Perhaps mention that 'processed' is computed?

3. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -0,0 +1,98 @@
   +    FilterFormat::create(array(
   ...
   +      'filters' => array(
   

   s/array()/[]/

4. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -0,0 +1,98 @@
   +    /** @var \Symfony\Component\Serializer\Serializer $serializer */
   +    $serializer = \Drupal::service('serializer');
   

   This should be assigned to $this->serializer in setUp().

5. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -0,0 +1,98 @@
   +    $this->assertEquals([
   +      'value' => 'text item',
   ...
   +    $this->assertEquals([
   +      'value' => '<strong>This</strong> is <b>important.</b>',
   

   This can be converted to use a @dataProvider, which will make it easier to add more test cases.

6. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -0,0 +1,98 @@
   +      'field_text' => [
   +          'value' => '<strong>This</strong> is <b>important.</b>',
   +          'format' => 'my_text_format',
   +      ],
   

   This uses 4 spaces of indentation, should be 2.

7. +++ b/core/modules/text/text.services.yml
   @@ -0,0 +1,6 @@
   +      - { name: normalizer, priority: 10 }
   

   Let's document why this needs to be 10.

   Also, why was 1 okay before, but now it needs to be 10?

@Wim Leers thanks for review
1-6 fixed

7. It didn't see any justification in this issue for why it needed to be 1. So I tried find the lowest it could be and still work. I added a comment for what I found in the services.yml file.
Here is what I found:
In #52 I changed it to 100 which caused REST related tests to fail with this error:

fail: [Fatal error] Line 0 of :
[02-Nov-2016 08:49:59 Australia/Sydney] Uncaught PHP Exception InvalidArgumentException: "Field value is unknown." at /var/www/html/core/lib/Drupal/Core/Entity/ContentEntityBase.php line 479

I found that 10 would make these pass 11 would fail. I found that hal.services.yml sets a number of service to 10. It seemed the new service needed to be the same or higher priority than those.
I tested switch those services in hal.services to 9 to see which one would cause the REST tests to fail.
i found that I could switch any of the services in hal.services from 9 to 10 except serializer.normalizer.field_item.hal
This makes sense because the fail mention above is in \Drupal\Core\Entity\ContentEntityBase::getTranslatedField and \Drupal\hal\Normalizer\FieldItemNormalizer::denormalize calls. \Drupal\hal\Normalizer\FieldItemNormalizer::createTranslatedInstance

Honestly I am little lost about why the new service has to be the same or before serializer.normalizer.field_item.hal but maybe my debugging would help some or is more experienced see what is going on.

Regarding point 7: yeah that's just the pain/brittleness of weights (Drupal) and priorities (Symfony). I'll bet it's because the YAML files are all read first, and then service providers are called. It used to be in a service provider, now it's in a YAML file.

1. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -0,0 +1,117 @@
   +  public function testNormalize(array $create_values, array $expected) {
   +    $entity = EntityTest::create($create_values);
   ...
   +        'field_text' => 'text item',
   ...
   +        'field_text' => [
   +          'value' => '<strong>This</strong> is <b>important.</b>',
   +          'format' => 'my_text_format',
   +        ],
   

   I think it'd be simpler to hardcode EntityTest::create(['field_text' => $text_item]), which would then simplify the data providers.

2. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -0,0 +1,117 @@
   +    $test_cases[] = [
   ...
   +    $test_cases[] = [
   

   Let's not use $blah[] to auto-append, let's instead use strings as array keys. This results in human-readable error reporting if the test fails :)

1. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,31 @@
   +    $attributes['processed'] = $this->serializer->normalize($object->processed, $format, $context);
   

   A bigger thing I just thought of: this is not bubbling the necessary cacheability metadata.

   This is the first normalization case where that will actually be necessary.

   (For example: entity reference fields just contain the reference, they don't include that referenced entity in the response.)

   This is the first field whose data we send with REST EntityResource responses that is invalidated by certain cache tags and varies by certain cache contexts.

   So we need to figure out a way to bubble that up from the normalizer to the overall response.

2. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -0,0 +1,117 @@
   +   * @var \Symfony\Component\Serializer\Serializer
   

   Should typehint to the interface.

3. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -0,0 +1,117 @@
   +    $field_storage_config = FieldStorageConfig::create([
   ...
   +    $field_config = FieldConfig::create([
   

   Let's not assign these to a variable, because we don't use those variables anyway.

4. +++ b/core/modules/text/text.services.yml
   @@ -0,0 +1,7 @@
   +    # This normalizer needs to be the same or greater priority than serializer.normalizer.field_item.hal
   

   Needs two more leading spaces :)

Wouldn't the cacheability metadata be a problem with any response if we want to use this data for caching? All entity fields could be cached etc..? It could be a problem for entity references if someone added their own normalizer for an entity reference field I guess.

All entity fields could be cached etc..?

But until now, all fields are normalized to just the values that are stored in the entity. Hence the entity cache tag has been sufficient. To my knowledge this is the first example where the normalization depends on more than just the entity?

It could be a problem for entity references if someone added their own normalizer for an entity reference field I guess.

Yes.

#57 -1,2 - fixed
#58 - 2,3,4 - fixed
#58 - 1 tried a fix with @Wim Leers' help not working yet figure problem is in \Drupal\text\Normalizer\TextItemBaseNormalizer::bubble but have to a break from this.

1. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -2,17 +2,26 @@
   - * This class does not implement DenormalizerInterface because the 'proccessed'
   + * This class does not implement DenormalizerInterface because the 'processed'
   

   Hah, nice catch :)

2. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -2,17 +2,26 @@
   +  protected $renderer;
   
   @@ -20,12 +29,49 @@ class TextItemBaseNormalizer extends ComplexDataNormalizer {
   +  public function __construct(RendererInterface $renderer) {
   ...
   +  protected function bubble(CacheableDependencyInterface $object) {
   

   Let's put all of this in a trait.

3. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -20,12 +29,49 @@ class TextItemBaseNormalizer extends ComplexDataNormalizer {
   +   * Constructs a TextItemBaseNormalizer service.
   

   c/p remnant

4. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -20,12 +29,49 @@ class TextItemBaseNormalizer extends ComplexDataNormalizer {
   +      $build['#cache']['contexts'] = $object->getCacheContexts();
   +      $build['#cache']['tags'] = $object->getCacheTags();
   +      $build['#cache']['max-age'] = $object->getCacheMaxAge();
   

   Can be simplified to

   CacheableMetadata::createFromObject($object)->applyTo($build);
   

Now looking into the test failures.

Here's #61 applied to 8.2.x. We'll need separate patches for 8.2.x and 8.3.x because \Drupal\serialization\Tests\EntitySerializationTest was moved in 8.3.x.

This should have the same failures as #61.

Rebased on latest 8.2.x. Hopefully it'll apply this time.

The problem was that your unit test serialized the same entity object twice. But the first time $entity->FIELD->processed was accessed, the computed value was stored on the entity object (see \Drupal\text\TextProcessed::getValue()). And so any text format changes would not be honored.

The fix is simple: apply normalization to cloned entity objects, so that each normalization operates on a "fresh" entity object.

1. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -0,0 +1,160 @@
   +    $context = new RenderContext();
   +    $data = $this->container->get('renderer')
   +      ->executeInRenderContext($context, function () use ($entity) {
   +        return $this->serializer->normalize($entity);
   +
   +      });
   +    $this->assertEquals($expected, $data['field_text'][0]);
   

   This is not asserting that the expected cacheability metadata was bubbled.

   (This did not cause the test failure, but is an incompleteness in the test coverage.)

2. $filter_format = FilterFormat::load('plain_text');
   

   We should not hardcode this inline!

3. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,77 @@
   +class TextItemBaseNormalizer extends ComplexDataNormalizer {
   +  /**
   

   Needs a newline in between.

4. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,77 @@
   +    if (empty($value['format'])) {
   

   Is this really possible in the real world? I'd think this is only possible in this unit test scenario that you've got going?

5. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,77 @@
   +      // The default filter format.
   +      $filter_format_id = 'plain_text';
   

   This should most definitely not be hardcoded, it should be retrieved from configuration. Again, if its' even necessary.

6. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,77 @@
   +      $filter_format_id = $value['format'];
   
   +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -0,0 +1,160 @@
   +        $filter_format = FilterFormat::load('plain_text');
   

   We call these variables $text_format, not $filter_format.

7. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -0,0 +1,160 @@
   +      $context = new RenderContext();
   +
   +      $data = $this->container->get('renderer')
   +        ->executeInRenderContext($context, function () use ($entity) {
   +          return $this->serializer->normalize($entity);
   +
   +        });
   

   Spacing issues.

I only addressed points 1 & 2, the rest is up to tedbow.

Having looked at \Drupal\text\TextProcessed::getValue() due to #67, I came to realize something sad.

It does this:

$this->processed = check_markup($text, $item->format, $item->getLangcode());

And check_markup() does this:

function check_markup($text, $format_id = NULL, $langcode = '', $filter_types_to_skip = array()) {
  $build = array(
    '#type' => 'processed_text',
    '#text' => $text,
    '#format' => $format_id,
    '#filter_types_to_skip' => $filter_types_to_skip,
    '#langcode' => $langcode,
  );
  return \Drupal::service('renderer')->renderPlain($build);
}

In other words: the cacheability metadata of the filtering process itself is never added. Therefore this is still missing cache tags of e.g. referenced files processed by the \Drupal\editor\Plugin\Filter\EditorFileReference text filter.

We'll need to change TextItemBaseNormalizer to not merely access and normalize ->processed (which is what the patch does right now: $attributes['processed'] = $this->serializer->normalize($object->processed, $format, $context);), we will need to do something like check_markup(), but without rendering it in isolation (which is what renderPlain() does, we need it to render with bubbling! So we need TextItemBaseNormalizer::normalize() to do something like:

  $build = array(
    '#type' => 'processed_text',
    '#text' => $object->value,
    '#format' => $object->format,
    '#filter_types_to_skip' => [],
    '#langcode' => '',
  );
  $attributes['processed'] = $this->serializer->normalize($this->renderer->render($build), $format, $context);

re #68
4.

Is this really possible in the real world? I'd think this is only possible in this unit test scenario that you've got going?

Yes if it is plain text field will be empty here. But will default to the default filter in processing. I have step debugged to check this.

3, 5, 6, 7 fixed

will look into #69

1. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -49,7 +63,7 @@ public function normalize($object, $format = NULL, array $context = []) {
          // The default filter format.
   

   s/default/fallback/

2. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -36,7 +36,7 @@ class TextItemBaseNormalizerTest extends KernelTestBase {
   -  static $fallbackFormatId = 'plain_text';
   +  protected static $fallbackFormatId = 'plain_text';
   

   Better :)

#75.1 fixed.

While following along ... I notice a very small thing

we can specify that a method parameter should be an array.

#76: nice, thanks!

This still needs work for #69.

Fixed the fails in #76. Had to check if the filter actually loads before calling \Drupal\Core\Cache\CacheDependencyBubbleTrait::bubble

Addressed #69
$attributes['processed'] = $this->serializer->normalize($this->renderer->render($build), $format, $context);
Doesn't work because you will get the error.

Render context is empty, because render() was called outside of a renderRoot() or renderPlain() call. Use renderPlain()/renderRoot() or #lazy_builder/#pre_render instead.

Used
$attributes['processed'] = $this->serializer->normalize($this->renderer->renderPlain($build), $format, $context);
Add to fix \Drupal\Tests\serialization\Kernel\EntitySerializationTest
It was checking text processing using the format filter "full_format" but didn't actually create this FilterFormat.
Also it was using this test message

ComplexDataNormalizer produces expected array for $fieldName.

That is not accurate because not all fields are using ComplexDataNormalizer, text and entity reference for sure. So fixed that.
Also it was providing 'processed' when it was creating the entity to normalize. This value should not be provided on create but should be a result of normalization.

1. +++ b/core/modules/serialization/tests/src/Kernel/EntitySerializationTest.php
   @@ -134,7 +160,7 @@ public function testNormalize() {
   -      $this->assertEqual($expected[$fieldName], $normalized[$fieldName], "ComplexDataNormalizer produces expected array for $fieldName.");
   +      $this->assertEqual($normalized[$fieldName], $expected[$fieldName], "Field normalization produces expected array for $fieldName.");
   

   Instead of swapping the expected and the normalized variable, you could also just append an "s" to assertEqual and get the same effect :)

2. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,85 @@
   +      '#format' => $object->format,
   ...
   +    $value = $field_item->getValue();
   

   I'm wondering whether its guaranteed that the object has a format. I guess the rendering process applies a default value otherwise? It is also weird that we once fetch it via $object->format and once via $field_item->getValue()['format'], it would be nice to be consistent here.

3. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,85 @@
   +    /** @var \Drupal\text\Plugin\Field\FieldType\TextItemBase $field_item */
   +    $field_item = $object;
   

   You could name $object directly $field_item in the method itself, see https://3v4l.org/qN9IV

1. +++ b/core/modules/serialization/tests/src/Kernel/EntitySerializationTest.php
   @@ -60,6 +61,29 @@ protected function setUp() {
   +    // Create filter because it is needed text normalization.
   

   s/filter/text format/

   Also, broken English.

2. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -57,7 +57,14 @@ public function __construct(RendererInterface $renderer, ConfigFactoryInterface
   +    $build = array(
   

   s/array()/[]/

3. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -57,7 +57,14 @@ public function __construct(RendererInterface $renderer, ConfigFactoryInterface
   +    $attributes['processed'] = $this->serializer->normalize($this->renderer->renderPlain($build), $format, $context);
   

   This is wrong. This is still losing the bubbled cacheability metadata.

   You can either:

   1. use renderPlain() and then retrieve the cacheability metadata:
      

      …->renderPlain($build)…;
      $this->bubble(CacheableMetadata::createFromRenderArray($build));
      

   2. or, just use render() like I proposed, and do that in a render context. And a render context is in fact already provided by \Drupal\rest\RequestHandler::renderResponse(). This is also why that $this->bubble(…) call works.
      So when you said you were getting an error, I'd bet that you were getting that error only in tests… because you forgot to set up a render context there.

   Either way is fine. I think #1 may actually even be better because it's more explicit.

4. The fact that tests are passing despite the previous point means there's missing test coverage. None of the filters you're using are bubbling cacheability metadata. I suggest using \Drupal\filter_test\Plugin\Filter\FilterTestCacheTags.

5. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -68,7 +75,10 @@ public function normalize($object, $format = NULL, array $context = []) {
   +    if ($filter = FilterFormat::load($filter_format_id)) {
   

   $filter is a grossly misleading variable name here. It should be $text_format. $filter corresponds to \Drupal\filter\Plugin\FilterInterface, which is a very different thing.

@Wim Leers and @dawehner thanks for reviews

re #80
1. fixed.
2. I moved the logic to get the default format if needed above this and use that.
Changed to be more consistent.
3. fixed

re #81
1. fixed
2. fixed
3 & 4
I added your suggest about renderPlain
I also update the test to add the tags from \Drupal\filter_test\Plugin\Filter\FilterTestCacheTags::process and the required cache contexts for the contain paramater.

The test is passing.

5. fixed

Sorry interdiff was wrong.

1. +++ b/core/lib/Drupal/Core/Cache/CacheDependencyBubbleTrait.php
   @@ -0,0 +1,34 @@
   + * Provides bubble function to apply cache dependency to render context.
   ...
   +trait CacheDependencyBubbleTrait {
   

   "CacheDependency" makes no sense; the term is "CacheableDependency".

   I think ConditionalCacheabilityMetadataBubblingTrait would be a better name.

   Should also document when it is okay to use this. Something like:
   Should be used with great care. Should only be used by things that may be used both inside of a render context, and outside. For example: generating URLs for CLI vs for HTTP responses, serializing/normalizing data for scripts vs for HTTP responses, et cetera.

2. +++ b/core/lib/Drupal/Core/Cache/CacheDependencyBubbleTrait.php
   @@ -0,0 +1,34 @@
   +   * Bubbles the bubbleable metadata to the current render context.
   

   This is not bubbling bubbleable metadata (which implies attachments, per \Drupal\Core\Render\BubbleableMetadata), it's only bubbling cacheability metadata.

3. +++ b/core/lib/Drupal/Core/Cache/CacheDependencyBubbleTrait.php
   @@ -0,0 +1,34 @@
   +  protected function bubble(CacheableDependencyInterface $object) {
   +    if ($this->renderer()->hasRenderContext()) {
   +      $build = [];
   +      CacheableMetadata::createFromObject($object)->applyTo($build);
   +      $this->renderer()->render($build);
   +    }
   +  }
   

   This is equivalent to \Drupal\Core\Render\MetadataBubblingUrlGenerator::bubble() with one exception: this is bubbling only cacheability metadata, whereas \Drupal\Core\Render\MetadataBubblingUrlGenerator::bubble() is also bubbling attachments. That's an important difference.

4. +++ b/core/modules/serialization/tests/src/Kernel/EntitySerializationTest.php
   @@ -60,6 +61,29 @@ protected function setUp() {
   +    // Create a text format because it is needed for text normalization.
   

   s/text/TextItemBase/

5. +++ b/core/modules/serialization/tests/src/Kernel/EntitySerializationTest.php
   @@ -60,6 +61,29 @@ protected function setUp() {
   +    // @see \Drupal\text\Normalizer\TextItemBaseNormalizer::normalize().
   

   I think an @see \Drupal\text\Plugin\Field\FieldType\TextItemBase::propertyDefinitions() would actually be more appropriate.

6. +++ b/core/modules/serialization/tests/src/Kernel/EntitySerializationTest.php
   @@ -60,6 +61,29 @@ protected function setUp() {
   +      'format' => 'full_html',
   +      'name' => 'Full HTML',
   +      'filters' => [
   +        'filter_html' => [
   +          'module' => 'filter',
   +          'status' => TRUE,
   +          'weight' => 10,
   +          'settings' => [
   +            'allowed_html' => '<p>',
   +          ],
   +        ],
   +        'filter_autop' => [
   +          'module' => 'filter',
   +          'status' => TRUE,
   +          'weight' => 10,
   +          'settings' => [],
   +        ],
   

   This does not remotely look like the Full HTML text format. So why call it that? Let's give it a random name.

   'format' => $this->randomMachineName(),
   'name' => $this->randomString(),
   

7. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,86 @@
   +    $build =[
   

   s/=[/= [/

8. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,86 @@
   +      '#type' => 'processed_text',
   +      '#text' => $value['value'],
   +      '#format' => $filter_format_id,
   +      '#filter_types_to_skip' => [],
   +      '#langcode' => '',
   +    ];
   +    $attributes['processed'] = $this->serializer->normalize($this->renderer->renderPlain($build), $format, $context);
   +    $this->bubble(CacheableMetadata::createFromRenderArray($build));
   

   We're missing a comment that explains why we're doing all this and not just using $field_item->processed.

   I also think that moving the rendering logic to a protected helper method would make the code more understandable.

9. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,86 @@
   +    if ($text_format = FilterFormat::load($filter_format_id)) {
   +      $this->bubble($text_format);
   +    }
   

   We don't need to do this; \Drupal\filter\Element\ProcessedText::preRenderText() already does it for us. Hence line 79 will already contain those cache tags + contexts.

   (Because the filtered text, i.e. the output to be cached, does itself also depend on the text format config entity: if it changes, then the filtered output may need to change too.)

10. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
    @@ -0,0 +1,174 @@
    +      'format' => 'my_text_format',
    +      'name' => 'My text format',
    

    This is not random, but it's also not something that's identifiable/established. So this is fine.

11. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
    @@ -0,0 +1,174 @@
    +          'status' => TRUE,
    ...
    +          'status' => 1,
    ...
    +          'status' => TRUE,
    

    Let's be consistent.

12. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
    @@ -0,0 +1,174 @@
    +        'filter_test_cache_tags' => [
    

    Let's also add filter_test_cache_contexts.

13. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
    @@ -0,0 +1,174 @@
    +          'module' => 'filter',
    

    This is wrong, it's provided by the filter_test module.

    Also, you can just remove those keys. We don't have them in FilterAdminTest either, for example.

14. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
    @@ -0,0 +1,174 @@
    +    if ($text_format->id() == 'my_text_format') {
    +      // Add tags that are hardcoded FilterTestCacheTags::process().
    +      $tags = array_merge($tags, ['foo:bar', 'foo:baz']);
    +    }
    

    It doesn't make sense to conditionally add expectations. It's just a clear sign that these expectations belong in the data provider.

    Let's let the data provider return a CacheableMetadata object with the expectations.

1. fixed
2. fixed
3. added comment
4. fixed
5. I think TextItemBaseNormalizer::normalize is better because that is where you see that test won't work without the actual filter being created. Before this patch you could have pointed TextItemBase::propertyDefinitions() to but it was being normalized without the need for filter actually being created because it was handled by ComplexDataNormalizer.
6. Changed it to 'my_text_format because that is what other tests are using see 10.
7. fixed
8. created new function normalizeProcessed and added comment in there.
9. removed. thanks for the explination
10. ok
11. fixed
12. done

13. removed
14. Ok added to dataProvider. For others benefit you have to call addContexts not setContexts in dataprovider because setContexts checks for valid contexts which since Drupal is not available when the dataProvider function is called will always throw a not very help error.

Personally it would be great to answer #2626924-40: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata in the issue summary. Its maybe a question others will have as well.

+++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
@@ -0,0 +1,105 @@
+      $filter_format_id = $this->config->get('filter.settings')->get('fallback_format');

Should also add the filter.settings config cache key to it or do we not care about that?

Also, just putting it out there (haven't reviewed properly yet), it seems like a lot of this logic is happening in the nromalizer. This suggests to me that maybe this should be happening in the field item itself in some way instead? Then it could fix the problem for all other cases? This solution looks like it totally relies on global(ish) state or the renderer anyway.

Looking great! Very close.

1. +++ b/core/lib/Drupal/Core/Cache/ConditionalCacheabilityMetadataBubblingTrait.php
   @@ -0,0 +1,42 @@
   + * Provides bubble function to apply cache dependency to render context.
   

   Still says "cache dependency".

2. +++ b/core/lib/Drupal/Core/Cache/ConditionalCacheabilityMetadataBubblingTrait.php
   @@ -0,0 +1,42 @@
   +   * Bubbles the bubbleable cacheability metadata to the current render context.
   

   s/the bubbleable//

3. +++ b/core/lib/Drupal/Core/Cache/ConditionalCacheabilityMetadataBubblingTrait.php
   @@ -0,0 +1,42 @@
   +   *   The cacheable dependency object.
   

   s/The/A/

4. +++ b/core/lib/Drupal/Core/Cache/ConditionalCacheabilityMetadataBubblingTrait.php
   @@ -0,0 +1,42 @@
   +  /**
   +   * Gets the render service.
   +   *
   +   * @return \Drupal\Core\Render\RendererInterface
   +   *   The renderer.
   +   */
   +  protected function renderer() {
   +    return isset($this->renderer) ? $this->renderer : \Drupal::service('renderer');
   +  }
   

   This can simply be removed AFAICT.

5. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,105 @@
   +  protected $config;
   ...
   +    $this->config = $config_factory;
   

   Let's rename $this->config to $this->configFactory.

   There's 70 cases of that in core, and only one that does it like this code is currently doing it.

6. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,105 @@
   +    $attributes['processed'] = $this->normalizeProcessed($value, $format, $context);
   

   This is so much clearer!

   But I expected this to contain a $ths->serializer->normalize(…) call.

   And then for the helper method to only do the "generate 'processed' property's value" stuff. And for that helper method to receive $field_item, not $field_item->getValue(). Because then you can even typehint the helper method's parameter to TextItemBase $field_item :)

7. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,105 @@
   +      $filter_format_id = $this->config->get('filter.settings')->get('fallback_format');
   

   @dawehner is right in #87. We can fix that by changing this to:

   $filter_settings = $this->config->get('filter.settings');
   $this->bubble($filter_settings);
   $filter_format_id = $filter_settings->get('fallback_format');
   

8. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -0,0 +1,184 @@
   +    $cacheability = new BubbleableMetadata();
   

   s/$cacheability/$expected_cacheability/

9. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -0,0 +1,184 @@
   +    $cacheability = new CacheableMetadata();
   ...
   +      $cacheability,
   

   That creation of a new instance can just be moved into the array.

10. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
    @@ -0,0 +1,184 @@
    +    $test_cases['no_format_specified'] = [
    ...
    +    $test_cases['my_text_format'] = [
    

    You can have spaces in these array keys. It's meant to be a human-readable description.

11. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
    @@ -0,0 +1,184 @@
    +    $cacheability = new CacheableMetadata();
    +    // Add the tags for the 'filter_test_cache_tags' filter.
    +    $cacheability->setCacheTags(['foo:bar', 'foo:baz']);
    +    // Add the contexts for the 'filter_test_cache_contexts' filter.
    +    $cacheability->setCacheContexts(['languages:' . LanguageInterface::TYPE_CONTENT]);
    ...
    +      $cacheability,
    

    This one can also be moved into the array itself. That'd make this data provider a lot easier to read.

    (new CacheableMetadata())
      // comment
      ->setCacheTags(…)
      // comment
      ->setCacheContexts(…)
    

This suggests to me that maybe this should be happening in the field item itself in some way instead? Then it could fix the problem for all other cases? This solution looks like it totally relies on global(ish) state or the renderer anyway.

See #69 for a full explanation. A short addition to what I wrote there: the normalization system comes from Symfony, which has no concept of cacheability metadata. Drupal's normalization system is effectively identical to Symfony's — we didn't layer anything on top. Therefore, if we want our GET responses to be cacheable, then we need to bubble cacheability metadata in a way that's supported.

The only alternative is to modify \Drupal\text\TextProcessed::getValue(), so that it doesn't assign a \Drupal\Component\Render\MarkupInterface to $this->processed, but a value object containing a string, plus cacheability metadata. But that'd then be a BC break for anything that is already using the processed property of TextItemBase fields.

IOW: given the constraints (normalization system not designed for cacheability or BC break in TextProcessed), this is the only feasible solution that I see.

If you see an alternative solution, I'd love to hear about it :)

@dawehner thanks for review.
Yes adding $this->bubble($filter_settings);
@Wim Leers thanks for review
1-5 fixed
6. Chatted with @Wim Leers about this and helped me clear this up.

Resulted in return FilterProcessResult::createFromRenderArray($build)->setProcessedText($processed_text);
In helper function \Drupal\text\Normalizer\TextItemBaseNormalizer::normalizeProcessed

The problem is this seems to be the first call to FilterProcessResult::createFromRenderArray in core as far as I can tell.
This ultimitely calls \Drupal\Core\Cache\CacheableMetadata::createFromRenderArray
which contains $meta = new static(); so this throws an exception

Warning: Missing argument 1 for Drupal\filter\FilterProcessResult::__construct(), called in /var/www/d8_3_rest/core/lib/Drupal/Core/Cache/CacheableMetadata.php on line 150 and defined in Drupal\filter\FilterProcessResult->__construct() (line 80 of core/modules/filter/src/FilterProcessResult.php).

This fixed by making the argument to \Drupal\filter\FilterProcessResult::__construct optional.
I did this.

7. fixed
8. fixed
9. fixed
10. fixed
11. fixed

The only alternative is to modify \Drupal\text\TextProcessed::getValue(), so that it doesn't assign a \Drupal\Component\Render\MarkupInterface to $this->processed, but a value object containing a string, plus cacheability metadata. But that'd then be a BC break for anything that is already using the processed property of TextItemBase fields.

What I don't understand, why can't this new code at least call out to $item->processed and then extract the cacheable metadata from it?

What I don't understand, why can't this new code at least call out to $item->processed and then extract the cacheable metadata from it?

Again: because $item->processed contains a MarkupInterface object, which does not contain cacheability metadata. We could change that, but that'd break BC.

This looks almost ready to me. Just a few more nits/clarifications:

1. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,99 @@
   +   * Normalizes the processed attribute.
   ...
   +  protected function normalizeProcessed(TextItemBase $field_item) {
   

   This is not normalizing anything anymore. This is computing the processed attribute while retaining cacheability metadata.

   Also should get an @see \Drupal\text\TextProcessed::getValue() plus an explanation like the one in #90 to answer the question in #88/#92, which is important to have documented for sure.

2. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,99 @@
   +   * @return FilterProcessResult
   

   Needs FQCN.

3. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,99 @@
   +    // The rendering process needs to be applied to caching. New cacheability
   +    // metadata can be introduced in the process.
   +    // @see \Drupal\editor\Plugin\Filter\EditorFileReference::process().
   

   The first sentence makes no sense.

   I think all of this can be simplified to It's necessary to capture the cacheability metadata associated with the processed text. See https://www.drupal.org/node/2278483.

Again: because $item->processed contains a MarkupInterface object, which does not contain cacheability metadata. We could change that, but that'd break BC.

Sorry, that was not obvious for me. This though means we can create a render context, process the data and then extract the cacheability metadata out of that? I really try to let us have a normalizer which is minimal as possible. There is no reason to have logic in the normalizer, its not super special what this one tries to solve.

Sorry, that was not obvious for me.

Yep, totally understandable. Hence I repeated the key elements calmly :)

This though means we can create a render context, process the data and then extract the cacheability metadata out of that?

Yes. That's what \Drupal\rest\EventSubscriber\ResourceResponseSubscriber::renderResponseBody() does.

I really try to let us have a normalizer which is minimal as possible. There is no reason to have logic in the normalizer, its not super special what this one tries to solve.

+1! I'd love that.
But the unfortunate reality is that normalizers generate output that ends up in responses. Those responses need cacheability metadata. So the normalizers must bubble cacheability metadata. Hence the $this->bubble() call below. Unfortunately, \Drupal\text\TextProcessed::getValue() currently computes that property in a way that discards cacheability metadata and changing that would break BC. Hence the $processed_text = $this->normalizeProcessed($field_item); call below.

+  public function normalize($field_item, $format = NULL, array $context = []) {
+    $attributes = parent::normalize($field_item, $format, $context);
+    $processed_text = $this->normalizeProcessed($field_item);
+    $this->bubble($processed_text);
+    $attributes['processed'] = $this->serializer->normalize($processed_text->getProcessedText(), $format, $context);
+    return $attributes;
+  }

So given these constraints, do you see a better way to do this, or do you also think this is the only way?

1. fixed

plus an explanation like the one in #90 to answer the question in #88/#92, which is important to have documented for sure.

I did my best for an explanation but let me know what you think
2. fixed
3. fixed

Much better! :) Looks great/ready to me. Just a few final nits.

1. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -57,22 +57,31 @@ public function __construct(RendererInterface $renderer, ConfigFactoryInterface
   +   * Computes the processed attribute.
   

   s/processed/'processed'/

   s/attribute/property of TextItemBase fields/

   Add an

    * @see \Drupal\text\Plugin\Field\FieldType\TextItemBase::propertyDefinitions()
   

2. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -57,22 +57,31 @@ public function __construct(RendererInterface $renderer, ConfigFactoryInterface
   +   * The 'processed' field attribute for fields that extend TextItemBase will
   

   s/field attribute/property/

3. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -57,22 +57,31 @@ public function __construct(RendererInterface $renderer, ConfigFactoryInterface
   +   * not contain cacheability metadata. Therefore this function returns a
   

   will not contain cacheability metadata (see \Drupal\text\TextProcessed::getValue()).

4. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -57,22 +57,31 @@ public function __construct(RendererInterface $renderer, ConfigFactoryInterface
   +   * FilterProcessResult which can be used to get both the processed text and
   

   s/can be used/can carry/

5. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -57,22 +57,31 @@ public function __construct(RendererInterface $renderer, ConfigFactoryInterface
   +   * cacheable.
   

   s/cacheable/cached (and invalidated) correctly/

I'd RTBC this, but dawehner has indicated on Twitter that he wants to reply to #95, but has not yet found the time. Let's give him the time: https://twitter.com/da_wehner/status/801143016257949696.

Much better! :) Looks great/ready to me. Just a few final nits.

so close, I am like...

Ok. Here is a patch that addresses all the nits

:D

As I explained in #97, I'd RTBC, but this also needs the dawehner stamp of approval :)

So what I was thinking we could do instead is the following. Feel free to ignore me, just hacked that together in a minute.

As I explained in #97, I'd RTBC, but this also needs the dawehner stamp of approval :)

Sorry, I don't want to block anything :(

So we "kinda" discussed this via Twitter/GitHub gist. Copying that verbatim here, from https://gist.github.com/dawehner/283fb5ab0dcd0ceefc88df9afc84e0ec.

dawehner

So what I was thinking was creating a render context, execute $item->processed in there, and then call $this->bubble($render_context->bubble());

Wim Leers

What does

execute $item->processed

mean?
Either it exists, or it doesn't. When it doesn't exist, it's generated by \Drupal\text\TextProcessed::getValue(). When it does exist, it's a MarkupInterface instance, which doesn't allow cacheability metadata to be retrieved, nor does it allow "execution" of any way.

So, can you clarify?

dawehner

$Item->processed is using the fieldapi magic, sorry, I should have made this clear.
If you "call" $item->processed, its called out \Drupal\text\TextProcessed to retrieve its value: \Drupal\text\TextProcessed::getValue ... which calls out to check_markup, which renders it using ::renderPlain.

So in case we would create a render context around this magic here, wouldn't we be able to catch the cacheability metadata from the render context?

Wim Leers

Ah! Yes, that's possible. Except in the case of $item->processed already having been generated. Then the magic wouldn't happen again, and we wouldn't be able to catch the cacheability metadata.

So dawhner's idea could work. With the caveat that I outlined in #101: it's built only once, so if it was already built, then it wouldn't work. (caveat 1)

But there's another problem: ::renderPlain() prevents bubbling. IOW: once you call check_markup(), you have no chance at all to get at the cacheability metadata. That's very much intentional! Quoting the docs for check_markup():

/**
  * Runs all the enabled filters on a piece of text.
  *
  * …
  *
  * Note: this function should only be used when filtering text for use elsewhere
  * than on a rendered HTML page. If this is part of a HTML page, then a
  * renderable array with a #type 'processed_text' element should be used instead
  * of this, because that will allow cacheability metadata to be set and bubbled
  * up and attachments to be associated (assets, placeholders, etc.). In other
  * words: if you are presenting the filtered text in a HTML page, the only way
  * this will be presented correctly, is by using the 'processed_text' element.

i.e. it's specifically designed for use cases like sending e-mails. In fact, that's specifically the reason we kept it. See #2217877: Text filters should be able to add #attached, #post_render_cache, and cache tags, and the change record, which states: You should only use it in scenarios where you're not rendering HTML (e.g. a plain-text e-mail whose contents are processed text).
(caveat 2)

Third, I'm not at all certain it'd be okay to always bubble the cacheability metadata for the filtered/processed text from \Drupal\text\TextProcessed::getValue(). Because there's always a render context when responding to a route (because of \Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber). Doing this automatically could be more trouble than it's worth. That being said, we absolutely could change \Drupal\text\TextProcessed::getValue() to not bubble, but to at least contain a value object that contains cacheability metadata. A \Drupal\filter\FilterProcessResult would make perfect sense… because that's what we already use for this very purpose.

In other words: the only way we can have safe bubbling of cacheability metadata for $item->processed, is if it contains a value object that implements CacheableDependencyInterface. It currently contains a MarkupInterface object. Changing that would break BC.

I see two options:

1. To do this, we would not be able to use check_markup() anymore in \Drupal\text\TextProcessed::getValue(), instead we would need to move TextItemBaseNormalizer::computeProcessedAttribute() to TextProcessed. End result: $item->processed instanceof \Drupal\filter\FilterProcessResult === TRUE.
2. Update \Drupal\Core\Render\Markup to also implement CacheableDependencyInterface and the getters of AttachmentsInterface (we'd need to split up AttachmentsInterface to multiple interfaces). Then the Markup class would contain not just the HTML markup string, but also the associated cacheability metadata and attachments.

Either of those options would allow for this code in TextItemBaseNormalizer:

+  public function normalize($field_item, $format = NULL, array $context = []) {
+    $attributes = parent::normalize($field_item, $format, $context);
+    $this->bubble($field_item->processed);
+    $attributes['processed'] = $this->serializer->normalize((string)$field_item->processed, $format, $context);
+    return $attributes;
+  }

instead of the current patch's:

+  public function normalize($field_item, $format = NULL, array $context = []) {
+    $attributes = parent::normalize($field_item, $format, $context);
+    $processed_text = $this->normalizeProcessed($field_item);
+    $this->bubble($processed_text);
+    $attributes['processed'] = $this->serializer->normalize($processed_text->getProcessedText(), $format, $context);
+    return $attributes;
+  }

I like the sound of that last one. But it's a big undertaking, with a high probability of not succeeding.

So, I'm thinking we should indeed not block this on that. The implementation introduced here doesn't lock us down. In fact, as you can see from the above two code snippets, it would be absolutely trivial to start using this in the future :)

An alternative approach could be to introduceprocessed_filtered or so, which has the cacheable metadata attached, instead of bubbled up magically.

@dawehner: But "processed" implies "filtered". "To process" and "to filter" were (and still are) used as synonyms in the filter module. (Yes, that's confusing.) Also, it'd imply adding a new property to \Drupal\text\Plugin\Field\FieldType\TextItemBase::propertyDefinitions().

Also, it'd imply adding a new property to \Drupal\text\Plugin\Field\FieldType\TextItemBase::propertyDefinitions().
Sure this means that, but is that a problem? It is still a computed property.

It means a duplicate computed property. And adding a property is also a BC break I suspect?

It means a duplicate computed property. And adding a property is also a BC break I suspect?

I seriously cannot see how an additional computed property is a BC break.

This is looking good. It's kind of sad we have to involve so much of the render system here, in a normalizer.. but hey ho. Not the fault of this issue! Here are a couple of points:

1. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,109 @@
   +      $this->bubble($filter_settings);
   

   Why do we bubble the settings directly here when we'er using the fallback format but not in the case below when we have a format. I think the processed_text element handles that anyway? So this part can be removed - we just need the filter format ID?

2. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,109 @@
   +    // It's necessary to capture the cacheability metadata associated with the
   +    // processed text. See https://www.drupal.org/node/2278483.
   +    $processed_text = $this->renderer->renderPlain($build);
   +    return FilterProcessResult::createFromRenderArray($build)->setProcessedText($processed_text);
   

   This comment is misleading here I think, as renderPlain doesn't allow us to capture the metadata anyway? Do we need to get processed text here. Can't we get the metadata, then render it? Maybe in the normalize method'?

Initially I had similar thoughts to Daniel, with a new computed property when reading this issue through. @WimLeers what BC issues do you see with this approach? I could see that property just returning what we currently have the in the normalizer method and be done (same code and the TextItem already pretty much).

#108

1. Because we only consult the filter.settings configuration when the if (empty($value['format'])) { condition is met. We then use that configuration to look up the default text format ID.

   The end result is that we always end up with a text format ID, and use that to process the text using that text format. This results in \Drupal\filter\Element\ProcessedText::preRenderText() being called, which always associates the text format's cache tags. (See the end of that function.)

2. renderPlain() does not bubble the cacheability metadata, but it also doesn't throw it away: $build contains the bubbled cacheability metadata at $build['#cache'] (and the bubbled attachments at $build['#attached']).

   That means we can capture it from there and then bubble it manually, which is what this does.

#108.1 + #108.2 made me see one small detail that could be improved: the burden lies on the normalize() method to do explicit bubbling. This is great. But computeProcessedAttribute() still bubbles something in one case: #108.1. We should change that code to not do bubbling separately. And, in fact, \Drupal\filter\Element\ProcessedText::preRenderText() should handle this for us, because it already has logic to deal with fallback format. But turns out it's not bubbling the filter.settings config's cacheability metadata. So that's the real thing we should fix here.

Aha, 6 new failures! This makes sense — #98 was rolled before #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method landed. This is great! Because it's proving that the changes this patch is making to TextItemBase fields' normalizer is also affecting the output for any entity type that has such a field :) Which is exactly what we want of course.

Once you fix that, it complains that there is an unexpected processed property… because we're adding that here. Again, yay!

This reroll fixes the test expectations just for Comment entities. Leaving it to @tedbow to fix the others.

#108

Initially I had similar thoughts to Daniel, with a new computed property when reading this issue through. @WimLeers what BC issues do you see with this approach? I could see that property just returning what we currently have the in the normalizer method and be done (same code and the TextItem already pretty much).

The addition of a new property can break existing PHP code, because it doesn't expect this new property, since that's changing the data model. Whereas the approach in this patch is not breaking the data model, it's just fixing the normalization.

If we're going to add a new computed property, we might as well change \Drupal\text\TextProcessed::getValue() to not use check_markup(), and to change it to the logic used in \Drupal\text\Normalizer\TextItemBaseNormalizer::computeProcessedAttribute() instead.

Added changes to TermResourceTestBase for testing text processing

The addition of a new property can break existing PHP code, because it doesn't expect this new property, since that's changing the data model. Whereas the approach in this patch is not breaking the data model, it's just fixing the normalization.

If we're going to add a new computed property, we might as well change \Drupal\text\TextProcessed::getValue() to not use check_markup(), and to change it to the logic used in \Drupal\text\Normalizer\TextItemBaseNormalizer::computeProcessedAttribute() instead.

Well, let's be clear. The second alternative you proposed is worse, as it changes the behaviour of existing normal expected codepaths. My suggestion above is not touching those. Sure, you could access any object in PHP and set a random value on there, given that even adding protected variables to classes are BC breaks.

I think it would be though worth discussing concrete examples of broken code and then expand our BC policy to make things clearer.

1. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,101 @@
   +      '#filter_types_to_skip' => [],
   +      '#langcode' => '',
   

   These are the default values, so we could skip them.

2. +++ b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
   @@ -0,0 +1,101 @@
   +    $processed_text = $this->renderer->renderPlain($build);
   +    return FilterProcessResult::createFromRenderArray($build)->setProcessedText($processed_text);
   

   Its such a sad api, mutating our value object after construction time. Well it is not the fault of this patch for sure, but we should try to improve our APIs in those areas still

Let's see:

4 files changed, 41 insertions(+), 87 deletions(-)

Some temporary fixes ... I had on my local machine

This could fix a good amount of failures.

Funnily/ironically the failures here would be fixed by #2751325: All serialized values are strings, should be integers/booleans when appropriate I think :D

#127: heh, tagged it as a blocker at #2751325-143: All serialized values are strings, should be integers/booleans when appropriate.

#2751325: All serialized values are strings, should be integers/booleans when appropriate is in! Unblocked.

#124 doesn't apply. re-rolling

@tedbow
Did you got distracted :)

Drive-by re-roll. There was a conflict with parts of #2751325: All serialized values are strings, should be integers/booleans when appropriate which had same fixes for maybe same things. So I left those out of the re-roll. Hope I didn't miss something.

Ok fixed a couple tests.

\Drupal\Tests\hal\Kernel\NormalizeTest
This was taking into account "proccessed". Had to add a custom filter to use like in \Drupal\Tests\serialization\Kernel\EntitySerializationTest

EntitySerializationTest
Changed to use \Drupal\Component\Serialization\Json::encode instead of json_encode b/c there is difference in handle special characters and this what the serializer service will actually use.
also fixed a missing comma

\Drupal\Tests\text\Kernel\TextWithSummaryItemTest
In this test after the filter was set you actually have to reset the entity cache, save and reload the entity before the process text change will show up.
This not a test that was touched before in this issue so is this a change in behavior.
This seems reasonable but because the test passed before it seems you didn't have to before. Does that sound right?

All the other test failures from #134 seemed to from
$this->assertEquals($this->getExpectedCacheContexts(), empty($cache_contexts_header_value) ? [] : explode(' ', $cache_contexts_header_value));
...rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:408
b/c cache_contexts_header_value is returning the 'url.site' for Comments, Terms. There are also 2 test bases that in this patch updated getExpectedCacheContexts

This patch is kind of hard to come back to if you haven't seen it in a while. The IS is not at all helpful. We need the IS to be helpful for core committers.

This seems reasonable but because the test passed before it seems you didn't have to before. Does that sound right?

The fail was already present in #122. So it was introduced between #118 and #122, where @dawehner refactored how TextProcessed works.

I'm fine with continuing this direction.

1. +++ b/core/lib/Drupal/Core/Cache/ConditionalCacheabilityMetadataBubblingTrait.php
   @@ -0,0 +1,32 @@
   +trait ConditionalCacheabilityMetadataBubblingTrait {
   

   We should refactor this away just like we did at #2825812-66: ImageItem should have an "derivatives" computed property, to expose all image style URLs.

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
   @@ -64,6 +65,7 @@ protected function createEntity() {
   +      ->setDescription("It is a little known fact that llamas cannot count higher the seven.")
   
   @@ -93,8 +95,9 @@ protected function getExpectedNormalizedEntity() {
   -          'value' => NULL,
   +          'value' => 'It is a little known fact that llamas cannot count higher the seven.',
              'format' => NULL,
   +          'processed' => "<p>It is a little known fact that llamas cannot count higher the seven.</p>\n",
   

   This is effectively a functional test. Great!

   But shouldn't we add an explicit one? Let the text module subclass \Drupal\Tests\rest\Functional\EntityResource\EntityTest\EntityTestResourceTestBase, let it add a text field in its setUp(). Then assert that the text field is serialized as expected.

3. +++ b/core/modules/text/src/TextProcessedResult.php
   @@ -48,7 +49,17 @@ public function getValue() {
   -      $this->processed = check_markup($text, $item->format, $item->getLangcode());
   ...
   +        '#langcode' => '',
   

   We've lost the langcode.

4. +++ b/core/lib/Drupal/Core/Cache/ConditionalCacheabilityMetadataBubblingTrait.php
   @@ -0,0 +1,32 @@
   +trait ConditionalCacheabilityMetadataBubblingTrait {
   

   We should refactor this away just like we did for #2825812: ImageItem should have an "derivatives" computed property, to expose all image style URLs.

5. +++ b/core/lib/Drupal/Core/Cache/ConditionalCacheabilityMetadataBubblingTrait.php
   @@ -0,0 +1,32 @@
   +trait ConditionalCacheabilityMetadataBubblingTrait {
   

   We should refactor this away, just like we did for #2825812: ImageItem should have an "derivatives" computed property, to expose all image style URLs.

@Wim Leers thanks review. finally getting back this!
1. Yes! refactored. copied \Drupal\rest\EventSubscriber\ResourceResponseSubscriber directly from [#11955703]. Deleted ConditionalCacheabilityMetadataBubblingTrait
2. added an explicit test.
3. Added langcode here
4,5 @see item 1

Ok hopefully this will get the tests passing.

Most of the test fails in #139 where because the expected contexts were not correct. This was fixed by updates to :: getExpectedCacheContexts()

After that was fixed it exposed another tricker problem.
Basically that problem was that:

1. \Drupal\Tests\hal\Functional\EntityResource\HalEntityNormalizationTrait::applyHalFieldNormalization() was adding 'lang' key for all field values which was working except for the processed text fields.
2. The 'lang' key was being added at the end of the array but the $actual field values for these hal fields didn't have the 'lang' at the end of the array
3. because \Drupal\hal\Normalizer\TextItemBaseNormalizer::normalize() is adding 'processed' after it calls \Drupal\hal\Normalizer\FieldItemNormalizer::normalize().
4. So 'processed' is at the end of the array.

Without making applyHalFieldNormalization() much more complicated the order of the field keys is not going to match but that doesn't seem like something we should be testing for that I think

So I created \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::nestedKsort() to called that instead of ksort().

This fixes the problem but I am just realizing 1 performance problem is that we would be doing a nested sort not only in HAL but also json format when don't need to be.
I am sure that is worth the complexity it would add to solve it for just performance reasons.

The only other fail was the test normalizer in \Drupal\Tests\serialization\Kernel\FieldItemSerializationTest need to have a higher priority than the new Text item normalizer so it would still be used in the test.

So I created \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::nestedKsort() to called that instead of ksort().

One of the other rest or serialization patches is doing the same thing! (Difficult to find which one, d.o doesn't have "patch search" functionality.)

This fixes the problem but I am just realizing 1 performance problem is that we would be doing a nested sort not only in HAL but also json format when don't need to be.
I am sure that is worth the complexity it would add to solve it for just performance reasons.

Eh, I'm not remotely concerned about this. These arrays are not enormous. Who cares if it takes 0.01 seconds or 0.02 seconds.

The only other fail was the test normalizer in \Drupal\Tests\serialization\Kernel\FieldItemSerializationTest need to have a higher priority than the new Text item normalizer so it would still be used in the test.

Nice — it's great to see you updated the docs to reflect this. Although it'd be good to document why you choose that particular priority.

Time for another round of patch review — hopefully the last!

1. +++ b/core/modules/hal/hal.services.yml
   @@ -8,6 +8,11 @@ services:
   +      - { name: normalizer, priority: 20 }
   

   Let's document why we want priority 20.

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -1217,4 +1217,19 @@ protected function assertResourceNotAvailable(Url $url, array $request_options)
   +  protected function nestedKsort(&$array) {
   

   Let's make this static, to prove that this doesn't rely on any object (instance) data.

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestTextItemNormalizerTest.php
   @@ -0,0 +1,81 @@
   +        'value' => 'Cádiz is the oldest continuously inhabited city in Spain and a nice place to spend a Sunday with friends.',
   

   :)

4. +++ b/core/modules/text/src/TextProcessedResult.php
   @@ -0,0 +1,88 @@
   +class TextProcessedResult extends TypedData {
   

   Should we add a @todo that says this is deprecated and will be removed before 9.0.0, because this is what we'll make \Drupal\text\TextProcessed behave like?

5. +++ b/core/modules/text/tests/src/Kernel/Normalizer/TextItemBaseNormalizerTest.php
   @@ -0,0 +1,176 @@
   +class TextItemBaseNormalizerTest extends KernelTestBase {
   

   Ideally we would make this a unit test, but given the amount of things we'd need to mock, I think a kernel test is justified here.

6. Test coverage I asked for in #137 is present now.

1. added
2. fixed. But also wondering if this would be better in \Drupal\simpletest\AssertHelperTrait. It seems it could be useful in other situations. If not there then maybe \Drupal\Tests\rest\Functional\ResourceTestBase because at least it could be useful for other custom REST tests.
3. :)
4. Added @deprecated. Would it still need a @todo. But also I look at the class comments for these 2 classes and they are exactly the same.
I also noticed that there was old comment that mentioned "check_markup" so I also updated that since it is not being called anymore.
5. good we are in agreement

+++ b/core/modules/hal/hal.services.yml
@@ -12,6 +12,7 @@ services:
+      // The priority needs to higher than serializer.normalizer.field_item.hal.

Wow, how many tests can I break with just 1 line :(

#143.2: maybe, but adding this new helper method to that generic trait is out of scope here. Let's first see if there are other tests that need it. Premature abstraction and all that.
#145: :)

The IS was updated by @frob in #138 (thanks!). But it's still confusing. I updated the problem/motivation section. We still need the proposed resolution explained, and why this solution was chosen over others.

We also need a CR to make people aware that the processed property is now being normalized.

Once the IS is updated and CR is created, this is RTBC.

Finally, this is a contributed project blocker for both JSON API (#2775205: [PP-2] [FEATURE] Discus how to make JSON API use text input formats for long text fields) and GraphQL (#2690347: [PP-1] Expose rendered text fields (text+format)).

I think this method might be overly complicated
See: #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts

If that seems like good idea we should probably postpone this.

#2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts sure looks appealing. In theory we could commit the current patch and then remove the normalizers this patch adds when #2871591 happens. But we risk breaking BC then, so it's probably safer to postpone this until we reach consensus in #2871591.

Ok first off @frob sorry right now I am not getting your point and too tired to look back through this issue, long day. Will try to address later.
Or maybe this patch will help clear it.

These patches are to bring inline with #2871591-26: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts
There I have posted a patch that rips out all "TextItems" specific changes

Here 2626924-151-do-not-test.patch contains only "TextItems" specific changes
It would fail but can be used to look at what would need once #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts lands

2626924-152-including-2871591.patch as you might guess includes all changes need for both issues.
Hopefully should pass.

Setting to need review to see if it passes but still should probably postpone on #2871591

#151: NICE! The new patch is lovely: much easier to understand, because all the infrastructure work is offloaded to #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts. This issue then just needs to take advantage of that new infrastructure.

And as a triple bonus of this approach:

1. this new patch allows OpenAPI documentation for the normalization to be generated automatically
2. this new patch allows https://www.drupal.org/project/jsonapi to benefit automatically, without needing to duplicate the normalizer there
3. this new patch allows https://www.drupal.org/project/graphql to benefit automatically, without needing to duplicate custom logic there

Marking postponed. This is RTBC-worthy as soon as #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts is in!

I have some question about this issue and basically every other issue blocked by #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts

Can we work on the computed properties already, but skip the normalization step work ... and then once #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts is in, which seemed to be blocked by typed data resources, we can iterate on that.

Can we work on the computed properties already, but skip the normalization step work ..

+1, we need that first anyway. If you have the time to reroll this, I'll review! If not, no worries. Great point in any case :)

I'm totally happy to work on it for a bit.

Here is a quick reroll.

Now this removes the files which should not have to be touched.

#2910211: Allow computed exposed properties in ComplexData to support cacheability. was split off from #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts.

Ok starting this back up and updating the patch to work with the latest changes from #2910211-6: Allow computed exposed properties in ComplexData to support cacheability. which in turn is working off #2871591-133: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts

Great — it's working for Comment REST test coverage! But since we last worked on this, #2835845: EntityResource: Provide comprehensive test coverage for BlockContent entity also landed, and that contains a TextItem field too, and hence it needs to be updated. All test failures are coming from the BlockContent REST test coverage.

#2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts is in, #2910211: Allow computed exposed properties in ComplexData to support cacheability. is the last remaining blocker (was split off from #2871591 to make its scope narrower and help it land sooner), and that too is now RTBC!