Problem/Motivation

Looking at #2626924: [PP-1] Expose TextItems' "processed" property when normalizing

To accomplish this

  • We are adding 2 computed field properties to TextItemBase.
  • Then we also have to create 2 TextItemNormalizers, one for the Serialization module (default normalization) and one for the HAL module (HAL normalization).
  • Then of course we need the tests for the normalizers.

Then in #2825812: ImageItem normalizer should expose all public image style URLs which has similar problem of exposing extra read-only information to normalization
We accomplish this by:

  • Creating two ImageItemNormalizers , one for Serialization and one for Hal.
  • Then of course we need the tests for the normalizers.
  • But in this case we don't add the computed field properties ImageItem

You could see how this could get complicated very quickly as we find new information related to field items that we want to include in normalization.
This doesn't take into account contrib modules like JSON API that also do normalization. It would need to duplicate these normalizers.

If we want to do it in the manner of processed text above we would need at least 1 computed field property and then 2 normalizers each time(default normalization provided by the serialization module and HAL normalization for the hal module, with tests).

We could do it like the Image Style URLs which does still require 2 new normalizers but not the computed fields. The problem with this is then the adding of the image_styles in the normalization is completely unknown to the Typed Data API.
This makes automatically documenting what is in our REST responses(or JSON API) very difficult.
Indeed combining the Schemata module and OpenAPI you can start get pretty decent documentation for our REST resources. But this relies on converting Typed Data to a schema (e.g. JSON Schema).

But if we continue to add extra information in normalizers that aren't reflected in Typed Data at all, then Schemata or any other way we try to automatically document the schema for our resources will always be incomplete. :(

We can't rely on computed fields alone because \Drupal\jsonapi\Normalizer\FieldItemNormalizer uses \Drupal\serialization\Normalizer\ComplexDataNormalizer::normalize() which has

/** @var \Drupal\Core\TypedData\TypedDataInterface $field */
foreach ($object as $name => $field) {
  $attributes[$name] = $this->serializer->normalize($field, $format, $context);
}

Using foreach with anything that extends \Drupal\Core\Field\FieldItemBase will ultimately call \Drupal\Core\TypedData\Plugin\DataType\Map::getProperties() which doesn't include computed properties.

We could override this in FieldItemNormalizer to normalize all properties both computed and non-computed. But then we run into the problem that we are already using computed fields for things like the entity property in EntityReferenceItem. An we don't know what else contrib might be using for computed properties.

We could required each field type (each \Drupal\Core\Field\FieldItemInterface implementation) to specify which computed items should NOT be normalized but doing that now would be a BC break.

Proposed resolution

So in order to included only the computed fields we want in normalization we need to allow field types to opt in only specific computed fields.

We can do this by adding a new property, $export_computed_properties, to \Drupal\Core\Field\Annotation\FieldType.

This property would default to an empty array so no existing computed fields would be included by default.

Then in \Drupal\serialization\Normalizer\FieldItemNormalizer and \Drupal\serialization\Normalizer\FieldItemNormalizer (and maybe JSON API?) we call a shared ComputedPropertiesNormalizerTrait that will include the computed properties specified in $export_computed_properties for the field type.

Then field types like TextItem and ImageItem can create computed fields and just include that ones that should be included in any kind of export in $export_computed_properties.

This eliminates the need to make 2 new normalizers (not counting JSON API) for every case where we want to add extra information in the normalization.

It also means that since we will be using Typed Data and explicitly defining which computed properties will be in the normalization (or other exports) we easily/automatically document these in auto-generated REST (and JSON API) documentation.

Remaining tasks

Decide if this is good idea!

User interface changes

API changes

Data model changes

Comments

tedbow created an issue. See original summary.

tedbow’s picture

Status: Active » Needs review
FileSize
40.09 KB
16.67 KB

This patch tries this idea starting from #2626924-145: Expose TextItems' "processed" property when normalizing

It should still working for exposing processed text but doesn't require TextItem specific normalizers.

I have included an interdiff to show what can be removed from #2626924: Expose TextItems' "processed" property when normalizing but still get the normalization.

Presumably #2825812: ImageItem normalizer should expose all public image style URLs could also be modified to use this method without requiring ImageItem specific normalizers just adding a "images_styles" computed property.

tedbow’s picture

Title: Allow Field Types to specify computed properties that should be exported in serialization and other contexts. » Allow Field Types to specify computed properties that should be exported in normalization and other contexts.
Related issues: +#2825812: ImageItem normalizer should expose all public image style URLs, +#2626924: [PP-1] Expose TextItems' "processed" property when normalizing
Wim Leers’s picture

Title: Allow Field Types to specify computed properties that should be exported in normalization and other contexts. » Allow @FieldType plugins to specify computed properties that should be exported in normalization and other contexts.
Issue summary: View changes
Issue tags: +Entity Field API

Great write-up!

I added formatting and clarified some things. Please confirm that my changes are all correct, in other words please confirm that I did not misinterpret.

