### Problem/Motivation

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: [PP-1] 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 CommentFileSizeAuthor #7033.21 KBtedbow #7016.26 KBtedbow #6637.31 KBtedbow #669 KBtedbow #5331.87 KBtedbow #536.3 KBtedbow #50520 bytestedbow #5026.65 KBtedbow #4427.38 KBWim Leers #43878 bytesWim Leers #4025.39 KBtedbow #3526.45 KBtedbow #352.26 KBtedbow #3325.95 KBtedbow #331.48 KBtedbow #3025.28 KBtedbow #3010.25 KBtedbow #2629.91 KBtedbow #2623.26 KBtedbow #2541.1 KBtedbow #251.56 KBtedbow #216.79 KBtedbow #2140.71 KBtedbow #1835.84 KBtedbow #183.67 KBtedbow #1736.33 KBtedbow #179.8 KBtedbow #17136.67 KBtedbow #1235.05 KBtedbow #126.94 KBtedbow #1039.83 KBtedbow #107.44 KBtedbow #216.67 KBtedbow #240.09 KBtedbow Members fund testing for the Drupal project. Learn more ## Comments tedbow created an issue. See original summary.  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: [PP-1] 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.  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: [PP-1] ImageItem normalizer should expose all public image style URLs, +#2626924: [PP-1] Expose TextItems' "processed" property when normalizing  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.

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.

#6.3: note that this is a change relative to the patch in #2825812: [PP-1] 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',
],
],

#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.

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?

 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.

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.

 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.

@@ -0,0 +1,31 @@
+/**
+ * Interface for defining exposed data values.
+ *
+ * Should only be set for computed values.
+ */

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).

@@ -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.

#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:

@@ -0,0 +1,31 @@
+ * Interface for defining exposed data values.
...

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 :)) #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. :)  Related issues: +#2876686: [PP-1] Normalization of LinkItem's 'options' property related issue shows that some complex properties does not fits in annotation  Issue summary: View changes FileSize 136.67 KB 9.8 KB 36.33 KB 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  Issue tags: +typed data FileSize 3.67 KB 35.84 KB 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.  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.

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"

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?

Then creating ExposableDataDefinitionTrait to implement these methods.

Then ExposableDataDefinitionTrait just has to be used in:

• \Drupal\Core\Field\FieldConfigBase

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.

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.

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

 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

FileSize
23.26 KB
29.91 KB

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.

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.

 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.

@@ -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.

 Issue tags: +blocker
 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().

 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.

#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

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.

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!

 Status: Needs review » Needs work
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

 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.

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.

@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.

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.

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." 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?  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.

Let's get this moving forward again?

 Status: Needs work » Needs review
FileSize
878 bytes

So let's get this going again.

This fixes one of the two failures. The other failure requires \Drupal\Tests\serialization\Unit\Normalizer\TestComplexData::getProperties() to be fixed. @tedbow is best placed to fix that.

FileSize
27.38 KB

FFS d.o.

+++ b/core/lib/Drupal/Core/TypedData/ComplexDataInterface.php
@@ -102,4 +102,12 @@ public function toArray();
+  /**
+   * Gets an array of property objects for exposed properties.
+   *
+   * @return \Drupal\Core\TypedData\TypedDataInterface[]
+   *   The exposed properties.
+   */
+  public function getExposedProperties();

While I dislike adding something that is onely used for Rest to the generic ComplexDataInterface, I don't really see a way around it. However, it should be thoroughly documented what "exposed" in this case means and what it doesn't mean.

