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: [PP-1] 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: [PP-1] 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?

tedbow’s picture

Status: Needs work » Needs review
FileSize
7.44 KB
39.83 KB

UPDATE: I changed my mind since this first part but wanted to leave in my thinking incase others think this makes more sense
@amateescu,

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?

The behaviour for the schemata module would be a benefit but is not the reason for the proposed change.

The issues mentioned in the summary:
#2626924: [PP-1] Expose TextItems' "processed" property when normalizing
#2825812: ImageItem normalizer should expose all public image style URLs

Are about exposing related information in the normalization process. This idea is to simplify the process but also make sure the related information is not lost to the type data aware system like Schemata but not only schemata.

I think the reason to do this on the field level and not the TypeData level is because the Field level is where the normalization level has been created to not expose all computed properties. And it makes sense not to expose all computed properties on expose all TypeData properties on fields.

Would there be other uses of TypedData properties outside of the field system where the default would be to not expose all computed properties?
I don't think so. But could be wrong.

I am not familiar with TypedData uses outside of the field system. One place I do see it being use is \Drupal\Core\Field\FieldConfigBase::getItemDefinition it doesn't a computed property as far as I can tell.

AFTER THINKING ON IT, CHANGED MY MIND
Ok after trying to see how these might be done on the TypedData level I think it could be good idea and doable
Or at the very least something to explore and to get feedback from those who know TypedData better than me.

This patch:

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

    Remove this.

  2. Adds \Drupal\Core\TypedData\ExposableDataDefinitionInterface
  3. Changes \Drupal\Core\TypedData\DataDefinition to implement ExposableDataDefinitionInterface
  4. Instead of using the annotation field types would use \Drupal\Core\TypedData\ExposableDataDefinitionInterface::setExposed() to specify properties that should be exposed.
  5. Updates \Drupal\serialization\Normalizer\ComputedPropertiesNormalizerTrait::getComputedAttributes() to look for properties that both computed and exposed.

Again the TextItem changes are still here to demonstrate that this is feasible. They should be removed it we decide this is whole issue is good idea.

For example of how to test a property as exposed see \Drupal\text\Plugin\Field\FieldType\TextItemBase::propertyDefinitions

$properties['process_result'] = DataDefinition::create('string')
      ->setLabel(t('Processed text (object)'))
      ->setDescription(t('The text with the text format applied.'))
      ->setComputed(TRUE)
      ->setClass(TextProcessedResult::class)
      ->setSetting('text source', 'value')
      ->setExposed(TRUE);

I am not sure about the backwards compatibility implications of 1 and 2 in the list above.

I think functionality it will be the same as using the annotations. I personally like this way better.
Thanks for the suggestion @amateescu!

Status: Needs review » Needs work

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

tedbow’s picture

Status: Needs work » Needs review
FileSize
6.94 KB
35.05 KB

Ok errors make sense

1) Drupal\Tests\serialization\Unit\Normalizer\EntityReferenceFieldItemNormalizerTest::testNormalize
Prophecy\Exception\Call\UnexpectedCallException: Method call:
- getProperties(true)
on Double\Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem\P11 was not expected, expected calls were:
- getIterator()
- get(exact("entity"))

\Drupal\serialization\Normalizer\ComputedPropertiesNormalizerTrait::getComputedAttributes() now calls \Drupal\Core\TypedData\ComplexDataInterface::getProperties(TRUE) to get all properties including computed ones to determine which ones are "exposed".

This caused 2 error in this same test.

Text.Drupal\Tests\text\Kernel\Normalizer\TextItemBaseNormalizerTest errors.
This test should have been removed because the covered class \Drupal\text\Normalizer\TextItemBaseNormalizer no longer exists.