I personally think this approach makes a ton of sense. Especially after looking at the patch:

  1. +++ b/core/modules/hal/hal.services.yml
    @@ -8,12 +8,6 @@ services:
    -  serializer.normalizer.text_item.hal:
    
    +++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
    @@ -30,6 +33,8 @@ public function normalize($field_item, $format = NULL, array $context = []) {
    diff --git a/core/modules/hal/src/Normalizer/TextItemBaseNormalizer.php b/core/modules/hal/src/Normalizer/TextItemBaseNormalizer.php
    
    diff --git a/core/modules/hal/src/Normalizer/TextItemBaseNormalizer.php b/core/modules/hal/src/Normalizer/TextItemBaseNormalizer.php
    deleted file mode 100644
    
    +++ b/core/modules/serialization/tests/src/Kernel/EntitySerializationTest.php
    @@ -220,7 +220,7 @@ public function testSerialize() {
    diff --git a/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
    
    diff --git a/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php b/core/modules/text/src/Normalizer/TextItemBaseNormalizer.php
    deleted file mode 100644
    
    +++ /dev/null
    @@ -1,8 +0,0 @@
    -services:
    -  serializer.normalizer.text_item_base:
    

    Hurray!

  2. +++ b/core/modules/hal/tests/src/Kernel/NormalizeTest.php
    @@ -175,7 +175,7 @@ public function testNormalize() {
    -          'processed' => "<p>{$values['field_test_text']['value']}</p>",
    +          'process_result' => "<p>{$values['field_test_text']['value']}</p>",
    

    So this is the only difference in the original patch versus this one.

    And that's only because TextItemBase's processed property does not include cacheability metadata, so to not break BC, a new computed property had to be defined, and it has this slightly different name. Makes sense.

Status: Needs review » Needs work

The last submitted patch, 2: 2871591-2.patch, failed testing.

dawehner’s picture

Do

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

any reason for this to be public?

  1. +++ b/core/lib/Drupal/Core/Field/Annotation/FieldType.php
    @@ -91,4 +91,11 @@ class FieldType extends DataType {
    +   * @var array
    

    I guess its string[]?

  2. +++ b/core/modules/hal/hal.services.yml
    @@ -8,12 +8,6 @@ services:
           - { name: normalizer, priority: 10 }
    -  serializer.normalizer.text_item.hal:
    -    class: Drupal\hal\Normalizer\TextItemBaseNormalizer
    -    arguments: ['@renderer']
    -    tags:
    -      # The priority needs to higher than serializer.normalizer.field_item.hal.
    -      - { name: normalizer, priority: 20 }
       serializer.normalizer.field.hal:
         class: Drupal\hal\Normalizer\FieldNormalizer
    

    Nice!

  3. +++ b/core/modules/hal/tests/src/Kernel/NormalizeTest.php
    @@ -175,7 +175,7 @@ public function testNormalize() {
               'format' => $values['field_test_text']['format'],
    -          'processed' => "<p>{$values['field_test_text']['value']}</p>",
    +          'process_result' => "<p>{$values['field_test_text']['value']}</p>",
             ],
    
    +++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
    @@ -201,7 +201,7 @@ protected function getExpectedNormalizedEntity() {
               'value' => 'The name "llama" was adopted by European settlers from native Peruvians.',
               'format' => 'plain_text',
    -          'processed' => '<p>The name &quot;llama&quot; was adopted by European settlers from native Peruvians.</p>' . "\n",
    +          'process_result' => '<p>The name &quot;llama&quot; was adopted by European settlers from native Peruvians.</p>' . "\n",
             ],
    ...
         ];
    

    Oh, isn't that an API change? We could support that by using a hashmap instead of a list in the annotation.

  4. +++ b/core/modules/serialization/src/Normalizer/ComputedPropertiesNormalizerTrait.php
    @@ -0,0 +1,45 @@
    +  protected function getComputedAttributes(FieldItemInterface $field_item, $format, array $context) {
    ...
    +      $attribute = $this->serializer->normalize($property, $format, $context);
    ...
    +      if ($attribute instanceof CacheableDependencyInterface && isset($context['cacheability'])) {
    +        $context['cacheability']->addCacheableDependency($attribute);
    +
    +      }
    

    It feels like in an ideal world we would wrap the serializer to do this for us.

Wim Leers’s picture

#6.3: note that this is a change relative to the patch in #2825812: ImageItem normalizer should expose all public image style URLs. It's not a change relative to HEAD! HEAD doesn't output processed at all. This is what's in HEAD:

      'comment_body' => [
        [
          'value' => 'The name "llama" was adopted by European settlers from native Peruvians.',
          'format' => 'plain_text',
        ],
      ],
dawehner’s picture

#6.3: note that this is a change relative to the patch in #2825812: ImageItem normalizer should expose all public image style URLs. It's not a change relative to HEAD! HEAD doesn't output processed at all. This is what's in HEAD:

Ah good to know! Sorry for the confusion.

In general +1 for naming the field exactly like the computed property. This is IMHO much better.

amateescu’s picture

If the goal is to provide a way to document which computed properties are "special" for the Schemata module (which says it works with Typed Data), shouldn't this new annotation property be at the typed data level?

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

Instead of over-complicating things with this new class, why don't we send some 'return_as_object' setting to the existing class and do the FilterProcessResult dance based on that?