On a more general note, I'm not sure this is actually needed. The issue summary lists #2626924: [PP-1] Expose TextItems' "processed" property when normalizing and #2825812: [PP-1] ImageItem normalizer should expose all public image style URLs as examples of where we want to include computed properties as part of normalized output. However, is there ever a case where we do not want that? At least by default I mean. Field types can always specify dedicated normalizers, but this issue exists to try to avoid having to do that just because you have a computed property. So I am wondering whether it wouldn't make sense to just include computed properties by default and be done with it (I guess controlled by a bc_* config value, but...). Entity reference fields have a computed entity property that could be problematic but entity references need special handling anyway (proven by the fact that both Serialization and HAL modules provide dedicated normalizers), so I don't think that's a particularly strong argument against this. (And in some cases it can even be pretty neat to have the serialized referenced entity as part of the normalized output.) Thoughts?

 Status: Needs review » Needs work

The last submitted patch, 44: 2871591-43.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

@tstoeckler note that #2626924: [PP-1] Expose TextItems' "processed" property when normalizing is adding a new process_result computed property, because the existing processed computed property cannot be used due to it not containing the necessary cacheability metadata. Cacheability metadata is necessary for REST/JSON API/GraphQL responses to be cacheable.
So I'm afraid that even just that means that your proposal doesn't work. (And auto-detecting whether the value contained in a property implements CacheableDependencyInterface or not also doesn't work, because the default NULL value for empty non-required fields would then still get in the way.

Furthermore, e.g. LanguageItem's language property is computed but we wouldn't want that one — it only makes sense for internal (PHP) use. Exactly the same is true for DateTimeItem's date computed property and DateRangeItem's start_date and end_date computed properties.

In other words: there's quite a few examples of "convenience" computed properties that can only ever make sense in a PHP context. We need to somehow figure out which those are.

You could read getExposedProperties() as getNormalizableProperties() or getPropertiesThatMakeSenseOutsideOfPhp().

@tstoeckler re #45

So I am wondering whether it wouldn't make sense to just include computed properties by default and be done with it (I guess controlled by a bc_* config value, but...). Entity reference fields have a computed entity property that could be problematic but entity references need special handling anyway

I think the problem would be if this was default any field item in contrib or custom code would automatically pick up this behavior. @Wim Leers pointed out a couple problems in core but we have no idea how this would effect contrib and custom field types because we don't know what computed properties they would be using.

 Status: Needs work » Needs review

Ok at least some the remaining errors are from changes in #2846554: Make the PathItem field type actually computed and auto-load stored aliases

Basically \Drupal\path\Plugin\Field\FieldType\PathItem::ensureLoaded() was added to make sure that all the properties were loaded when try to get any of the properties. But get() was not overridden so ensureLoaded() was not called.

This patch overrides get() and calls ensureLoaded().

FileSize
26.65 KB
520 bytes

Not sure what happened there.

Patch attached

 Status: Needs review » Needs work

The last submitted patch, 50: 2871591-49.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Not quite convinced by #47/#48. I just did the following change on a clean 8.4.x:

--- a/core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php
+++ b/core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php
@@ -27,7 +27,7 @@ class ComplexDataNormalizer extends NormalizerBase {
public function normalize($object,$format = NULL, array $context = []) {$attributes = [];
/** @var \Drupal\Core\TypedData\TypedDataInterface $field */ - foreach ($object as $name =>$field) {
+    foreach ($object->getProperties(TRUE) as$name => $field) {$attributes[$name] =$this->serializer->normalize($field,$format, $context); } return$attributes;

And did a Rest GET request before and after the patch. This is the resulting diff of the normalized output:

@@ -16,12 +16,14 @@
],
"langcode": [
{
-      "value": "en"
+      "value": "en",
+      "language": {}
}
],
"type": [
{
"target_id": "article",
+      "entity": {},
"target_type": "node_type",
"target_uuid": "f3c3f83c-e5b4-4249-a659-5bc61d98414d"
}
@@ -35,6 +37,7 @@
"revision_uid": [
{
"target_id": 1,
+      "entity": {},
"target_type": "user",
"target_uuid": "91374fea-8e74-4ac3-b7f5-bf2c8c7aa9df",
"url": "\/user\/1"
@@ -54,6 +57,7 @@
"uid": [
{
"target_id": 1,
+      "entity": {},
"target_type": "user",
"target_uuid": "91374fea-8e74-4ac3-b7f5-bf2c8c7aa9df",
"url": "\/user\/1"
@@ -102,14 +106,17 @@
"field_date": [
{
-      "value": "2017-12-31T22:59:59"
+      "value": "2017-12-31T22:59:59",
+      "date": {}
}
],
"body": [
{
"format": "basic_html",
-      "summary": ""
+      "summary": "",
+      "summary_processed": ""
}
],
"comment": [

Surely the added information is not necessarily semantically sensible, but our Rest output is super verbose regardless. So basically we are adding knowledge about Rest to the core typed data system, to avoid this bit of ugliness. It is legitimately pretty ugly, so I'm not actually 100% sure that that's a bad trade-off, but I do want to make sure that everyone is aware of the trade-off we are making.

(And yes, #2626924: [PP-1] Expose TextItems' "processed" property when normalizing will make that even a tad uglier, because of an additional, seemingly duplicated property, but the fact that text fields lose cacheability metadata is a bug anyway (regardless of Rest, it's just greatly exacerbated by Rest) and is neither Typed Data's nor Rest's fault, so I don't think it's a strong argument in either direction.)

 Status: Needs work » Needs review
FileSize
6.3 KB
31.87 KB

This patch fixes ComplexDataNormalizerTest for new logic.

It adds a new test function for the case where ComplexDataNormalizer is extended and the extending class has $supportedInterfaceOrClass that does not extend ComplexDataInterface as was described in real contrib situation in #36 by @mradcliffe(thanks again). Re @tstoeckler comments #45 and #52 We can't just consider how automatically including computed properties would affect core field types. I think for BC we have to consider that contrib or custom code will have field types that have computed properties... Regardless of REST this will change normalized output for any entities that use these fields. Also what happens if there is no normalizers that support the computed properties? Right now that is not problem and contrib modules defining field types with computed properties don't have to worry about these properties going through the normalization process. If we start to just run all computed properties through the normalization process I fear this could have unforeseen ramifications and expectations. As stated in the issue summary 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. And 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. Well for BC we could add (yet another) config setting to control this behavior, so I don't think that's a strong argument. I think we should consider how this affects fresh installs and for those I don't see how this breaks anything - it's just ugly. You're right, it would definitely need a bit more testing with contrib, but e.g. entity reference's entity property that the issue summary cites doesn't cause any problems. Again, I don't want to shoot down the current patch I just don't feel that alternatives have been discussed sufficiently (and that hasn't changed with the latest comments). ok another reason to put it in typed data. from @Wim Leers' comment on #14 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. Putting the BC concerns aside: If we export every computed property by default but then know that certain normalizers such as entity reference will override this behavior it will be super hard(impossible?) for any system like Schemata to know what the data coming out of Drupal will ultimately look like. Whether the Schemata module lives on the problem remains how do we automatically information of what will be export from Drupal for REST, JSON API or other modules. If we move this to normalization process and just allow any particular normalizers to choose this what computed properties to include it would very opaque to any systems outside of Drupal. Below is a brainstorm of how this might be possible: If we move this to the normalization process then to get the same effect, to know what exported data properties would available for any entity, we would have to: 1. Add a new public function getExposedPropertes() to ComplexDataNormalizer 2. In ComplexDataNormalizer::getExposedPropertes() return all exposed properties(if that is default behavior) 3. Any Normalizer that wants to override behavior would override getExposedPropertes() and return the correct properties. 4. Then Schemata/OpenAPI(or other similar modules) would have to figure out which normalizer would be used for a particular Complex data type. 5. Then call getExposedPropertes() for the normalizer. 6. But todo the 2 steps above you couldn't use \Symfony\Component\Serializer\Serializer::getNormalizer() because that expects a$data object and when you are doing stuff like making auto generate documentation you actually don't have a data object. You just know that type.
7. Furthermore if you made an alterative to Serializer::getNormalizer() that didn't require a data object of the type then you also NOTcould use NormalizerInterface::supportsNormalization() because it also expects a data object.
8. NormalizerBase::$supportedInterfaceOrClass is protected so to get information we would have to add something like NormalizerBase::supportNormalizationOfClass($className)

this is just 1 idea but I think the key problem it points to is if you just have class like \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem right now in the current normalization process it is impossible to determine which properties will be exposed

I feel like it would get very complicated very quickly if we don't use the TypeData system as the Single Source of Truth.

 Related issues: +#2392845: Clarify the notion of "computed field"

#49: hah, funny that you found a bug in #2846554: Make the PathItem field type actually computed and auto-load stored aliases, I did too, at #2824851-73: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior. In that case, it was for ::equals().

#52

not semantically sensible is definitely an understatement… those entity keys can never ever ever be remotely useful. We've been going through a lot of effort to make our normalizations better (e.g. #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX). Making the output look like you describe would basically mean we say "screw any user talking to Drupal via json or hal_json :( Our normalizations may be verbose (and yes, I totally agree with that!) and not ideal (totally agree with that too!), but at least every single thing you see contains correct, actionable data. That would no longer be true. In fact, it'll literally look broken.

We're not arguing to add "REST knowledge", we're arguing to add metadata that allows code to know which properties make sense to serialize at all. This would also help e.g. https://www.drupal.org/project/default_content. It would help core REST. It would help https://www.drupal.org/project/jsonapi. It would help https://www.drupal.org/project/graphql. And it will help anything else of the like in the future. In a nutshell: it allows us to know which properties make sense to export or normalize or serialize or freeze or represent or render — whatever term you prefer, it's the same concept: the ability to take data stored in Drupal and represent it in a non-PHP-object way, so that it can be shared, stored, reused …

RE: the fact that text fields lose cacheability metadata is a bug anyway — yes it's a bug, but it's a bug that we cannot fix. Fixing it implies changing the type (i.e. from string to \Drupal\filter\FilterProcessResult), which is a BC break and therefore not allowed.

#54:

Well for BC we could add (yet another) config setting to control this behavior, so I don't think that's a strong argument.

This is inaccurate.

Today, many properties are missing in our normalizations. For TextItem, it's the processed_result property. For LinkItem, it's the options property. For ImageItem, it's image style URLs. And so on. This affects core REST (in both json and hal_json formats), as well as https://www.drupal.org/project/jsonapi, as well as https://www.drupal.org/project/graphql.

If we use getProperties(TRUE), then we get things in the output that are definitely unacceptable. And that's just core. For field types in contrib modules, we don't know what's stored in properties — there could be sensitive things in there. And there definitely will be those similar examples of things that only make sense in PHP land to normalize (like the entity property on EntityReferenceItem).

You say that BC layer would allow you to choose to keep the current behavior (which would mean no processed text, a.o.) or opt in to the new behavior (which results in clearly unusable normalizations). But … that's choosing to either not get all the data, or to get broken data. How is that a sensible choice?

just ugly is hugely euphemistic, and is ignoring the real DX consequences. It would instantly disqualify Drupal.

I completely understand your POV: you're an Entity/Field/Typed Data API maintainer. You want to keep things as simple, consistent as possible. You want to ensure that responsibilities are cleanly delineated. But … the whole reason Typed Data exists is so that there is sufficient metadata to describe the data structure, purpose, capabilities, restrictions etc of every entity field and every entity field property. I'd love to be able to use setComputed() instead of dealing with this issue, but there's #2392845: Clarify the notion of "computed field". If setComputed() would actually mean "is calculated value" and we'd have a isConvienceProperty() for things like entity and date, then we'd be able to use that. Perhaps that's an approach that you'd like better?

That would solve 90% of our problem here!

Btw for default content it ould allow to filter user's internal fields or manage that more preciesly

I totally agree with @tstoeckler that adding more and more stuff to the typed data stuff smells wrong. Its already an almost non understandable system, we should aim for simplicity.

1. +++ b/core/lib/Drupal/Core/TypedData/ComplexDataWithExposedPropertiesTrait.php
@@ -0,0 +1,27 @@
+  public function getExposedProperties() {
+    $properties =$this->getProperties(TRUE);
+    /* @var  \Drupal\Core\TypedData\TypedDataInterface[] $properties */ + foreach (array_keys($properties) as $property_name) { + if (!$properties[$property_name]->getDataDefinition()->isExposed()) { + unset($properties[$property_name]); + } + } + return$properties;
+  }

I'm always fascinated by new methods which themselves just call out to public methods. For example in this case it would not be a trait, it could be a helper living somewhere and we just call it with a complex data object. It feels like we are ignoring one dimension of programming in general.

@@ -218,4 +218,23 @@ public function getConstraint($constraint_name); + /** + * 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(); + Well for BC we could add (yet another) config setting to control this behavior, so I don't think that's a strong argument. This is inaccurate. I think all what tstoeckler is saying: Maybe we should put this exposed vs. not exposed level into an internal setting and get it via getSetting instead. Would that make sense? Thanks a lot @tedbow and @Wim Leers for those write-ups. They put this issue in a much better perspective for me. I will write a proper technical reply next week but need to re-read and process a bunch of things first. Just wanted to make sure you know I greatly appreciate comments like #55 and #56. I know it takes not only a lot of time but also a lot of emotional effort to argue something that you are convinced of with someone coming from a completely different angle. Thanks! Just a quick note re #58.2: Actually what @Wim Leers thought I was saying, was in fact what I meant. As far as I know off the top of my head, typed data itself doesn't have the concept of settings, that's only on the field level. (I always get this stuff confused, though, so I may be wrong.) As far as I know off the top of my head, typed data itself doesn't have the concept of settings, that's only on the field level. (I always get this stuff confused, though, so I may be wrong.) This is totally true for generic typed data, but its not true for \Drupal\Core\TypedData\ComplexDataInterface. ... which is what you can potentially iterate over. All I try is to avoid adding "yet another" thing, even well, if we want to be API first, having these ideas as a first class citizen maybe make sense. Thanks a lot @tedbow and @Wim Leers for those write-ups. They put this issue in a much better perspective for me. I will write a proper technical reply next week but need to re-read and process a bunch of things first. Just wanted to make sure you know I greatly appreciate comments like #55 and #56. I know it takes not only a lot of time but also a lot of emotional effort to argue something that you are convinced of with someone coming from a completely different angle. Thanks! ❤️  Status: Needs review » Needs work I think this general approach looks good, I read the previous discussion for the reasoning (putting this at the typed data level) and agree this seems like the only real place this can live to work properly. Here are some other comments: 1. +++ b/core/lib/Drupal/Core/TypedData/ComplexDataWithExposedPropertiesTrait.php @@ -0,0 +1,27 @@ +$properties = $this->getProperties(TRUE); I agree with Daniel here too I think - it's kind of weird when traits have a hidden dependency on the class that is using the trait. 2. +++ b/core/lib/Drupal/Core/TypedData/ComplexDataWithExposedPropertiesTrait.php @@ -0,0 +1,27 @@ + foreach (array_keys($properties) as $property_name) { + if (!$properties[$property_name]->getDataDefinition()->isExposed()) { + unset($properties[$property_name]); + } It could be clearer to just return the result of array_filter here? 3. +++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php @@ -218,4 +218,23 @@ public function getConstraint($constraint_name);
+  public function setExposed($exposed); It would be nice if$exposed had a default value of TRUE here. It's small but being able to do setExposed() in the chain is a bit nicer.

@@ -0,0 +1,34 @@
+    // For non-computed fields 'exposed' defaults to TRUE.
+    if (!$this->isComputed() && !isset($this->definition['exposed'])) {

This comment and the code don't match. it doesn't default it still checks the configured definition property. I think this could be condensed down to return $this->isComputed() && !empty($this->definition['exposed']) ?

@@ -0,0 +1,34 @@
+  public function setExposed($exposed) { Same as interface comment, maybe defaults to TRUE.. 6. +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php @@ -185,4 +185,12 @@ protected function ensureLoaded() { + public function get($property_name) {
+    $this->ensureLoaded(); + return parent::get($property_name);
+  }

Is this needed for this patch? Just wondering.

7. +++ b/core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php
@@ -25,10 +28,19 @@ class ComplexDataNormalizer extends NormalizerBase {
+    if ($object instanceof ComplexDataInterface) { +$attributes = $this->normalizeProperties($object, $format,$context);
+    }
+    else {
+      $attributes = []; + /** @var \Drupal\Core\TypedData\TypedDataInterface$field */
+      foreach ($object as$name => $property) { +$attributes[$name] =$this->serializer->normalize($property,$format, $context); + } We could have another normalizer that is just for traversable objects? we could then have our normalizers extend that? We could keep the ComplexDataNormalizer in place for anything using it already in contrib etc. Side note - yes when this was implemented entities did implement the ComplexDataInterface.. 8. +++ b/core/modules/serialization/src/Normalizer/ComplexDataPropertiesNormalizerTrait.php @@ -0,0 +1,41 @@ + if (is_object($attribute) && method_exists($attribute, '__toString')) { Usually normalization produces a string or array of data from objects, not objects. Looks like this is to do with the test coverage more than anything? Generally if it's an object it should have a normalizer, and normalize() would return the data that's used. This seems like kind of a hack of the normalization layer with some in-place normalization. We have a normalizer for Markup objects I think, I guess this is not one of those. @damiankloip thanks for the review 6. yes this needed see #49 7. This sounds like follow up because we need to cover both cases anyways in ComplexDataNormalizer because as noted elsewhere the class is already being extended in Contrib for classes that don't implement ComplexDataInterface Ah sorry, I missed that comment... Does that mean we need a small addition to some test coverage for that too while we're here (sorry)? I.e. something quick to cover the case in \Drupal\Tests\path\Kernel\PathItemTest ?  Version: 8.4.x-dev » 8.5.x-dev Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle. FileSize 9 KB 37.31 KB deleting comment, moving to #70 with correct patch  Status: Needs work » Needs review  Status: Needs review » Needs work The last submitted patch, 66: 2871591-66.patch, failed testing. View results Whoops I did the patch against 8.4. Also need to fix \Drupal\Tests\serialization\Unit\Normalizer\ComplexDataNormalizerTest  Status: Needs work » Needs review FileSize 16.26 KB 33.21 KB Sorry @all that this has taken me so long to get back to. #62 1. Remove the trait and create \Drupal\Core\TypedData\TypedDataHelper 2. fixed 3. Set default to TRUE 4. Actually I think the comment does match. If we changed it to return$this->isComputed() && !empty($this->definition['exposed']) Then only computed properties could ever be exposed. But all non-computed properties should be exposed unless$this->definition['exposed'] is expliciting set to TRUE.
5. fixed
6. re #64 add test coverage to PathItemTest. The trick is that for isEmpty and the 2 ways to get the property values you have to make sure that none of the other ways is called first or ->ensuredLoaded() would have already been called.
7. see #63
8. This was added to make sure it will work with logic needed for #2626924: [PP-1] Expose TextItems' "processed" property when normalizing. We could remove this here and add it back there if needed(I think it will be).

Also updated ComplexDataNormalizerTest, EntityReferenceFieldItemNormalizerTest, TimestampItemNormalizerTest removed expected calls to getExposedProperites(). I created \Drupal\Tests\serialization\Unit\Normalizer\TypedDataTestTrait because all of these these tests had the same code.