Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts

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.

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: ImageItem should have an "derivatives" computed property, to expose all 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?

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.

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

#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 issue shows that some complex properties does not fits in annotation

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.

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

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

Note that as of #2626924-151: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata, this is hard-blocking #2626924.

And soon it will likely also be a hard blocker for #2825812: ImageItem should have an "derivatives" computed property, to expose all image style URLs.

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

+++ 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

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.

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?

Let's get this moving forward again?

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.

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: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata and #2825812: ImageItem should have an "derivatives" computed property, to expose all 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?

@tstoeckler note that #2626924: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata 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().

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 @@
   "menu_link": [],
   "field_date": [
     {
-      "value": "2017-12-31T22:59:59"
+      "value": "2017-12-31T22:59:59",
+      "date": {}
     }
   ],
   "body": [
     {
       "value": "\u003Cp\u003Easdasdsadas\u003C\/p\u003E\r\n",
       "format": "basic_html",
-      "summary": ""
+      "processed": "\u003Cp\u003Easdasdsadas\u003C\/p\u003E",
+      "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: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata 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.)

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

#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: Add a trait to standardize handling of computed item lists. 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.

2. +++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
   @@ -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!

❤️

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.

4. +++ b/core/lib/Drupal/Core/TypedData/ExposableDataDefinitionTrait.php
   @@ -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']) ?

5. +++ b/core/lib/Drupal/Core/TypedData/ExposableDataDefinitionTrait.php
   @@ -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.

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 ?

Sorry for the lack of reviews here. My bad. Let's get this going again!

Q: Can we remove all the PathItem-related changes after #2905524: The count() method used on PathFieldItemList behaves differently if isEmpty() has been called. lands?

We could remove this here and add it back there if needed(I think it will be).

I think that makes sense.

Q: Can you create a change record (which will be necessary anyway), which would then also make it easier to understand and review this patch!

1. --- a/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityAdapter.php
   +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityAdapter.php
   

   Unnecessary change here.

2. +++ b/core/lib/Drupal/Core/TypedData/ExposableDataDefinitionTrait.php
   @@ -0,0 +1,34 @@
   + * Methods are implemented for \Drupal\Core\TypedData\DataDefinitionInterface.
   

   Nit: Let's add an @see?

3. +++ b/core/lib/Drupal/Core/TypedData/ExposableDataDefinitionTrait.php
   @@ -0,0 +1,34 @@
   +    // For non-computed fields 'exposed' defaults to TRUE.
   +    if (!$this->isComputed() && !isset($this->definition['exposed'])) {
   +      return TRUE;
   +    }
   +    return !empty($this->definition['exposed']);
   

   I think the logic is correct, but perhaps a bit confusing. The following would be clearer I think:

   // Respect the definition, otherwise default to TRUE for non-computed fields.
   if (isset($this->definition['exposed']))
    {
     return $this->definition['exposed'];
   }
   else {
     return !$this->isComputed();
   }

4. +++ b/core/lib/Drupal/Core/TypedData/ExposableDataDefinitionTrait.php
   @@ -0,0 +1,34 @@
   +   * Implements setExposed() for \Drupal\Core\TypedData\DataDefinitionInterface.
   

   Nit: this seems unnecessary?

5. +++ b/core/lib/Drupal/Core/TypedData/TypedDataHelper.php
   @@ -0,0 +1,27 @@
   +/**
   + * Helper class for Typed Data.
   + *
   + * @ingroup typed_data
   + */
   +class TypedDataHelper {
   +
   +  /**
   +   * Gets an array exposed properties from a complex data object.
   +   *
   +   * @param \Drupal\Core\TypedData\ComplexDataInterface $data
   +   *   The complex data object.
   +   *
   +   * @return \Drupal\Core\TypedData\TypedDataInterface[]
   +   *   The exposed properties.
   +   */
   +  public static function getExposedProperties(ComplexDataInterface $data) {
   

   This feels a bit strange. I'm not sure yet how to do it better though.

6. --- a/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   --- a/core/modules/path/tests/src/Kernel/PathItemTest.php
   +++ b/core/modules/path/tests/src/Kernel/PathItemTest.php
   

   Can't these changes happen in their own issue? Ideally this issue would only build infrastructure.

7. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestExposedPropertyNormalizerTest.php
   @@ -0,0 +1,99 @@
   +class EntityTestExposedPropertyNormalizerTest extends EntityTestResourceTestBase {
   ...
   +    if (!FieldStorageConfig::loadByName('entity_test', 'field_test_exposed')) {
   +      // Auto-create a field for testing.
   +      FieldStorageConfig::create([
   +        'entity_type' => 'entity_test',
   +        'field_name' => 'field_test_exposed',
   +        'type' => 'exposed_string_test',
   +        'cardinality' => 1,
   +        'translatable' => FALSE,
   +      ])->save();
   +      FieldConfig::create([
   +        'entity_type' => 'entity_test',
   +        'field_name' => 'field_test_exposed',
   +        'bundle' => 'entity_test',
   +        'label' => 'Test exposed-field',
   +      ])->save();
   +    }
   

   I'd expect also a field_test_unexposed to be added here, to prove that we're not just exposing everything.

8. +++ b/core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php
   @@ -25,10 +28,19 @@ class ComplexDataNormalizer extends NormalizerBase {
   +    // $object will not always match $supportedInterfaceOrClass.
   +    // @see \Drupal\serialization\Normalizer\EntityNormalizer
   +    // Other normalizer that extend this class may only provide $object that
   +    // implements \Traversable.
   

   I first had a hard time understanding this. But it makes sense: class EntityNormalizer extends ComplexDataNormalizer, without overriding this normalize() method, therefore this may also be called with EntityInterface $object.

   IDK how to make this comment clearer. It's good enough for now.

9. +++ b/core/modules/serialization/src/Normalizer/ComplexDataPropertiesNormalizerTrait.php
   @@ -0,0 +1,42 @@
   +      if (is_object($attribute) && method_exists($attribute, '__toString')) {
   +        $attribute = (string) $attribute;
   +      }
   

   This is weird. Why do we need this?

10. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/TypedDataTestTrait.php
    @@ -0,0 +1,36 @@
    + * Test trait that provides functions for retreiving typed data properties.
    

    Nit: s/retreiving/retrieving/

11. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/TypedDataTestTrait.php
    @@ -0,0 +1,36 @@
    +trait TypedDataTestTrait {
    

    The name is super generic. I think ExposedTypedDataTestTrait probably better?

12. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/DataType/ExposedStringData.php
    @@ -0,0 +1,37 @@
    +  public function getCastedValue() {
    +    return $this->getValue();
    +  }
    

    The docs for the interface say this:

      /**
       * Gets the primitive data value casted to the correct PHP type.
       *
       * @return mixed
       */
      public function getCastedValue();
    

    Therefore I think this needs to become

    return (string) $this->getValue();
    

    Which means you can probably also remove the (string) casting in ComplexDataPropertiesNormalizerTrait — i.e. my point 9 above and @damiankloip's remark in #62.8.

I'll hold off reviewing this until it's green again, because it seems that some of the things I pointed at, and were changed, play a role in this patch now failing. I might be reviewing something that's going to change anyway.

#78: +1. I think it's important that we keep ensuring that cacheability bubbling can be layered on top without BC breaks though. So can you also open an issue for that?

1. +++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
   --- a/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   
   +++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
   --- a/core/modules/path/tests/src/Kernel/PathItemTest.php
   +++ b/core/modules/path/tests/src/Kernel/PathItemTest.php
   

   So these hunks will go away once #2908674: When using get() method directly on PathItem the alias is not loaded lands. Great.

2. +++ b/core/modules/path/tests/src/Kernel/PathItemTest.php
   --- a/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
   +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
   

   All changes here are related to cacheability too. They should be removed from this patch — they were apparently overlooked in #78.

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestExposedPropertyNormalizerTest.php
   @@ -0,0 +1,87 @@
   +    // The 'non_exposed_value' key will not be returned because setExposed(TRUE)
   ...
   +    $expected['field_test_exposed'] = [
   +      [
   +        'value' => 'value to expose',
   +        'exposed_value' => 'Computed! value to expose',
   +      ],
   +    ];
   

   The comment describes something that does not exist?

4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestExposedPropertyNormalizerTest.php
   @@ -0,0 +1,87 @@
   +      // Auto-create a field for testing.
   

   This comment isn't very useful. We can remove it.

5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestExposedPropertyNormalizerTest.php
   @@ -0,0 +1,87 @@
   +    $post_entity = parent::getNormalizedPostEntity();
   +    $post_entity['field_test_exposed'] = [
   +      [
   +        'value' => 'value to expose',
   +      ],
   +    ];
   +    return $post_entity;
   

   Can be simplified to:

   return parent::getNormalizedPostEntity()
    + [
     'field_test_exposed' => …
   ];
   

   More importantly: let's choose a value that is not the same as the value set in createEntity().

6. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/ComplexDataNormalizerTest.php
   @@ -7,8 +7,8 @@
   +use Drupal\Core\Cache\CacheableDependencyInterface;
   

   We don't need this anymore, see #78 again.

7. +++ b/core/modules/system/tests/modules/entity_test/src/ComputedString.php
   @@ -0,0 +1,37 @@
   +class ComputedString {
   
   +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/DataType/ComputedStringData.php
   @@ -0,0 +1,37 @@
   +class ComputedStringData extends StringData {
   

   If we'd have that follow-up that #78 describes (and which I asked for earlier in this comment), which would layer on cacheability bubbling, then the structure of this code will make far more sense too! :)

2. Oops, didn't mention intend to mention PathItemTest in this point! I was indeed pointing to ResourceResponseSubscriber.

7. Nope, didn't want a change, was merely observing :)

#86 failed because:

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestExposedPropertyNormalizerTest.php
   @@ -0,0 +1,87 @@
   +  protected function getNormalizedPostEntity() {
   

   This now has weird indentation… which was also hiding a functional change, which also caused failures.

2. it didn't include #2908674: When using get() method directly on PathItem the alias is not loaded — EDIT: apparently this was intentional, I first missed the last paragraph of Ted's last comment.

Trying to do some really comprehensive reviews to get this one to an RTBC'able point.

1. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -6,12 +6,14 @@
    abstract class FieldConfigBase extends ConfigEntityBase implements FieldConfigInterface {
    
   +  use ExposableDataDefinitionTrait;
   

   Doesn't this also mean that the config schema should be expanded? i.e. field.schema.yml

2. +++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
   @@ -218,4 +218,23 @@ public function getConstraint($constraint_name);
   +   *   Whether the data value is exposed.
   

   s/is/should be/

3. +++ b/core/lib/Drupal/Core/TypedData/ExposableDataDefinitionTrait.php
   @@ -0,0 +1,36 @@
   +/**
   + * Exposable property methods.
   + *
   + * Methods are implemented for \Drupal\Core\TypedData\DataDefinitionInterface.
   + *
   + * @see \Drupal\Core\TypedData\DataDefinitionInterface
   + */
   

   Changing this comment to be similar to what \Drupal\Core\Plugin\Definition\DependentPluginDefinitionTrait and \Drupal\system\Tests\Entity\EntityDefinitionTestTrait.

4. +++ b/core/lib/Drupal/Core/TypedData/TypedDataHelper.php
   @@ -0,0 +1,27 @@
   +   * @return \Drupal\Core\TypedData\TypedDataInterface[]
   +   *   The exposed properties.
   

   This does not document what the keys are. Done.

5. +++ b/core/lib/Drupal/Core/TypedData/TypedDataHelper.php
   @@ -0,0 +1,27 @@
   +  public static function getExposedProperties(ComplexDataInterface $data) {
   

   This is only used in one place. Which means it doesn't make sense for this to be in a separate "helper" class. We can still extract it later.

   (In #70, it used to be necessary, because many other things called this, but that's no longer the case. So we can now simplify this, yay!)

   Done.

6. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestExposedPropertyNormalizerTest.php
   @@ -0,0 +1,87 @@
   +  protected static $mimeType = 'application/json';
   

   This is testing the json format. We should also test hal_json.

7. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestExposedPropertyNormalizerTest.php
   @@ -0,0 +1,87 @@
   +    $post_entity = parent::getNormalizedPostEntity() + [
   

   Nit: This can be simplified a bit more still.

8. +++ b/core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php
   @@ -25,10 +28,19 @@ class ComplexDataNormalizer extends NormalizerBase {
   +    // Other normalizer that extend this class may only provide $object that
   

   s/normalizer/normalizers/

   Fixed.

9. +++ b/core/modules/serialization/src/Normalizer/ComplexDataPropertiesNormalizerTrait.php
   @@ -0,0 +1,34 @@
   + * Normalization methods for complex data properties.
   

   This is accurate, but an @see would make it clear for which classes this can be used. Done.

10. +++ b/core/modules/serialization/src/Normalizer/TypedDataNormalizer.php
    @@ -18,7 +20,11 @@ class TypedDataNormalizer extends NormalizerBase {
    -    return $object->getValue();
    +    $value = $object->getValue();
    +    if (is_object($value) && $object instanceof PrimitiveInterface) {
    +      return $object->getCastedValue();
    +    }
    +    return $value;
    

    Why do we need this change?

    Why doesn't \Drupal\serialization\Normalizer\PrimitiveDataNormalizer already cover this?

11. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/ComplexDataNormalizerTest.php
    @@ -44,103 +45,77 @@ protected function setUp() {
    +    $complexData = $this->prophesize(ComplexDataInterface::class)->reveal();
    

    Nit: we don't do camelCase in variables, only on class properties.

    Fixed.

12. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/ExposedTypedDataTestTrait.php
    @@ -0,0 +1,36 @@
    + * Test trait that provides functions for retrieving typed data properties.
    

    Nit: missing "exposed".

13. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/ExposedTypedDataTestTrait.php
    @@ -0,0 +1,36 @@
    +trait ExposedTypedDataTestTrait {
    

    Nit: missing "properties". Should be named ExposedTypedDataPropertiesTestTrait

14. +++ b/core/modules/system/tests/modules/entity_test/config/schema/entity_test.data_types.schema.yml
    @@ -0,0 +1,15 @@
    +# Schema for the configuration of the exposed string field type.
    
    +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/ExposedPropertyTestFieldItem.php
    @@ -0,0 +1,45 @@
    + * Defines the 'Exposed Property' entity test field type.
    ...
    + *   id = "exposed_string_test",
    + *   label = @Translation("Exposed Property (test)"),
    ...
    +class ExposedPropertyTestFieldItem extends StringItem {
    

    "exposed property" vs "exposed string test" vs "exposed property (test)" vs "exposed property test" vs "exposed string".

    Let's make this consistent.

    I propose:

    • @FieldType = exposed_property_test
    • @DataType = computed_string_test

15. +++ b/core/modules/system/tests/modules/entity_test/config/schema/entity_test.data_types.schema.yml
    @@ -0,0 +1,15 @@
    +field.storage_settings.exposed_string_test:
    +  type: mapping
    +  label: 'String settings'
    +  mapping:
    +    max_length:
    +      type: integer
    +      label: 'Maximum length'
    +    case_sensitive:
    +      type: boolean
    +      label: 'Case sensitive'
    +    is_ascii:
    +      type: boolean
    +      label: 'Contains US ASCII characters only'
    

    Rather than specifying these explicitly, we can extend field.storage_settings.string. (field.storage_settings.uri also does this, for example.)

    Done.

16. +++ b/core/modules/system/tests/modules/entity_test/config/schema/entity_test.data_types.schema.yml
    --- /dev/null
    +++ b/core/modules/system/tests/modules/entity_test/src/ComputedString.php
    

    I'd expect this class to be living in the same directory as the @DataType plugin that uses it.

17. +++ b/core/modules/system/tests/modules/entity_test/src/ComputedString.php
    @@ -0,0 +1,37 @@
    +class ComputedString {
    

    Should be marked @internal explicitly, because it is coupled to a @DataType plugin — it's an implementation detail of that.

18. +++ b/core/modules/system/tests/modules/entity_test/src/ComputedString.php
    @@ -0,0 +1,37 @@
    +  protected $notComputedValue;
    

    I think $sourceString or $sourceValue would be a better name.

19. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/DataType/ComputedStringData.php
    @@ -0,0 +1,37 @@
    + * The exposed string test data type.
    ...
    + *   label = @Translation("Exposed String")
    

    This is not exposed, but computed. Fixed.

20. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/DataType/ComputedStringData.php
    @@ -0,0 +1,37 @@
    + * This simply concatenates 'Exposed! ' before the 'value' property.
    

    This comment does not belong here anymore. Removed.

21. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/ExposedPropertyTestFieldItem.php
    @@ -0,0 +1,45 @@
    + *   description = @Translation("A field containing two computed string values, one exposed and one not exposed."),
    

    one string, and two computed string values

22. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/ExposedPropertyTestFieldItem.php
    @@ -0,0 +1,45 @@
    +      ->setLabel(new TranslatableMarkup('Text value exposed'))
    ...
    +      ->setLabel(new TranslatableMarkup('Text value non-exposed'))
    

    Nit: These are computed. The labels imply otherwise. Fixed.

I only did string fixes and removed the TypedDataHelper class, and merged its logic in the sole caller. In other words: only trivial things, so I can continue to review this.

Of the 22 remarks in #90, I only addressed #2, #3, #4, #5, #7, #8, #9, #11, #12, #13, #15, #19, #20, #21, #22.

Still to be done: #1, #6, #10, #14, #16, #17, #18. Leaving those to @tedbow.

Updating issue title to match current patch.

I said this in #90.#6:

This is testing the json format. We should also test hal_json.

You may wonder "why?". In general, you'd be right, this should not be necessary. But:

1. we're modifying core/modules/hal/src/Normalizer/FieldItemNormalizer.php and core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php to use ComplexDataPropertiesNormalizerTrait
2. the updated core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php ensures that both \Drupal\serialization\Normalizer\EntityNormalizer and \Drupal\serialization\Normalizer\FieldItemNormalizer inherit this, because both extend \Drupal\serialization\Normalizer\ComplexDataNormalizer
3. but the HAL module has its own field item normalizer which doesn't extend \Drupal\serialization\Normalizer\ComplexDataNormalizer, and therefore it uses the trait itself too

Therefore, since the code paths are different, we need to explicitly test both formats.

Finally, having written this, I just realized something: shouldn't we expand the test coverage, to not only test exposed vs non-exposed properties on fields, but also exposed vs non-exposed fields on entities?

1. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
   @@ -354,4 +354,18 @@ public function __sleep() {
   +  public function setInternal($internal = TRUE) {
   

   ❓ Should not default to TRUE because \Drupal\Core\TypedData\DataDefinition::setReadOnly() and \Drupal\Core\TypedData\DataDefinition::setRequired() also don't do this.

2. +++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
   @@ -219,22 +219,11 @@ public function getConstraint($constraint_name);
   -  public function setExposed($exposed = TRUE);
   ...
   -  public function isExposed();
   +  public function isInternal();
   

   👍 This removes the setter from the interface, keeping it only on the ipmlementation.

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestInternalPropertyNormalizerTest.php
   @@ -7,11 +7,11 @@
   - * Test that exposed properties are actually exposed in REST.
   + * Test that internal properties are not exposed in REST.
   

   👍 This is the only remaining occurrence of the string "expose" in the entire patch!

4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestInternalPropertyNormalizerTest.php
   @@ -30,13 +30,14 @@ class EntityTestExposedPropertyNormalizerTest extends EntityTestResourceTestBase
   +    // The 'internal_value' property in test field type will not return in
   +    // normalization because setInternal(FALSE) was not called for this
   

   s/will not return in normalization/is not exposed in the normalization/

5. +++ b/core/lib/Drupal/Core/Field/FieldConfigBase.php
   @@ -6,12 +6,15 @@
   +  use InternalDataDefinitionTrait;
   

   👍 This adds the isInternal() method implementation from the trait. Because there is no setter available on FieldConfigBase and subclasses (it only lives on DataDefinition and subclasses, which FieldConfigBase does not extend), this means that it will fall back to calling isComputed(), which is hardcoded to always return FALSE in \Drupal\field\Entity\FieldConfig::isComputed().

   ⁉️ However, the other subclass of FieldConfigBase, BaseFieldOverride, implements ::isComputed() like this:

     public function isComputed() {
       return $this->getBaseFieldDefinition()->isComputed();
     }
   

   Which means that if a base field marks itself as internal (via setInternal(TRUE), because \Drupal\Core\Field\BaseFieldDefinition extends DataDefinition), then that will continue to be respected.

   The only example of an entity type that has a computed base field in core is \Drupal\entity_test\Entity\EntityTestComputedField.

   @fago was very explicit in wanting it on DataDefinitionInterface, meaning that he knew it'd affect base fields. Which means we must also respect isInternal() for fields, not just for properties. Which means we'll also need to update \Drupal\serialization\Normalizer\ContentEntityNormalizer::normalize() to use ComplexDataPropertiesNormalizerTrait::normalizeProperties(), to ensure computed fields are also omitted from the normalization, unless they're explicitly marked as non-internal.

   This also means we should add REST test coverage for \Drupal\entity_test\Entity\EntityTestComputedField, because right now its computed field (and hence internal by default) is being exposed, it should not be.

Also pinged @fago for feedback here, that's why I'm leaving this at Needs review.

Won't this be a BC break for our REST responses?
…
That means any computed fields outside of core that are being returned REST responses will all of sudden not be returned.

Ugh, you're right :( What a mess. This means we'll have to treat fields and properties inconsistently, for BC reasons. Right?

This looks seriously great!

1. +++ b/core/lib/Drupal/Core/TypedData/InternalDataDefinitionTrait.php
   @@ -0,0 +1,26 @@
   + * Trait for data definitions that have test for internal properties.
   

   For some reason this doesn't sound like proper english, anyone may clarify?

2. +++ b/core/modules/hal/tests/src/Functional/EntityResource/EntityTest/EntityTestHalJsonInternalPropertyNormalizerTest.php
   @@ -0,0 +1,97 @@
   +      ])->save();
   +
   +    }
   
   +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestJsonInternalPropertyNormalizerTest.php
   @@ -0,0 +1,88 @@
   +      ])->save();
   +
   +    }
   

   🙃 lonely empty line

Overall, very much liking where this is headed! I think it's pretty much where we want to be. Great work pushing this forward Ted. A few comments:

1. +++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
   @@ -352,4 +354,18 @@ public function __sleep() {
   +    $this->definition['internal'] = $internal;
   

   Just to be pedantic, we could cast to a bool here.

2. +++ b/core/lib/Drupal/Core/TypedData/InternalDataDefinitionTrait.php
   @@ -0,0 +1,26 @@
   +    else {
   +      return $this->isComputed();
   +    }
   

   This is mainly a preference thing I guess, but we can just ditch the else here. The return value can just be the default return for this method.

3. +++ b/core/modules/hal/tests/src/Functional/EntityResource/EntityTest/EntityTestHalJsonInternalPropertyNormalizerTest.php
   @@ -0,0 +1,96 @@
   +        'non_internal_value' => 'Computed! value to be internal',
   

   non internal?

4. +++ b/core/modules/hal/tests/src/Functional/EntityResource/EntityTest/EntityTestHalJsonInternalPropertyNormalizerTest.php
   @@ -0,0 +1,96 @@
   +    if (!FieldStorageConfig::loadByName('entity_test', 'field_test_internal')) {
   

   Howcome we need this? when is the field already there in the test env?

5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestJsonInternalPropertyNormalizerTest.php
   @@ -0,0 +1,87 @@
   +        'non_internal_value' => 'Computed! value to be internal',
   

   Same here

6. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTestInternalField/EntityTestJsonInternalFieldTestBase.php
   @@ -0,0 +1,167 @@
   +  use AnonResourceTestTrait;
   +
   +  use BcTimestampNormalizerUnixTestTrait;
   

   nit: we can just have one use statement to define multiple here.

7. +++ b/core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php
   @@ -25,10 +29,19 @@ class ComplexDataNormalizer extends NormalizerBase {
   +      foreach ($object as $name => $property) {
   +        $attributes[$name] = $this->serializer->normalize($property, $format, $context);
   +      }
   

   This is where I mean access would now not be checked on the property level, compared to the normalizeProperties methods from the new trait.

8. +++ b/core/modules/serialization/src/Normalizer/ComplexDataPropertiesNormalizerTrait.php
   @@ -0,0 +1,55 @@
   +      if ($property instanceof AccessibleInterface && !$property->access('view', $context['account'])) {
   +        continue;
   +      }
   

   Hmm, this is introducing access checking we don't currently have. Not sure we should do that here? Question is, does this mean we then need to make the same change for regular iterations outside of this complex data case (the madness we have with some extending classes not implementing ComplexDataInterface)?

9. +++ b/core/modules/serialization/src/Normalizer/TypedDataNormalizer.php
   @@ -18,7 +20,11 @@ class TypedDataNormalizer extends NormalizerBase {
   +    $value = $object->getValue();
   +    if (is_object($value) && $object instanceof PrimitiveInterface) {
   +      return $object->getCastedValue();
   +    }
   +    return $value;
   

   Howcome we need this change? This is doing the same as the PrimitiveDataNormalizer I think? Which is meant to be running before this one.

As we discussed in Vienna, this 'internal' approach makes very much sense! Here's a high-level review for now:

1. +++ b/core/lib/Drupal/Core/TypedData/InternalDataDefinitionTrait.php
   @@ -0,0 +1,26 @@
   +trait InternalDataDefinitionTrait {
   

   Is this trait really needed? It only saved a few lines of code at the expense of making it harder to scan various classes for what the isInternal() method does.

   I kind of like how every setter and getter stand together in \Drupal\Core\TypedData\DataDefinition for example, the code is super easy to read.

2. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestInternalField.php
   @@ -0,0 +1,41 @@
   +class EntityTestInternalField extends EntityTest {
   

   It would be better if we didn't create an entirely new entity type for these two base fields and add them via entity_test_entity_base_field_info() instead.

3. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestInternalField.php
   @@ -0,0 +1,41 @@
   +    $fields['non_internal_string_field'] = BaseFieldDefinition::create('string')
   +      ->setLabel('Internal');
   

   Small contradiction here :)

4. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/DataType/ComputedStringData.php
   @@ -0,0 +1,34 @@
   +class ComputedStringData extends StringData {
   
   +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/InternalPropertyTestFieldItem.php
   @@ -0,0 +1,45 @@
   +      ->setComputed(TRUE)
   +      ->setClass(ComputedStringData::class)
   ...
   +      ->setComputed(TRUE)
   +      ->setClass(ComputedStringData::class);
   

   We don't have any other test data types, so I wonder why is this one special/needed.

   It seems to only return some concatenated values after all, which is also not dependent on the setComputed(TRUE) definition from the second hunk.

Thanks for the review, @amateescu! ❤️

1. I like that.
2. Ohh, interesting! Then we could just expand \Drupal\Tests\rest\Functional\EntityResource\EntityTest\EntityTestResourceTestBase, instead of adding lots of new test classes. I like it.
3. Well-spotted!
4. Because we want to have explicit test coverage for a field that has a computed property with both the default (internal=true) and the alternative (internal=false). If we don't have a test data type, we can't explicitly test that things are working the way they should. The thing it computes is irrelevant (and yes, it's silly/contrived, but that's because it's only for testing purposes), the normalizer' behavior is what we want to test, and we need this for that.
   Or do you see some better/more elegant way to test this?

Also updated entity_test_entity_base_field_info () to add the base field. I wondering though since I am adding the basefield when $entity_type->id() === 'entity_test' should we just be adding it in \Drupal\entity_test\Entity\EntityTest::baseFieldDefinitions.

We shouldn't add it unconditionally in entity_test_entity_base_field_info(), but only if \Drupal::state()->get('entity_test.test_internal'). We'd then set that state in our test coverage. In other words: we should only add this field for our specific test!

I was trying to review this but I don't fully understand what's going on, however there were some small things that could be improved, so attached is patch that fixes my own nits.

@tedbow asked me to look into #115 — some debugging later, ended up with this.

+++ b/core/lib/Drupal/Core/TypedData/DataDefinitionInterface.php
@@ -218,4 +218,12 @@ public function getConstraint($constraint_name);
+  /**
+   * Determines whether the data value is internal.
+   *
+   * @return bool
+   *   Whether the data value is internal.
+   */
+  public function isInternal();

This is introducing this concept of isInternal. I think it would be nice to see in the documentation of this method what means to have a internal value. As an example, take a look what ckeditor says about isInternal

#120++

I investigated the failing tests.

First note only tests for the hal_json format are failing, those for json are passing. That's very interesting.
Then, note that #111 removed the custom entity type for testing, in favor of adding a internal_string_field field to the EntityTest entity type. But it did not update the expected normalized entity. Because it's a field that's specifically intended to not be normalized, because it's internal:

+      // Set a value for the internal field to confirm that it will not be
+      // returned in normalization.

Or, as @tedbow put it in #104:

Yes so I think we have to have all fields be external unless they are explicitly set to internal.

Clearly this is working for json, but not hal_json.