There is no need to have normalizer for TextItems any more because this issue handles this in a more generic way.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/TypedData/ExposableDataDefinitionInterface.php
    @@ -0,0 +1,31 @@
    +/**
    + * Interface for defining exposed data values.
    + *
    + * Should only be set for computed values.
    + */
    +interface ExposableDataDefinitionInterface {
    

    I assume we won't commit this to 8.3.x. Given that I'm wondering whether we really need the new interface ... we can add methods to interface, especially when there is a clear base class (DataDefinition).

  2. +++ b/core/lib/Drupal/Core/TypedData/ExposableDataDefinitionInterface.php
    @@ -0,0 +1,31 @@
    +  /**
    +   * Sets the whether the data value should be exposed.
    +   *
    +   * @param bool $exposed
    +   *   Whether the data value is exposed.
    +   *
    +   * @return static
    +   *   The object itself for chaining.
    +   */
    +  public function setExposed($exposed);
    ...
    +   * Determines whether the data value is exposed.
    +   *
    +   * @return bool
    +   *   Whether the data value is exposed.
    +   */
    +  public function isExposed();
    

    IMHO we should explain what exposing means ... aka. it will be returned in the normalization process / REST

  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.',
    

    I like some knowledge transported via tests.

Wim Leers’s picture

#9: That's a very interesting point! Typed Data is the Single Source of Truth, that's how REST/JSON API/GraphQL can work, that's how Schemata/OpenAPI can work. And that's indeed the point of this issue: to not put extra logic in normalizers, but instead provide additional metadata. I think you're right in pointing out that this metadata does not belong in the @FieldType plugin annotation. Because in the end, a @FieldType is just a unit of abstraction, a unit of packaging. A unit/level of Typed Data. So why then put this metadata in that unit, rather than in the general system, which is the Typed Data API?
Great insight, thank you! :) In hindsight, it seems so obvious :D

(However, why don't we send some 'return_as_object' setting to the existing class and do the FilterProcessResult dance based on that? makes little sense to me, but maybe I'm misunderstanding this.)


#10 + #11:

  1. +++ b/core/lib/Drupal/Core/TypedData/ExposableDataDefinitionInterface.php
    @@ -0,0 +1,31 @@
    + * Interface for defining exposed data values.
    ...
    +interface ExposableDataDefinitionInterface {
    

    Is "exposed" the right term?

    I wonder if we want "normalizable", "serializable" or "exportable". Because this is really about exporting, normalizing, serializing Typed Data-based data structured for use in contexts other than PHP.

    REST is one example. But transporting this data via https://en.wikipedia.org/wiki/Protocol_Buffers for example would be another use case.

  2. +++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
    @@ -10,6 +11,8 @@
    +  use ComputedPropertiesNormalizerTrait;
    
    +++ b/core/modules/serialization/src/Normalizer/ComputedPropertiesNormalizerTrait.php
    @@ -0,0 +1,52 @@
    +/**
    + * Normalization methods for computed field properties.
    + */
    +trait ComputedPropertiesNormalizerTrait {
    
    +++ b/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
    @@ -11,6 +11,8 @@
    +  use ComputedPropertiesNormalizerTrait;
    
    @@ -54,4 +56,18 @@ protected function constructValue($data, $context) {
    +    $attributes = array_merge($attributes, $this->getComputedAttributes($object, $format, $context));
    

    Isn't this a misnomer at this point? It's no longer about computed properties alone…

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -411,9 +411,9 @@ public function testGet() {
    +    $this->nestedKsort($expected);
    ...
    +    $this->nestedKsort($actual);
    

    These could be simplified to static::nestedKsort()

  4. +++ b/core/modules/serialization/src/Normalizer/ComputedPropertiesNormalizerTrait.php
    @@ -0,0 +1,52 @@
    +      if ($data_definition->isComputed() && $data_definition instanceof ExposableDataDefinitionInterface) {
    +        if ($data_definition->isExposed()) {
    

    Rather than explicitly checking if computed fields/properties implement this interface and then also return TRUE, shouldn't we just be doing this for all fields/properties automatically?

    And shouldn't we ensure that all non-computed fields default to exposed === TRUE?

    Then there's no API change. There's just a need for computed fields/properties to explicitly opt in.

    So then:

    1. we iterate over all fields/properties automatically
    2. every non-computed field is opt-out (i.e. exposed by default)
    3. every computed field is opt-in (i.e. not exposed by default)

    That seems like a simple, elegant, non-BC breaking approach?

    (Of course, I may very well be forgetting about some things! In which case, I'd love to hear about that in the comments :))

Wim Leers’s picture

#13

  1. I like that. We've done similar things lately, for similar situations/cases. (I can't find links at the moment.)
  2. On the one hand, I agree, on the other, I disagree. It should not be tied to REST specifically. But it wouldn't heard to explicitly list that as an example use case. Also: this is why I think we may need a better name, see #14.1.
  3. :)
andypost’s picture

related issue shows that some complex properties does not fits in annotation

tedbow’s picture

Ok here is a patch that addresses some of the feedback
Re: #13

I assume we won't commit this to 8.3.x. Given that I'm wondering whether we really need the new interface ... we can add methods to interface, especially when there is a clear base class (DataDefinition).

UPDATE: I was wrong here see #21
After looking we don't have such a clear base class.

Interfaces that extend this interface
\Drupal\Core\TypedData\DataReferenceDefinitionInterface
\Drupal\Core\TypedData\ListDataDefinitionInterface
\Drupal\Core\TypedData\ComplexDataDefinitionInterface

I don't like the idea of introducing the new interface but just want to be clear it is will be more than just updating 1 base class.
If this change to DataDefinitionInterface would break 8 base classes in core is it going to be a headache in contrib and custom code?

#13.1 and #14.2
Yes exposed is probably not the right term. but this patch doesn't fix that.

#14.3
This part of the patch probably needs to be removed because it was need explicitly for TextItem processing.
@todo Remove all portions that were only needed for #2626924: [PP-1] Expose TextItems' "processed" property when normalizing
@todo Write tests that prove "exposed"(or whatever we end up calling them) properties are available in normalized output.

#14.4

And shouldn't we ensure that all non-computed fields default to exposed === TRUE?

Yes updated \Drupal\Core\TypedData\DataDefinition::isExposed() to return TRUE if it is not computed and 'exposed' has not been set.

Ok, if we really want this to be not specific to the normalized process then the normalizer should not be responsible for determining which properties are exposed.

This patch overrides getIterator() in FieldItemBase so that a loop like:

foreach ($field_item as $name => $property) {
...
}

Will always loop through exposed properties.
This simplifies the the normalization process.
Then:
Removed ComputedPropertiesNormalizerTrait
Add FieldItemPropertiesNormalizerTrait which can be used by both FieldItemNormalizers from json and hal_json normalizers.

UPDATE:Ok looking how this would affect JSON API. Since \Drupal\jsonapi\Normalizer\FieldItemNormalizer::normalize is also using

foreach ($field_item as $name => $property) {
...
}

Then they would automatically be getting these new "exposed" fields. From my discussions with e0ipso, I don't think this is inline with that modules assumptions at least the exposed properties in the related issues.

So will update this patch anyways but I don't think overriding getIterator() in FieldItemBase is the right approach.

So probably this would be up to FieldItemPropertiesNormalizerTrait loop over the properties.
Another idea would be to add getExposedProperties() to FieldItemInterface

tedbow’s picture

This patch removes overriding getIterator() in FieldItemBase.

Instead it adds \Drupal\serialization\Normalizer\FieldItemPropertiesNormalizerTrait::getExposedProperties.

Again if we did add isExposed() to DataDefinitionInterface this would be much simpler. But as I noted #17 it is not just as simple as updating 1 base class.

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

Status: Needs review » Needs work

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

tedbow’s picture

Status: Needs work » Needs review
FileSize
40.71 KB
6.79 KB

Test failure in #18 makes sense.

In EntityReferenceFieldItemNormalizerTest

$this->fieldItem->getProperties(TRUE)
      ->willReturn([])
      ->shouldBeCalled();

\Drupal\serialization\Normalizer\FieldItemPropertiesNormalizerTrait::getExposedProperties would rely on this returning all current none computed properties.

New patch which does:
1) If we want to introduce the concept of "exposed" properties in TypeData it should not be up the normalization system to determine which properties are exposed.

This patch adds \Drupal\Core\TypedData\ComplexDataInterface::getExposedProperties()

Since the doc says

Interface for complex data; i.e. data containing named and typed properties.

Everything that implements is going to need to now have the concept of "some of the my contain properties can be exposed"

2) Removes ExposableDataDefinitionInterface
I was wrong in #17

If this change to DataDefinitionInterface would break 8 base classes in core is it going to be a headache in contrib and custom code?

So adding isExposed and setExposed() to DataDefinitionInterface
Then creating ExposableDataDefinitionTrait to implement these methods.

Then ExposableDataDefinitionTrait just has to be used in:

  • \Drupal\Core\Field\FieldConfigBase
  • \Drupal\Core\TypedData\DataDefinition

Removing ExposableDataDefinitionInterface makes \Drupal\Core\TypedData\ComplexDataWithExposedPropertiesTrait::getExposedProperties() introduced above much simpler
Unit test will probably fail because of expected calls.

Status: Needs review » Needs work

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

dawehner’s picture

This patch adds \Drupal\Core\TypedData\ComplexDataInterface::getExposedProperties()

I am wondering whether this should actually be the behaviour if you iterate over the entries in a complex data?
We could then even drop the method again, but I'm just thinking out loud here.

Wim Leers’s picture

This sounds promising! I'll hold off on reviewing until the patch is green, unless you want me to review before then.

tedbow’s picture

Title: Allow @FieldType plugins to specify computed properties that should be exported in normalization and other contexts. » Allow ComplexData in TypedData to specify computed properties that should be exported in normalization and other contexts.
Status: Needs work » Needs review
FileSize
1.56 KB
41.1 KB

Ok this hopefully get the patch to green!(fixes EntityReferenceFieldItemNormalizerTest locally for me)

Also updating the title to reflect that this is no longer FieldType plugin specific.

Re #23

I am wondering whether this(meaning ComplexDataInterface::getExposedProperties()) should actually be the behaviour if you iterate over the entries in a complex data?
We could then even drop the method again, but I'm just thinking out loud here.

Meaning drop getExposedProperties() and just override getIterator() in base classes that implement ComplexDataInterface?

I think a problem would be that would be that we are using "exposed" to on computed properties that should be available when somehow exporting TypedData.

Presumably we are going to be using this process in the future for computed properties that should be "exposed" but where computing the fields could actually very expensive. So it seems making this the default behavior in getIterator() would be expensive

tedbow’s picture

The reason this interdiff is so huge is that this patch removes all change that were only needed for #2626924: [PP-1] Expose TextItems' "processed" property when normalizing

Those changes were only left in to prove exposed properties worked while I was working on this patch.

This patch also adds \Drupal\Tests\rest\Functional\EntityResource\EntityTest\EntityTestExposedPropertyNormalizerTest
Test the "exposed" properties normalization through the REST process.

There will need to be other tests.

I will update #2626924: [PP-1] Expose TextItems' "processed" property when normalizing with a patch that only has what is needed for "processed_result" property when using this patch.

Wim Leers’s picture

Component: serialization.module » typed data system
Issue tags: +API-First Initiative

#25:

Presumably we are going to be using this process in the future for computed properties that should be "exposed" but where computing the fields could actually very expensive. So it seems making this the default behavior in getIterator() would be expensive

This makes sense I think.

#26: Yay, a clean, focused patch!

There will need to be other tests.

For which aspects is test coverage missing then?


Patch review:

  1. +++ b/core/lib/Drupal/Core/TypedData/ComplexDataInterface.php
    @@ -102,4 +102,12 @@ public function toArray();
    +   * Gets exposed properties for the field item.
    

    This interface is not field-specific, so should not mention it.

    Look at the \Drupal\Core\TypedData\ComplexDataInterface::getProperties() docs for inspiration.

  2. +++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
    @@ -218,4 +218,23 @@ public function getConstraint($constraint_name);
    +  public function setExposed($exposed);
    

    Interestingly, DataDefinitionInterface does not contain setRequired() etc, in fact: this is the only setter, besides addConstraints().

    Yet \Drupal\Core\TypedData\DataDefinition does have a setRequired() method. So it doesn't live on the interface.

    I wonder why?

    Does that mean this method shouldn't be added to the interface either?

    (Perhaps the intent is that only getters are on the interface, no setters?)

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestExposedPropertyNormalizerTest.php
    @@ -0,0 +1,87 @@
    +    // If the exposed test field has not been created create it.
    

    Nit: You can leave this comment out; we don't have it at \Drupal\Tests\rest\Functional\EntityResource\Node\NodeResourceTestBase::createEntity() either

  4. +++ b/core/modules/serialization/src/Normalizer/FieldItemPropertiesNormalizerTrait.php
    @@ -0,0 +1,42 @@
    + * Normalization methods for computed field properties.
    

    Not just for computed field properties.

I think this really needs a review from a Typed Data maintainer (of which there's only one: fago), or at least an Entity/Field API maintainer.

Wim Leers’s picture

Issue tags: +blocker

Note that as of #2626924-151: [PP-1] Expose TextItems' "processed" property when normalizing, this is hard-blocking #2626924.

And soon it will likely also be a hard blocker for #2825812: ImageItem normalizer should expose all public image style URLs.

Wim Leers’s picture

Status: Needs review » Needs work
+++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
@@ -150,14 +158,25 @@ protected function renderResponseBody(Request $request, ResourceResponseInterfac
+        'cacheability' => new CacheableMetadata(),

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestExposedPropertyNormalizerTest.php
@@ -0,0 +1,87 @@
+class EntityTestExposedPropertyNormalizerTest extends EntityTestResourceTestBase {

+++ b/core/modules/system/tests/modules/entity_test/src/Plugin/DataType/ExposedStringData.php
@@ -0,0 +1,30 @@
+    return "Exposed! " . $string_value->getString();

Discussed with @tedbow.

The test coverage that's missing, is the cacheability metadata that these computed properties may have.

I think we can do that quite easily here: add a class similar to class FilterProcessResult extends BubbleableMetadata, which then carries this computed string plus its cacheability metadata (let's say it varies by the user cache context).

We can then test for that in the test, by overriding \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::getExpectedCacheContexts().

tedbow’s picture

Status: Needs work » Needs review
FileSize
10.25 KB
25.28 KB

@Wim Leers thanks for review!
#27
1. fixed
2. Yes I wondered this too. Didn't know if this was an oversight or intentional.
3. removed
4. Good point. So this also means that the whole trait should be changed from FieldItemPropertiesNormalizerTrait to ComplexDataPropertiesNormalizerTrait.

Further for the serialization module instead of invoking this in FieldItemNormalizer::normalize() this should be called in \Drupal\serialization\Normalizer\ComplexDataNormalizer::normalize() and FieldItemNormalizer no longer needs to override normalize().

For the HAL module there is no ComplexDataNormalizer so no change needed there.

This change doesn't break \Drupal\Tests\rest\Functional\EntityResource\EntityTest\EntityTestExposedPropertyNormalizerTest but it does break \Drupal\Tests\serialization\Unit\Normalizer\ComplexDataNormalizerTest but I think because the test is written.
The including \Drupal\Tests\serialization\Unit\Normalizer\TestComplexData doesn't implement the interface correctly. i.e. \Drupal\Tests\serialization\Unit\Normalizer\TestComplexData::getProperties returns no value so that breaks the trait.

Haven't fixed this test yet but lets see if it breaks anything else.

#28
Ok now testing this.

ExposedStringData is now returning the new \Drupal\entity_test\ComputedString which extends BubbleableMetadata.
Providing cache context and tags.

Status: Needs review » Needs work

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

tedbow’s picture

#30

Haven't fixed this test yet but let's see if it breaks anything else.

Well this is an interesting failure

Drupal\Tests\serialization\Unit\Normalizer\EntityNormalizerTest::testNormalize
Argument 1 passed to Drupal\serialization\Normalizer\ComplexDataNormalizer::normalizeProperties() must be an instance of Drupal\Core\TypedData\ComplexDataInterface, instance of Mock_ContentEntityBase_a58ad0c0 given, called in /var/www/html/core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php on line 30 and defined

Not what I expected.
So EntityNormalizerTest is causing a failure because it is calling the new ComplexDataPropertiesNormalizerTrait::normalizeProperties() but it is sending an object that doesn't implement ComplexDataInterface.

This is because what I did in point 4 in #30(from review in #27).

Further for the serialization module instead of invoking this in FieldItemNormalizer::normalize() this should be called in \Drupal\serialization\Normalizer\ComplexDataNormalizer::normalize() and FieldItemNormalizer no longer needs to override normalize().

So I was thinking that since the logic in ComplexDataPropertiesNormalizerTrait::normalizeProperties() worked with anything that implemented ComplexDataInterface I could just call this in ComplexDataNormalizer::normalize() because in ComplexDataNormalizer there is
protected $supportedInterfaceOrClass = 'Drupal\Core\TypedData\ComplexDataInterface';
So I thought that in ComplexDataNormalizer::normalize()

public function normalize($object, $format = NULL, array $context = []) {
  /** @var \Drupal\Core\TypedData\ComplexDataInterface $object */
  return $this->normalizeProperties($object, $format, $context);
}

$object would always implement ComplexDataInterface

Because \Drupal\serialization\Normalizer\FieldItemNormalizer extends \Drupal\serialization\Normalizer\ComplexDataNormalizer

FieldItemNormalizer has
protected $supportedInterfaceOrClass = FieldItemInterface::class;

And FieldItemInterface extends ComplexDataInterface

So I made the assumption that any normalizer that extended ComplexDataNormalizer would have a $supportedInterfaceOrClass that extended( or implemented) ComplexDataInterface

This is actually not the case and this causes the failure.

EntityNormalizer extends ComplexDataNormalizer

\EntityNormalizer::$supportedInterfaceOrClass = [EntityInterface::class];

EntityInterface does NOT implement ComplexDataInterface

So in EntityNormalizerTest::testNormalize() line 81 fails
$this->entityNormalizer->normalize($content_entity, 'test_format');

$content_entity does not implement ComplexDataInterface

EntityNormalizer doesn't override ComplexDataNormalizer::normalize()
So this causes a failure.

Whew! Is this confusing to anybody else?

Question 1: Given that why aren't all the Entity REST tests failing?

This is because none of the Entity REST tests are actually using EntityNormalizer directly. All the REST entity tests deal with entities that implement either ContentEntityInterface or ConfigEntityInterface.

So the entities are normalized with either ContentEntityNormalizer or ConfigEntityNormalizer
Both of these normalizers have overridden normalize() so ComplexDataNormalizer::normalize() is never called.

Question 2: Why was EntityNormalizer working before this patch?

This is because ComplexDataNormalizer::normalize() did this:

foreach ($object as $name => $field) {
   $attributes[$name] = $this->serializer->normalize($field, $format, $context);
}

So $object just had to implement Traversable

Moral of the story

Basically because the way we are extending normalizer classes for any normalizer in the Serializer module we cannot guarantee that $object sent its implementation of \Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize() will implement the interface specified in its $supportedInterfaceOrClass
Even though doc for $supportedInterfaceOrClass says

The interface or class that this Normalizer supports.

Weird.

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.48 KB
25.95 KB

So how should we fix the problem described in #32?

Option 1: Roll back changes to ComplexDataNormalizer::normalize()

This doesn't seem right because since amateescu pointed out #9 these exposed properties are more of TypedData thing. It has seemed to make more sense.

But moving the logic back to FieldItemNormalizer would work.

Option 2: Leave changes to ComplexDataNormalizer::normalize() and override normalize in EntityNormalizer

This would also make the tests pass because EntityNormalizer would be like ContentEntityNormalizer and not call ComplexDataNormalizer::normalize() with an $object that doesn't implement ComplexDataInterface

But if there were any contrib or custom normalizers that were like EntityNormalizer in that they extended ComplexDataNormalizer, did not override normalize() and their $supportedInterfaceOrClass did not implement ComplexDataInterface they would immediately break.

This seems like a BC break.

Option 3: In ComplexDataNormalizer::normalize() check if $object implements ComplexDataInterface

I am not huge fan of this but it seems like the only option that

  1. Doesn't break contrib normalizers that extend ComplexDataNormalizer
  2. Allows handling the concept "exposed" properties for other classes that implement ComplexDataInterface besides FieldItemInterface.

In addition we could use
We could use \Drupal\Core\Entity\Plugin\DataType\EntityAdapter::createFromEntity to create a TypedData object from an Entity when sent an Entity.
But since

  1. The object from createFromEntity will never have "exposed" and "non-exposed" properties and
  2. We still have to handle if some contrib normalizer is not sending EntityInterface or ComplexDataInterface object since the pattern we are demonstrating in core is $object has no relation to $supportedInterfaceOrClass

I am going to try option 3 in this patch.

Other ideas?

UPDATE:
chatted with @larowlan on IRC and he helped with extra info about EntityAdapter::createFromEntity and pointed out the Entities use to implement ComplexDataInterface early in the Drupal 8 development cycle(didn't know that). So that is probably why the normalizers are setup this way. Super helpful!

larowlan’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
new file mode 100644
index 0000000000..708e40f3a9

index 0000000000..708e40f3a9
--- /dev/null

+++ b/core/modules/serialization/src/Normalizer/ComplexDataPropertiesNormalizerTrait.php
@@ -0,0 +1,41 @@
+  protected function normalizeProperties(ComplexDataInterface $data, $format, array $context) {
...
+    foreach ($data->getExposedProperties() as $name => $property) {

Cause you're only calling ->getExposedProperties here, I think your solution may be to add a new interface, then move getExposedProperties from ComplexDataInterface to the new interface. Then have both ComplexDataInterface and EntityInterface extend it. Then the implementation in EntityInterface can do something different - but you're typehinting for the method

tedbow’s picture

Status: Needs work » Needs review
FileSize
2.26 KB
26.45 KB

@larowlan that solves the problem only for EntityInterface but doesn't solve

We still have to handle if some contrib normalizer is not sending EntityInterface or ComplexDataInterface object since the pattern we are demonstrating in core is $object has no relation to $supportedInterfaceOrClass

So any contrib or custom normalizer that followed the pattern that core provided, i.e. extending ComplexDataNormalizer for a $supportedInterfaceOrClass that does not extend will have their normalizer immediately break.
I am sure this something we offer BC for but that feels like bad DX.

Also adding getExposedProperties to when we don't actually need the concept of exposed/not exposed properties on Entities right now therefore adding complication that it is not necessary.

If BC break for custom/contrib normalizers is not concern then we should just override normalize in EntityNormalizer. I would be as simple as

public function normalize($object, $format = NULL, array $context = []) {
    /** @var \Drupal\Core\Entity\EntityInterface $object */
    return parent::normalize(EntityAdapter::createFromEntity($object), $format, $context);
}

But then I look at ComplexDataNormalizer and it is only overriding normalize() and setting $supportedInterfaceOrClass from NormalizerBase.

So why not have EntityNormalizer extend NormalizerBase directly. Entities are not complex data in the TypedData sense(though I guess they were at one point)

Then we would just have to override normalize() in EntityNormalizer and do what ComplexDataNormalizer::normalize() did before this patch.

Then only class that will extending ComplexDataNormalizer directly FieldItemNormalizer which actually sets $supportedInterfaceOrClass to an interface that extends ComplexDataInterface.

Updating the patch for this.

UPDATE:
Just noticed the doc for ComplexDataNormalizer says


/**
 * Converts the Drupal entity object structures to a normalized array.
 *
 * This is the default Normalizer for entities....

So that is *very* wrong, but guess probably comes from the history of Entities once being and implementation of TypeData.

mradcliffe’s picture

But if there were any contrib or custom normalizers that were like EntityNormalizer in that they extended ComplexDataNormalizer, did not override normalize() and their $supportedInterfaceOrClass did not implement ComplexDataInterface they would immediately break.

This seems like a BC break.

Explaining my motivations as a contrib developer:

I have one contrib normalizer that extends ComplexDataNormalizer so that I can use its normalize method. My $supportedInterfaceOrClass doesn't extend ComplexDataInterface itself, but all the classes that implement it extend Map at some point so I think I would be okay?

I have another normalizer that extends ComplexDataNormalizer but targets my custom sub class of ItemList. I should probably just change that to ListNormalizer. I think I felt it was easier to be able to iterate over the list items and call parent::normalize. I liked that better than calling the serializer property's normalize method. Looking at things again I don't think there's a big performance hit there like I thought.

So in my case I think I would be okay with a BC break here, but I don't know about other contrib. that would be in the same spot AND be providing custom stuff. The burden would probably be mostly on Drupal site builders who may be using a custom module developed by someone else.

Status: Needs review » Needs work

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

tedbow’s picture

@mradcliffe thanks offering a real contrib example/experience.

My $supportedInterfaceOrClass doesn't extend ComplexDataInterface itself, but all the classes that implement it extend Map at some point so I think I would be okay?

Depends with which way we go.

  1. Patch in #33 I think you be ok. Because it checks for ComplexDataInterface and handles $object not implementing it with previous logic.
  2. "Option 1" in #33 would work because ComplexDataNormalizer would be unchanged. The big downside is that only data that implements FieldItemInterface would actually get the benefit the new "exposed" properties in the normalization.
  3. Suggest in #34 would NOT work because your $object would not implement ComplexDataInterface or EntityInterface
  4. Patch in #35 would NOT work because ComplexDataNormalizer would throw an error because your $object would not implement ComplexDataInterface

So in my case I think I would be okay with a BC break here, but I don't know about other contrib. that would be in the same spot AND be providing custom stuff. The burden would probably be mostly on Drupal site builders who may be using a custom module developed by someone else.

(emphasis added)

I think also it would be on anyone that upgraded to 8.4.0(if this gets in) who is using a contrib module that does what you are describing where the module maintainer has done what you have done(which core has not suggested that you not do) and has not updated their normalizers.

I see you help maintain the TypedData documentation so I guessing you are more likely to find this issue and notice a change record than the average contrib maintainer ;)

So my guess is patch in #33 is the safest with maybe the logic handling non ComplexDataInterface objects being marked as deprecated if decide it should be.

Other follow updates would be:

  1. Update ComplexDataNormalizer phpdoc because it shouldn't refer to entities
  2. Update EntityNormalizer to extend Normalizer base directly as in #35
  3. Decide if it matters B
    asically because the way we are extending normalizer classes for any normalizer in the Serializer module we cannot guarantee that $object sent its implementation of \Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize() will implement the interface specified in its $supportedInterfaceOrClass
    Even though doc for $supportedInterfaceOrClass says "The interface or class that this Normalizer supports."
dawehner’s picture

asically because the way we are extending normalizer classes for any normalizer in the Serializer module we cannot guarantee that $object sent its implementation of \Symfony\Component\Serializer\Normalizer\NormalizerInterface::normalize() will implement the interface specified in its $supportedInterfaceOrClass
Even though doc for $supportedInterfaceOrClass says "The interface or class that this Normalizer supports."

Well, for me everything which is doing that, basically introduces a big in their code. I think this is one of those signs where we mixed up what single responsibility principle means. Its not about code sharing, its about having logical abstractions. Of course there is nothing to change here.

  1. +++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
    @@ -62,4 +62,15 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
    +    foreach ($object as $name => $field) {
    

    Note: This just works because \Drupal\Core\Entity\ContentEntityInterface extends \Traversable. This is not necessarily true for generic entities. To be honest there is not much we can actually do about it.

  2. +++ b/core/modules/serialization/src/Normalizer/EntityNormalizer.php
    @@ -62,4 +62,15 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
    diff --git a/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php b/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
    
    diff --git a/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php b/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
    index decca43227..c2f62f1d59 100644
    
    index decca43227..c2f62f1d59 100644
    --- a/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
    
    --- a/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
    +++ b/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
    
    +++ b/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
    +++ b/core/modules/serialization/src/Normalizer/FieldItemNormalizer.php
    @@ -11,6 +11,8 @@
    
    @@ -11,6 +11,8 @@
      */
     class FieldItemNormalizer extends ComplexDataNormalizer implements DenormalizerInterface {
     
    +  use ComplexDataPropertiesNormalizerTrait;
    +
       /**
        * {@inheritdoc}
        */
    

    I think we don't actually need this use statement, right?

tedbow’s picture

Status: Needs work » Needs review
FileSize
25.39 KB

Ok this is #33 rerolled plus #39.2

#35 will not work because of reasons mentioned in #38 and #33 appears to be the safest.

We can't assume that $object sent to \Drupal\serialization\Normalizer\ComplexDataNormalizer::normalize is implements ComplexDataInterface.

Status: Needs review » Needs work

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

Wim Leers’s picture

Let's get this moving forward again?