@DataType=map cannot be normalized, affects @FieldType=link, @FieldType=map

It's not clear to me what you are suggesting/asking. Can you clarify the two directions in the IS?

This is not what I expected.

                {
                    "target_plugin_id": "order_item_quantity",
                    "target_plugin_configuration": []
                }

So it did work partially, the property names are there, and the plugin ID has been exported properly.
Only the configuration is empty. Probably because of missing functionality in the used core normalizer (inability to normalize Map properties)

I find the issue not just happen on jsonapi,If we just serialize the commerce promotion,the error still happen(no matter whether to enable jsonapi,this exclude the jsonapi's own normalize's influence)

$a = \Drupal::entityTypeManager()->getStorage('commerce_promotion')->load(1);
$serializer = \Drupal::service('serializer');
$jsondata = $serializer->serialize($a,'json');

the result is:

commerce_promotion => {"promotion_id":[{"value":1}],"uuid":[{"value":"fd787635-8649-4a42-95bc-6a5134a21871"}],"langcode":[{"value":"en"}],"name":[{"value":"twenty per cent discount"}],"description":[],"order_types":[{"target_id":"default","target_type":"commerce_order_type","target_uuid":"f06d2445-997b-4b8f-a2fb-1d03e90dca54"}],"stores":[{"target_id":1,"target_type":"commerce_store","target_uuid":"4a3271e1-5bda-4e8e-a63f-2763e0e56122","url":"\/store\/1"}],"offer":[{"target_plugin_id":"order_percentage_off","target_plugin_configuration":[]}],"conditions":[{"target_plugin_id":"order_total_price","target_plugin_configuration":[]}],"coupons":[],"usage_limit":[],"start_date":[{"value":"2017-08-13"}],"end_date":[],"compatibility":[{"value":"any"}],"status":[{"value":true}],"weight":[{"value":0}],"default_langcode":[{"value":true}]}

Then I debug the flow of execution

https://www.processon.com/view/link/5992a90ee4b06df7265ae9a6
Then I try to find which interface to implement:PrimitiveInterface or ComplexDataInterface?

we can see that the target_plugin_configuration is an instance of map,but the map has implemented the ComplexDataInterface,So we shouldn't do it again,Maybe just override some function of map.I will look further into this matter

Ah, so you are saying there is actually a bug. We need to investigate it.

I'm not sure, So I need some more investigation about Map DataType and the whole serialize/normalize actuating logic.
Just implement ComplexDataInterface or other related interface seems to be useless.

Now I find the offer's targert_plugin_configuration's PropertyDefinitions is null
So the ComplexDataNormalizer hasn't executed the normalize() function
So the solution concentrated on that Class Map can't get PropertyDefinitions like bojanz saied,
The most import thing is to find whether we should repair the issue on the commerce module,
Continuous exploration...

Sorry for delay of this issue because of many other task has to be done

On the basis of my debug(break off on getting PropertyDefinitions),I have found the right solution to this issue.
https://www.drupal.org/docs/8/api/typed-data-api/data-type-plugins
https://www.drupal.org/docs/8/api/typed-data-api/data-definitions
we must define definition_class on promotion plugin's annotation definition
If the orientation is right, I think I can fix it this week

@skyredwang Just imitate the core/lib/TypedData

1. Annotation/CommercePromotionOffer :add definition_class
2. CommercePromotionOffer's plugin manager(PromotionOfferManager) :implements TypedDataManagerInterface

Then do everything like the typedata's documents

...

I find this issue can be fixed in core

if we add

if (empty($attributes)) {
    return $object->getValue();
   }

to web/core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php
Offer and conditions and anything else work ok

The issue concentrated on null propertyDefinitions

@skyredwang Thanks,you help me hit the right point

#2882727: Add custom HAL normalizer

Last monday(8.14),I have used this method to solve the serialization problem,It define custom normalizer(the second method you provided on the origin page),You veto my method when I told you,You said:
"it is not right solution, Just implement ComplexdataInterface".
@skyredwang

All Plugins(like field) that be setted to MapDataDefinition can't be normaliozed,Include core and third-party modules,It seems to be more serious in Drupal
the modules list:

1. core/LinkItem
2. commerce
3. BlockField

Avaliable solutions:

1. Core:ComplexDataNormalizer.php add:
   

   if (empty($attributes)) {
     return $object->getValue();
   }
   

2. Write custom normalizer for each plugin that belongs to Map

Each of above is not the perfect solution

web/modules/contrib/commerce/src/Plugin/Field/FieldType/PluginItem.php

  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
    $properties['target_plugin_id'] = DataDefinition::create('string')
      ->setLabel(t('Plugin ID'));
    $properties['target_plugin_configuration'] = MapDataDefinition::create()
      ->setLabel(t('Plugin configuration'))
    return $properties;
  }

  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
    $properties['target_plugin_id'] = DataDefinition::create('string')
      ->setLabel(t('Plugin ID'));
    $properties['target_plugin_configuration'] = MapDataDefinition::create()
      ->setLabel(t('Plugin configuration'))
      ->setPropertyDefinition('value', MapDataDefinition::create('string'));

    return $properties;
  }

,
I add setPropertyDefinition, how to pass arguments to PropertyDefinition may be a resolution, Who has some idea?

fixed another way

The above patch make the normalizer execute TypedDataNormalizer->normalize() finally when normalize 'target_plugin_configuration'

/**
 * Converts typed data objects to arrays.
 */
class TypedDataNormalizer extends NormalizerBase {

  /**
   * The interface or class that this Normalizer supports.
   *
   * @var string
   */
  protected $supportedInterfaceOrClass = 'Drupal\Core\TypedData\TypedDataInterface';

  /**
   * {@inheritdoc}
   */
  public function normalize($object, $format = NULL, array $context = []) {
    return $object->getValue();
  }

}

work nothing with:

1. Core:ComplexDataNormalizer.php add:

if (empty($attributes)) {
  return $object->getValue();
}

But it dosn't hack drupal core

As I said in IRC, there is an existing issue (at least one) and I found it now: #2876686: [PP-1] Normalization of LinkItem's 'options' property. It is currently only about LinkItem but I already suggested there to make the fix more generic.

But as I also said, that issue has been open for a long time, and the chance of getting a custom fix into commerce might be higher than seeing that fixed in core, which might also only be added to 8.5.

Drupal 8.4.0-alpha1 has been released on August 4, 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.

test result:

PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\TypedData\MapDataNormalizeTest
F

Time: 13.09 seconds, Memory: 8.00MB

There was 1 failure:

1) Drupal\Tests\Core\TypedData\MapDataNormalizeTest::testBasicValidateWithConstraint
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'key1' => 'value1'
-    'key2' => 'value2'
 )

/var/www/drupal85/core/tests/Drupal/Tests/Core/TypedData/MapDataNormalizeTest.php:84

FAILURES!
Tests: 1, Assertions: 1, Failures: 1.

I rewrite the test through kerneltest because the UnitTestBase cannot simply define the relation between normarlizer and services

I write two test:

1. testMapNormalize(fail)

Test normalize map data that don't set setPropertyDefinition

2. testMapWithPropertiesNormalize(success):
   Test normalize map data that set setPropertyDefinition

Results:

root@a63bee6ebe7d:/var/www/drupal85/core# ../vendor/bin/phpunit tests/Drupal/Tests/Core/TypedData/MapDataNormalizeTest.php
PHPUnit 4.8.35 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Core\TypedData\MapDataNormalizeTest
F.

Time: 31.22 seconds, Memory: 6.00MB

There was 1 failure:

1) Drupal\Tests\Core\TypedData\MapDataNormalizeTest::testMapNormalize
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    'key1' => 'value1'
-    'key2' => 'value2'
 )

/var/www/drupal85/core/tests/Drupal/KernelTests/KernelTestBase.php:1099
/var/www/drupal85/core/tests/Drupal/Tests/Core/TypedData/MapDataNormalizeTest.php:55

FAILURES!
Tests: 2, Assertions: 3, Failures: 1.

If we make this modification

class Map extends TypedData implements \IteratorAggregate, ComplexDataInterface {
. . .

public function getProperties($include_computed = FALSE) {
  $properties = [];
  $PropertyDefinitions = $this->definition->getPropertyDefinitions();
  if (!empty($PropertyDefinitions)) {
    foreach ($this->definition->getPropertyDefinitions() as $name => $definition) {
      if ($include_computed || !$definition->isComputed()) {
        $properties[$name] = $this->get($name);
      }
    }
  }
  //if map data is array and don't set PropertyDefinitions,We simply define 'any'
  //TypeData to $properties,so that it can be normalized
  elseif(is_array($this->getValue())) {
    foreach ($this->getValue() as $key => $value){
      $properties[$key] = DataDefinition::create('any');
    };
  }

  return $properties;
}

we can make the test above all passed,But it will make field can't work
So we must find other place to inject the PropertyDefinition

To make the patch take effect,All the existed data(like commerce_promotion) that use map must be re-saved;
below is the output of commerce_promotion in jsonapi

{
    "data": [
        {
            "type": "commerce_promotion--commerce_promotion",
            "id": "0f614dd7-161d-481f-81e4-ff2fe53af3bc",
            "attributes": {
                "promotion_id": 1,
                "uuid": "0f614dd7-161d-481f-81e4-ff2fe53af3bc",
                "langcode": "en",
                "name": "test ptomotion",
                "description": "this is a test for promotion of jsonapi",
                "offer": {
                    "target_plugin_id": "order_item_fixed_amount_off",
                    "target_plugin_configuration": {
                        "amount": {
                            "number": "2",
                            "currency_code": "CNY"
                        }
                    }
                },
                "conditions": [
                    {
                        "target_plugin_id": "order_total_price",
                        "target_plugin_configuration": {
                            "operator": ">",
                            "amount": {
                                "number": "10",
                                "currency_code": "CNY"
                            }
                        }
                    }
                ],
                "condition_operator": "AND",
                "usage_limit": null,
                "start_date": "2017-09-04",
                "end_date": null,
                "compatibility": "any",
                "status": true,
                "weight": 0,
                "default_langcode": true
            },
            "relationships": {
                "order_types": {
                    "data": [
                        {
                            "type": "commerce_order_type--commerce_order_type",
                            "id": "e33104d0-1723-4fe7-b030-be3d261dfc1f"
                        }
                    ],
                    "links": {
                        "self": "http://drupal85.dev/jsonapi/commerce_promotion/commerce_promotion/0f614dd7-161d-481f-81e4-ff2fe53af3bc/relationships/order_types",
                        "related": "http://drupal85.dev/jsonapi/commerce_promotion/commerce_promotion/0f614dd7-161d-481f-81e4-ff2fe53af3bc/order_types"
                    }
                },
                "stores": {
                    "data": [
                        {
                            "type": "commerce_store--online",
                            "id": "ac20ce9e-eb1a-4904-a8b0-a51175e85e06"
                        }
                    ],
                    "links": {
                        "self": "http://drupal85.dev/jsonapi/commerce_promotion/commerce_promotion/0f614dd7-161d-481f-81e4-ff2fe53af3bc/relationships/stores",
                        "related": "http://drupal85.dev/jsonapi/commerce_promotion/commerce_promotion/0f614dd7-161d-481f-81e4-ff2fe53af3bc/stores"
                    }
                },
                "coupons": {
                    "data": []
                }
            },
            "links": {
                "self": "http://drupal85.dev/jsonapi/commerce_promotion/commerce_promotion/0f614dd7-161d-481f-81e4-ff2fe53af3bc"
            }
        }
    ],
    "links": {
        "self": "http://drupal85.dev/jsonapi/commerce_promotion/commerce_promotion?q=jsonapi/commerce_promotion/commerce_promotion&_format=api_json%3F"
    }
}

add comments on DataDefinition::create('any'), and reduce some unneeded changed code by phpstorm

change $PropertyDefinitions to $property_definitions because it don't conform to php code standards

If the element of the map value is array , define it as map, else define it as any

Change directly getting PropertyDefinition to implementing setProperDefinition to speed up,
Because the code of PropertyDefinition just execute on the moment of data creating.

change(patch):
1.code standards
2.Change directly getting PropertyDefinition to implementing setProperDefinition to speed up,
Because the code of PropertyDefinition just execute on the moment of data creating.

The faling auto test LinkViewsTokensTest still fail when not apply the patch,
Below is the image that executed by myself when not apply the patch,

the #50 patch-8 use setProperDefinition has bug,
So the #46 patch-6 should be the right solution;
The patch-9 is same as patch6 with a little code standars changing

change judge if $this->definition->getPropertyDefinitions is empty to judge $properties,
So the code will be more nice

Above is the running result of LinkViewsTokensTest on https://simplytest.me,it's a refresh installing of 8.5.x with no other modules
So the failing test is not caused by the patch

I am wondering whether this is the right level of the fix. If you think about it this change results in getProperties to be more dynamic.

If you ask about the properties of this field on the schema level, you will still return no values, given the values depend on entity per entity level.
Then on the actual runtime you have different values and you end up with different properties.

Let's compare \Drupal\Core\TypedData\Plugin\DataType\Map::toArray and \Drupal\Core\Field\Plugin\Field\FieldType\MapItem::toArray ... as you see Map doesn't take into account its properties. On the other hand \Drupal\Core\Config\Schema\ArrayElement::getProperties is actually doing basically exactly what you need.

Given that I think the fix is on the right level!

Steps I think we need to do to get this in:

• Not only provide a test on the typed data but maybe actually a REST test with a MapItem field
• Update the issue summary with the solution we actually implement.
• Fix some minor english issues in your code comments. I'm happy to take it, if you want to

1. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Map.php
   @@ -155,11 +157,40 @@ protected function writePropertyValue($property_name, $value) {
   +    // If module creator send an array to map dataType and don't define setPropertyDefinition,
   +    // Auto creating the PropertyDefinition so that it can be normalized
   

   It would be nice to point to the MapItem field type ...

2. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Map.php
   @@ -155,11 +157,40 @@ protected function writePropertyValue($property_name, $value) {
   +    if (empty($properties) && get_class($this->getDataDefinition()) == 'Drupal\Core\TypedData\MapDataDefinition') {
   

   What about using instanceof and !is_subclass_of ?

3. +++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Map.php
   @@ -155,11 +157,40 @@ protected function writePropertyValue($property_name, $value) {
   +      $values = $this->values;
   +      if (is_array($values)) {
   

   Can values ever be NULL or something like this?

@dawehner Thanks for your advice,
I have optimized the code about the 2 and 3 of your advices;

It would be nice to point to the MapItem field type ...

Please help me complete the comments,Thank you!

I removed some level of ifs, and expanded the test coverage as well as expanded the documentation.

Sadly there is a failure on buildExampleTypedDataWithProperties. @liuxin Do you want to try to fix that failure? It seems at least not obvious for me.

@dawehner ok,I will fix the failure today

@dawehner
The testMapWithPropertiesNormalize will still fail without the modification of Map.php
So the failure didn't caused by the patch,It may should not be fixed here（still fixed on the 2895532-65.patch）.

Above is the flow chart of execute $serializer->normalize($typed_data, 'json');

When executing to the
core/lib/Drupal/Core/TypedData/TypedDataManager.php:173

    // Property path for the requested data object. When creating a list item,
    // use 0 in the key as all items look the same.
    $parts[] = $object->getPropertyPath() . '.' . (is_numeric($property_name) ? 0 : $property_name);
    $key = implode(':', $parts);
...
    $property = clone $this->prototypes[$key];

We can see that if the array data of map has no key or has custom numerical key,
There will be only one property (the first numerical value's property) will return

The original design didn't take into consideration of when use setPropertyDefinition, We may pass an array that has numerical key with different type of value
So I change (is_numeric($property_name) ? 0 : $property_name) to

        $parts[] = $object->getPropertyPath() . '.' . $property_name;

It feels we need someone with typed data knowledge who can judge whether having numeric keys is actually meant to be supported. I pinged @berdir about that.

Add Core's link options(Map dataType) serialization test

Change: code standard

Remove whitespace

Remove whitespace

delete my comment because misunderstand

+++ b/core/lib/Drupal/Core/TypedData/Plugin/DataType/Map.php
@@ -160,6 +162,33 @@ public function getProperties($include_computed = FALSE) {
+    if (empty($properties) && $this->getDataDefinition() instanceof MapDataDefinition) {
+      foreach ($this->values as $key => $value) {
+        if (!empty($value) && is_array($value)) {
+          // Recursively create the data definition for each element of the child array.
+          $properties[$key] = $this->getTypedDataManager()->create(
+            MapDataDefinition::create(),
+            $value,
+            $key
+          );
+        }
+        else {
+          $properties[$key] = $this->getTypedDataManager()->create(
+          // For scalar values we use 'any' as we don't have more information about it.
+          // If you have more information about your properties, please use setPropertyDefinition.
+            DataDefinition::create('any'),
+            $value,
+            $key
+          );
+        }
+      }
+    }

This seems overly complex to fake DataDefinitions just to get normalization to work.

I know this is tricky problem and earlier in the issue changing ComplexDataNormalizer was tried but what if a new MapNormalizer was added but use supportsNormalization() to make sure this normalizer only is used when getPropertyDefinitions() returns and empty value?

This patch tries that. The tests in the current patch still pass.

delete because misunderstand

Hi @tedbow Thanks for the advice
Two methods both work
But I don't think it's a good idea

If the water pipe inside the wall leaking, we should fix the water pipe not the wall

MapNormalizer ignores the root cause: missing property definition

And the map was used or inherited on many place like field, So we shouldn't increase the system's complexity

If you think

This seems overly complex to fake DataDefinitions just to get normalization to work.

I suggest use setPropertyDefinition instead like #50

The reason why I don't use #50, Because it has a little bug waiting to be fixed
(#50 bug:When the commerce_promotion's condition both choose curruency and order_total_price,It just set the first element's property definition ,The data modle is ["map-1" => ["map-2" => ["map-3a" => "value3a", "map-3b" => "value3b"]]];)

UPDATE: I had typo in \Drupal\serialization\Normalizer\MapNormalizer::supportsNormalization which caused a bunch errors, fixing

And the map was used or inherited on many place like field, So we shouldn't increase the system's complexity

\Drupal\Core\Field\FieldItemBase extends Map so because the priority of the new normalizer is higher it will be selected in \Symfony\Component\Serializer\Serializer::getNormalizer but only if there are no data definitions.

MapNormalizer ignores the root cause: missing property definition

I don't think this actually a problem but by intention.
I think root cause is actually getNormalizer() is not selecting a normalizer that cannot handle a Map with no data definitions. We should make sure our normalizers handle our typed data types not change for the Serialization module.

If we look at the class comment for Map

 * The "map" data type represent a simple complex data type, e.g. for
 * representing associative arrays. It can also serve as base class for any
 * complex data type.
 *
 * By default there is no metadata for contained properties. Extending classes
 * may want to override MapDataDefinition::getPropertyDefinitions() to define
 * it.

Maps are used when you can't know the keys beforehand and they will change with every instances. I would guess that if they are being used where you can keys beforehand and they are the same on every instance then you should actually have a data definition.

Also if we look at the class comment for \Drupal\Core\TypedData\DataDefinition

/**
 * A typed data definition class for defining data based on defined data types.
 */

It sounds like the previous patch was not sticking to this. It using the data definition to describe the values which are not typed data.

Hi @tedbow, Thanks for your so detailed explanation
I have tested the 2895532-90.patch for every extreme case that I have known, Every case works well.

depends on your comment

Maps are used when you can't know the keys beforehand and they will change with every instances

data will change with every instances
I re-debug the previous patch, And confirm that MapNormalizer is a better choice.

@liuxin thanks, glad you agree.

One thing I was wondering, should we name it something different than MapNormalizer since it is really a normalizer that should will only take effect if there are no data definitions? Maybe DefinitionlessMapNormalizer. Or maybe it is fine

Awesome work here!

@dawehner said this in #72:

It feels we need someone with typed data knowledge who can judge whether having numeric keys is actually meant to be supported. I pinged @berdir about that.

We still need a Typed Data expert to sign off.

@dawehner said this in #60:

Not only provide a test on the typed data but maybe actually a REST test with a MapItem field

We still need this. See \Drupal\Tests\datetime\Functional\EntityResource\EntityTest\EntityTestDatetimeTest for an example where we're testing \Drupal\datetime\Plugin\Field\FieldType\DateTimeItem.

This will guarantee that the actual REST responses, the REST developer experience will never regress in the future.

Patch review:

1. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
   @@ -168,9 +168,12 @@ public function getPropertyInstance(TypedDataInterface $object, $property_name,
   +    // the original code is:
   +    // code$parts[] = $object->getPropertyPath() . '.' . (is_numeric($property_name) ? 0 : $property_name);
   

   We can't leave old code around in a comment.

   This entire comment needs to be cleaned up.

2. +++ b/core/modules/serialization/serialization.services.yml
   @@ -71,6 +71,10 @@ services:
   +      - { name: normalizer, priority: 25}
   

   We need a comment to explain why this priority was chosen. It would be even better if we'd be able to remove this priority.

3. +++ b/core/modules/serialization/src/Normalizer/MapNormalizer.php
   @@ -0,0 +1,34 @@
   + * This normalizer only supports Map objects that do not have have property
   + * definitions.
   

   Comment could indicate what happens for Map objects that *do* have property definitions.

4. +++ b/core/modules/serialization/src/Normalizer/MapNormalizer.php
   @@ -0,0 +1,34 @@
   +class MapNormalizer extends TypedDataNormalizer {
   ...
   +  protected $supportedInterfaceOrClass = '\Drupal\Core\TypedData\Plugin\DataType\Map';
   

   This is interesting — this is normalizing at the @DataType level, not the @FieldType level!

   Unlike many other normalizers.

   This is actually preferable, because it means the JSON API module will be able to benefit from this too — see #2860350: Document why JSON API only supports @DataType-level normalizers!

@Wim Leers thanks and making sure it is not RTBC as committer would have definitely kicked it back, so saves us time.

1. fixed
2. Change the priority to 20 and gave a long reason in the inline comment.
   tl;dr
   • It has to more than serializer.normalizer.complex_data
   • It could be 1 but that leaves not remove for custom normalizers with priorities between the 2 normalizers.
3. Ughh, yeah I feel if we get into that we basically have to explain how the whole serialization system works with priority, format, supportsNormalization and $supportedInterfaceOrClass, I am not sure that belongs in anyone normalizer.
   Basically if for core if the class does not extend FieldItemInterface then it will be handled by ComplexDataNormalizer if it does will be handled by \Drupal\serialization\Normalizer\FieldItemNormalizer unless it is format is hal or it is or extends another interface/class that another normalizer with a higher priority supports.
4. yes like, ComplexDataNormalizer

So #94 \Drupal\Tests\field\Kernel\FieldTypePluginManagerTest::testMainProperty failed because it was looking the main property value this is because \Drupal\Core\Field\FieldItemBase::mainPropertyName returns value

so instead of overriding mainPropertyName() in the test field item it makes sense to just change the column name to value.

Also I didn't notice \Drupal\Core\Field\Plugin\Field\FieldType\MapItem already existed but it throws an exception if I try to use it in the test.

+++ b/core/modules/serialization/src/Normalizer/MapNormalizer.php
@@ -0,0 +1,34 @@
+      if ($definition instanceof ComplexDataDefinitionInterface && empty($definition->getPropertyDefinitions())) {
+        return TRUE;

This needs some documentation ...
Personally I still think the old way was the right way to fix it, see#60, given it actually not only fixes bugs in normalization, but in all the code relying on the properties being there. I am wondering, do we really not care?

@skyredwang
Dear mentor , below is my survey result:
The commerce order's field: adjustments defines an 'any' property on
modules/contrib/commerce/modules/order/src/Plugin/Field/FieldType/AdjustmentItem.php:29

$properties['value'] = DataDefinition::create('any')

And pass an object Drupal\commerce_order\Adjustment to $this->value on setValue() function
modules/contrib/commerce/modules/order/src/Plugin/Field/FieldType/AdjustmentItem.php:46

  public function setValue($values, $notify = TRUE) {
    if (is_array($values)) {
      // The property definition causes the adjustment to be in 'value' key.
      $values = reset($values);
    }
    if (!$values instanceof Adjustment) {
      $values = NULL;
    }
    parent::setValue($values, $notify);
  }

Serialization: first normalize data then encode data
But the normalize result is object, object can not be encode, so it can not be serialized

Proposed resolution:

1. Core type data’s normalize level(best):

   like 'any' type data's doc says:

   nor is it mappable to any primitive type. Thus, it may contain any PHP data for
   which no further metadata is available.

   so i think the right type data's design should be:

   1. When the data is object,we should use ‘any’, and the ‘any’ will renormalize the object
   2. When the data like ‘key’=>$object, we should use ‘map'

   Pros:

   1. ease custom module’s work
   2. accord with ‘any’ typedata’s definition

   Cons:

   1. like commerce order’s adjustment, must change two place(create two issue,one for core, one for commerce)

   ①.core’s type data(core)(ps: just an example for design's idea, the code below may not right)

   class TypedDataNormalizer extends NormalizerBase { 
   
     protected $supportedInterfaceOrClass = 'Drupal\Core\TypedData\TypedDataInterface’;
   
     public function normalize($object, $format = NULL, array $context = []) {
       $value = $object->getValue();
       return !is_object($value)?$value:$this->serializer->normalize($value, $format, $context);
     }
   
    }

   ②. Make Adjustment object could be normalized,but it should be easy(modules)

2. Custom module(commerce) level: don’t pass object in setValue() (not good)

   pros:

   1. Can solve the problem without hack core

   Cons:

   1. This work of changing is huge and complicated,
   2. it violate the ‘any’ typedata’s original intention of design(most important)
   3. Many other module must change lot of code

3. Write custom normalizer for object (worst)

   Pros: 1.the most simplest and violent resolution
   Cons: 1.make all the custom module do many similar boring works other than business logic,then make drupal worse,not better

Besides: If we choose the first resolution, The issue about map's normalization should made similar change.

@dawehner re #97

Personally I still think the old way was the right way to fix it, see#60, given it actually not only fixes bugs in normalization, but in all the code relying on the properties being there.

(emphasis added)

I think the problem is code should not be relying on properties being there. Looking at the phpdoc for Map

* By default there is no metadata for contained properties. Extending classes
* may want to override MapDataDefinition::getPropertyDefinitions() to define
* it.

It think problem is with \Drupal\Core\TypedData\Plugin\DataType\Map::toArray. I think it should be taking into account the case where the Map object doesn't have properties because it seems like they don't in the TypedData sense.

It seems to me the at TypeDataInterface

   * Constructs a TypedData object given its definition and context.

The definition here defines a type of data. Not an individual instance values which the other method was describing.

@tedbow

I think it should be taking into account the case where the Map object doesn't have properties because it seems like they don't in the TypedData sense.

If this is right,

1. data with no property defined shouldn't use type data,
2. Drupal shouldn't **define MapNormlizer for map data withno property definition** either,

core/lib/Drupal/Core/TypedData/DataDefinition.php?h=8.5.x#n17

class DataDefinition implements DataDefinitionInterface, \ArrayAccess {
   ...
  protected $definition = [];
   ...
    public function setDataType($type) {
    $this->definition['type'] = $type;
    return $this;
  }
  ...
    public function setLabel($label) {
    $this->definition['label'] = $label;
    return $this;
  }
  ...

core/lib/Drupal/Core/TypedData/MapDataDefinition.php?h=8.5.x#n8
class MapDataDefinition extends ComplexDataDefinitionBase {

core/lib/Drupal/Core/TypedData/ComplexDataDefinitionBase.php?h=8.5.x#n15

abstract class ComplexDataDefinitionBase extends DataDefinition implements ComplexDataDefinitionInterface {
...
  protected $propertyDefinitions;

we can see that propertyDefinitions != definition

Map object doesn't have properties because it seems like they don't in the TypedData sense.
It seems to me the at TypeDataInterface

* Constructs a TypedData object given its definition and context.

The definition here defines a type of data. Not an individual instance values which the other method was describing.

Map object which doesn't have properties(propertyDefinitions),but it has definition, so it accord Conform to the TypeDataInterface‘s phpdoc, So "Map object doesn't have properties seems like they don't in the TypedData sense" is not right

#98's problem : store an object to map or any,It an't been normalize and serialized
I create a issue https://www.drupal.org/node/2915705 and upload the patch
Both 2895532-80.patch and 2895532-96.patch will work combine https://www.drupal.org/node/2915705's patch

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestMapItemNormalizerTest.php
   @@ -0,0 +1,95 @@
   +    'key2' => 'no, val you',
   

   😂

2. +++ b/core/modules/serialization/serialization.services.yml
   @@ -71,6 +71,24 @@ services:
   +    # This normalizer must be higher than serializer.normalizer.complex_data so
   +    # that serializer.normalizer.complex_data is not used for Map objects that
   +    # do not provide property definitions. While giving this normalizer a
   

   Would it be possible to check for this case in ComplexDataNormalizer::supportsNormalization() and return FALSE to avoid worrying about priorities?

3. +++ b/core/modules/serialization/serialization.services.yml
   @@ -71,6 +71,24 @@ services:
   +    # This normalizer must be higher than serializer.normalizer.complex_data so
   +    # that serializer.normalizer.complex_data is not used for Map objects that
   +    # do not provide property definitions. While giving this normalizer a
   
   +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/MapItem.php
   --- /dev/null
   +++ b/core/tests/Drupal/Tests/Core/TypedData/MapDataNormalizeTest.php
   
   +++ b/core/tests/Drupal/Tests/Core/TypedData/MapDataNormalizeTest.php
   @@ -0,0 +1,137 @@
   +class MapDataNormalizeTest extends KernelTestBase {
   

   MapDataNormalizerTest? Also, shouldn't this test be this be in the serialization module?

4. +++ b/core/modules/serialization/src/Normalizer/MapNormalizer.php
   @@ -0,0 +1,34 @@
   +  protected $supportedInterfaceOrClass = '\Drupal\Core\TypedData\Plugin\DataType\Map';
   

   Nit:

   use Drupal\Core\TypedData\Plugin\DataType\Map;
   ...
   protected $supportedInterfaceOrClass = Map::class;
   

5. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/MapItem.php
   @@ -0,0 +1,45 @@
   + * Defines the 'map' field type.
   

   Nit: "the 'map_test' field type."

6. +++ b/core/tests/Drupal/Tests/Core/TypedData/MapDataNormalizeTest.php
   @@ -0,0 +1,137 @@
   +   * The serializer type.
   

   Your mind's on "types" isn't it? s/type/service/

7. +++ b/core/tests/Drupal/Tests/Core/TypedData/MapDataNormalizeTest.php
   @@ -0,0 +1,137 @@
   +   * The typeDataManager.
   

   "typed data manager.'

8. +++ b/core/tests/Drupal/Tests/Core/TypedData/MapDataNormalizeTest.php
   @@ -0,0 +1,137 @@
   +   * Test whether map data with properties can be normalized
   

   Missing ending . (dot).

9. +++ b/core/tests/Drupal/Tests/Core/TypedData/MapDataNormalizeTest.php
   @@ -0,0 +1,137 @@
   +   * Builds some example type data object with no properties.
   ...
   +   * Builds some example type data object with properties.
   

   s/type/typed/

2. It think it makes more sense to use priorities here.
Well I guess you could inComplexDataNormalizer::supportsNormalization() but in addition to empty($definition->getPropertyDefinitions()) you would also have to check $data instanceof \Drupal\Core\TypedData\Plugin\DataType\Map where as in \Drupal\serialization\Normalizer\MapNormalizer::supportsNormalization parent::supportsNormalization($data, $format)

The other problem could be that another module may have added a special case of normalizer where protected $supportedInterfaceOrClass = 'Drupal\Core\TypedData\ComplexDataInterface'; that they want to override core's ComplexDataNormalizer and they used a higher priority to so that it supersedes core normalizer. In that case if we just update ComplexDataNormalizer::supportsNormalization() not the serializer.normalizer.complex_data priority then the custom normalizer would still handle Map::class

We are already using priorities similarly to make the wrong normalizer doesn't get used in serializer.normalizer.timestamp_item , password_field_item at least. for instance instead of setting the priority of serializer.normalizer.timestamp_item we could have updated FieldItemNormalizer::supportsNormalization() to check
if (!$data instanceof TimestampItem::class)

I think we should keep the pattern of using priorities for this unless we have good reason not to.

3. Fixed and move to serialization module
4. fixed
5. fixed
6. fixed
7. fixed
8. fixed
9. fixed

#106.2 makes sense to me. Thanks for the thorough explanation.

Reuploading a renamed patch for the testbot.

This seems to be working quite well. One thing I've tried locally is to extend the MenuLinkContentResourceTestBase class to we can also test the link base field there and test normalize/denormalize.

Here's what I did which was passing on one test that I tried:

diff --git a/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentResourceTestBase.php
index 0b1f967387..fa5072639b 100644
--- a/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentResourceTestBase.php
@@ -59,7 +59,15 @@ protected function createEntity() {
       'id' => 'llama',
       'title' => 'Llama Gabilondo',
       'description' => 'Llama Gabilondo',
-      'link' => 'https://nl.wikipedia.org/wiki/Llama',
+      'link' => [
+        'uri' => 'https://nl.wikipedia.org/wiki/Llama',
+        'options' => [
+          'fragment' => 'a-fragment',
+          'attributes' => [
+            'class' => ['example-class'],
+          ],
+        ],
+      ],
       'weight' => 0,
       'menu_name' => 'main',
     ]);
@@ -81,6 +89,12 @@ protected function getNormalizedPostEntity() {
       'link' => [
         [
           'uri' => 'http://www.urbandictionary.com/define.php?term=drama%20llama',
+          'options' => [
+            'fragment' => 'a-fragment',
+            'attributes' => [
+              'class' => ['example-class'],
+            ],
+          ],
         ],
       ],
       'bundle' => [
@@ -115,7 +129,12 @@ protected function getExpectedNormalizedEntity() {
         [
           'uri' => 'https://nl.wikipedia.org/wiki/Llama',
           'title' => NULL,
-          'options' => [],
+          'options' => [
+            'fragment' => 'a-fragment',
+            'attributes' => [
+              'class' => ['example-class'],
+            ],
+          ],
         ],
       ],
       'weight' => [

+++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/MapItem.php
@@ -0,0 +1,45 @@
+ * Defines the 'map_test' field type.
+ *
+ * @FieldType(
+ *   id = "map_test",
+ *   label = @Translation("Map Test"),
+ *   description = @Translation("Another dummy field type."),
+ * )
+ */
+class MapItem extends FieldItemBase {

May I ask why we don't use the MapItem field in Drupal core? By doing so we would have some additional test coverage for that one.

For #110

In #109 @Berdir says

Here's what I did which was passing on one test that I tried:

- should we add a test for that?

1. +++ b/core/modules/serialization/serialization.services.yml
   @@ -71,6 +71,24 @@ services:
   +    # not allow for any other custom normalizers to given priority between
   

   to be given?

2. +++ b/core/modules/serialization/serialization.services.yml
   @@ -71,6 +71,24 @@ services:
   +    # normalizer an especially high priority allows normalizers with range of
   +    # priorities for other classes that do not provide property definitions.
   

   a range?

3. +++ b/core/modules/serialization/serialization.services.yml
   @@ -71,6 +71,24 @@ services:
   +    # Giving this normalizer an especially
   +    # high priority will NOT cause it to be used for most classes that extend
   

   no need for the line break here

I applied patch from #107 and it worked for me, thanks !
I tested this having the following modules enabled:
- Link
- LInk attributes
- Rest

Attaching screenshot after applying.
Passing to RTBC.

I think the small changes in #111 still need to be addressed -- most are small docs issues but it sounds like there's an additional test case we could add. Leaving RTBC for the moment though. Thanks!

This resolves the few comment fixes identified in #111.

@samuel.mortenson will look into tests for this based on #110 and #111.

I copied @Berdir's test from #109 and it seems to work fine, I think it's good coverage to add and we always need more llamas in core.

For #110 - I think @dawehner brings up a good point, there is no core "MapItem" field in core, but fields like LinkItem use map data. I'll open a follow up to discuss adding one, because why not?

Created #2946233: Add a MapItem field to core for storing arbitrary serialized data.

--- a/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentResourceTestBase.php

@@ -59,7 +59,15 @@ protected function createEntity() {
-      'link' => 'https://nl.wikipedia.org/wiki/Llama',
+      'link' => [
+        'uri' => 'https://nl.wikipedia.org/wiki/Llama',
+        'options' => [
+          'fragment' => 'a-fragment',
+          'attributes' => [
+            'class' => ['example-class'],
+          ],
+        ],
+      ],

We changed the creation of link item to be from string to an array. That is fine because are not testing creating field values here.

\Drupal\Tests\link\Kernel\LinkItemTest tests creating the field value with a string or an array so that is covered.

I read through everything since my last comment/patch in #106 right before it was RTBC. It was all addressed.

🎉

I'm working on a review. My stupid computer froze, so I lost most of my review :/ Stay tuned. This will not remain RTBC, so un-RTBC'ing to ensure it won't get committed in the mean time.

First, some trivial bits:

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/MenuLinkContent/MenuLinkContentResourceTestBase.php
   @@ -59,7 +59,15 @@ protected function createEntity() {
   -      'link' => 'https://nl.wikipedia.org/wiki/Llama',
   +      'link' => [
   +        'uri' => 'https://nl.wikipedia.org/wiki/Llama',
   +        'options' => [
   +          'fragment' => 'a-fragment',
   +          'attributes' => [
   +            'class' => ['example-class'],
   +          ],
   +        ],
   +      ],
   

   ❤️ this test coverage — thanks @samuel.mortenson in #117 for doing this, and thanks to @Berdir in #109 for suggesting it so Sam basically only had to c/p it :)

   I checked other entity types. Both Item and Feed have 'link' fields … but they're actually using @FieldType=uri — IOW they're poorly named.

   The only other entity type with a @FieldType=link base field is Shortcut. Seems worth updating its test coverage too.

2. +++ b/core/modules/serialization/tests/src/Kernel/MapDataNormalizerTest.php
   @@ -0,0 +1,137 @@
   +      ->setPropertyDefinition('key4', MapDataDefinition::create()
   +        ->setPropertyDefinition(0, DataDefinition::create('boolean'))
   +        ->setPropertyDefinition(1, DataDefinition::create('string'))
   +        ->setPropertyDefinition('key7', DataDefinition::create('string'))
   

   Nit: inconsistent indentation. Fixed.

3. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/MapItem.php
   @@ -0,0 +1,45 @@
   + *   id = "map_test",
   ...
   +class MapItem extends FieldItemBase {
   

   We already have \Drupal\Core\Field\Plugin\Field\FieldType\MapItem. If we're going to keep this, it should be called MapTestItem. And it could then subclass the existing MapItem.

   And then this:

   +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/MapItem.php
   @@ -0,0 +1,45 @@
   +  public static function schema(FieldStorageDefinitionInterface $field_definition) {
   

   This would not be necessary.

The more complex review points will follow in my next comment.

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestMapItemNormalizerTest.php
@@ -0,0 +1,95 @@
+ */
+class EntityTestMapItemNormalizerTest extends EntityTestResourceTestBase {
+
+  use AnonResourceTestTrait;
+
+  /**
+   * {@inheritdoc}
+   */
+  protected static $format = 'json';
+
+  protected static $mapValue = [
+    'key1' => 'value',

We need a test for this as well for hal_json.

I tested it on an actual entity, and what happened is that the properties inside the field were pushed to the very top of the entity.

The reason is that hal field normalizers are expected to return a top-level structure, keyed by the field name, so they can add stuff to top-level _links.

That means we need something like a HalMapFieldItemNormalizer that returns the expected structure for hal and has a higher prio than the default MapNormalizer.

(Crosspost, sorry for the unassign)

I did an extremely deep dive on this issue, because it's important to get this right — committing an incomplete solution may put us in a place in the future where we have to break BC, which would be a bad place to be in.

Please read the following review points in sequence before starting to reply to them. The later ones build on the previous ones.

#117: actually, core does have \Drupal\Core\Field\Plugin\Field\FieldType\MapItem.

1. +++ b/core/modules/serialization/serialization.services.yml
   @@ -71,6 +71,24 @@ services:
   +    # @see \Drupal\serialization\Normalizer\MapNormalizer::supportsNormalization
   +      - { name: normalizer, priority: 20}
   
   +++ b/core/modules/serialization/src/Normalizer/MapNormalizer.php
   @@ -0,0 +1,35 @@
   + * This normalizer only supports Map objects that do not have have property
   + * definitions.
   ...
   +  public function supportsNormalization($data, $format = NULL) {
   +    /* @var \Drupal\Core\TypedData\Plugin\DataType\Map $data  */
   +    if (parent::supportsNormalization($data, $format)) {
   +      $definition = $data->getDataDefinition();
   +      if ($definition instanceof ComplexDataDefinitionInterface && empty($definition->getPropertyDefinitions())) {
   +        return TRUE;
   +      }
   +    }
   +    return FALSE;
   +  }
   

   This seems fairly unmaintainable.

   We're overriding ComplexDataNormalizer simply because @DataType=map is schemaless by default (no property definitions). To fix it, we're special-casing schemaless @DataType=map occurrences to let it be handled by MapNormalizer … but we're not handling @DataType=map occurrences that do have a schema (that do have property definitions).

   And this then results in the parent class's ::normalize() being called, which happens to be \Drupal\serialization\Normalizer\TypedDataNormalizer::normalize().

   At minimum we should be more explicit in our naming, and make clear in the class name and the service name that this only deals with some @DataType=map occurrences: PropertylessMapNormalizer. It then suddenly becomes also much clearer why we handle some cases but not others. In surprising contrast, the class docblock does already explain this!

   Done.

2. I think it'd also be preferable to have explicit logic for normalizing @DataType=map, rather than relying on a parent method. But that's a matter of preference. As long as we have sufficiently strong test coverage, I'd be fine with either.
3. The REST integration test coverage for MenuLinkContent and Shortcut does verify the return TRUE case. \Drupal\Tests\rest\Functional\EntityResource\EntityTest\EntityTestMapItemNormalizerTest makes it seem like it's testing the return FALSE case, but it's NOT!

   It's testing MapTestItem (@FieldType=map_test), which is something entirely different! That still uses the same propertyless @DataType=map under the hood (MapDataDefinition::create()->setLabel(t('Freeform')),).

   This means we have zero test coverage for the return FALSE case.

4. This brings us to surprising aspects of \Drupal\Tests\rest\Functional\EntityResource\EntityTest\EntityTestMapItemNormalizerTest: why is it testing with MapTestItem instead of core's \Drupal\Core\Field\Plugin\Field\FieldType\MapItem as @dawehner asked? Because it specifies ['value' => …] to be the property definitions, rather than []. Why does it do that?

   Let me take you on a wild ride deep into the depths of Entity/Field/Typed Data API and Drupal 8 git/issue queue archeology. Grab some popcorn or perhaps a coffee. 🍿☕️

   …

   Here we go: all this confusing behavior exists in core because #2563843: MapItem is broken for configurable fields tried to remove core's @FieldType=map; hat removal was committed, but then reverted because some contrib modules already started using it, even though D8 had not even been released. Entity API maintainer @tstoeckler pointed out in #2563843-37: MapItem is broken for configurable fields that consequently #2229181: MapItem::propertyDefinitions() claims it has a static value property, which is wrong needs to be rolled back (which is the code that caused property definitions to never be defined for @FieldType=map. In other words: the current MapItem
   (@FieldType=map) implementation is nonsensical/broken. Read the comment I linked to for details.

   @tstoeckler then created #2887105: MapItem violates FieldItemInterface::schema()'s contract - breaks widgets specifically to fix this. At that point (with #2887105 in) … MapTestItem would be 100% identical to MapItem. Effectively, MapTestItem is a work-around for this error:

   Undefined index: value
   
   /Users/wim.leers/Work/d8/vendor/symfony/phpunit-bridge/DeprecationErrorHandler.php:270
   /Users/wim.leers/Work/d8/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php:2086
   /Users/wim.leers/Work/d8/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php:1323
   /Users/wim.leers/Work/d8/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php:1309
   /Users/wim.leers/Work/d8/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php:430
   /Users/wim.leers/Work/d8/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1474
   /Users/wim.leers/Work/d8/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1527
   /Users/wim.leers/Work/d8/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1475
   /Users/wim.leers/Work/d8/core/lib/Drupal/Core/Field/FieldStorageDefinitionListener.php:97
   /Users/wim.leers/Work/d8/core/lib/Drupal/Core/Entity/EntityManager.php:573
   /Users/wim.leers/Work/d8/core/modules/field/src/Entity/FieldStorageConfig.php:334
   /Users/wim.leers/Work/d8/core/modules/field/src/Entity/FieldStorageConfig.php:288
   /Users/wim.leers/Work/d8/core/lib/Drupal/Core/Entity/EntityStorageBase.php:434
   /Users/wim.leers/Work/d8/core/lib/Drupal/Core/Entity/EntityStorageBase.php:389
   /Users/wim.leers/Work/d8/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:259
   /Users/wim.leers/Work/d8/core/lib/Drupal/Core/Entity/Entity.php:387
   /Users/wim.leers/Work/d8/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:637
   /Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestMapItemNormalizerTest.php:66
   /Users/wim.leers/Work/d8/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:180
   

   … which is caused by this line in \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getDedicatedTableSchema():

         $data_schema['fields'][$real_name]['not null'] = $properties[$column_name]->isRequired();
   

   … which is explained in detail in the aforementioned @tstoeckler comment at #2563843-37: MapItem is broken for configurable fields. And @joachim reported this at #2887105-4: MapItem violates FieldItemInterface::schema()'s contract - breaks widgets.

   That's right, the simple act of doing:

         FieldStorageConfig::create([
           'entity_type' => 'entity_test',
           'field_name' => 'field_map',
           'type' => 'map',
           'cardinality' => 1,
           'translatable' => FALSE,
         ])->save();
   

   in HEAD causes it to fail with the cited error. If that's not proof that @FieldType=map is broken in HEAD, then I don't know what is. The reason this is not failing for the \Drupal\commerce_log\Entity\Log entity type's params base field or the \Drupal\commerce_order\Entity\Order entity type's base field is simply that they're base fields. See \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getSchemaFromStorageDefinition() which calls \Drupal\Core\Entity\Sql\DefaultTableMapping::requiresDedicatedTableStorage() which calls \Drupal\Core\Entity\Sql\DefaultTableMapping::allowsSharedTableStorage() which checks if it's a base field. IOW: the LoC cited above that fails will only NOT fail for single-cardinality non-base @FieldType=map fields … which is what all entity types so far are doing. If one of them would either be non-base fields (unlikely because @FieldType=map has no_ui=TRUE), or use it with multiple cardinality, then they'd also run into this fail.

   TL;DR: MapTestItem only exists to be able to create a @FieldType=map field with which to test on the generic EntityTest entity type.

   I think this in principle does not need to block this issue.

5. Regarding the previous point: quoting @tstoeckler at #2906600-7: FieldItemList::equals() doesn't work correctly for computed fields with custom storage:
   

   2. Map fields will always be considered equal regardless of their values due to #2887105: MapItem violates FieldItemInterface::schema()'s contract - breaks widgets. Note that even though I consider that a bug I don't think it makes sense to block this (or anything) on that as @Berdir does not consider it a bug, so regardless where we collectively come down on this I don't think it will be fixed anytime soon.

   Since #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior, we rely on ::equals() to ensure safe PATCHing without information disclosure. IOW: if we don't change MapItem::propertyDefinitions(), MapItem::equals() will behave incorrectly. This would not introduce a security vulnerability, but would make it literally impossible to PATCH map fields, since the content entity storage logic uses ::equals() to determine whether data needs to be written.

   (This would not introduce a security hole because including a map field in a PATCH request and setting it to an arbitrary value, if we don't have field edit access but we do have field view access, that'd result in \Drupal\rest\Plugin\rest\resource\EntityResource::checkPatchFieldAccess() returning FALSE which would result in \Drupal\rest\Plugin\rest\resource\EntityResource::patch() NOT setting the received value.)

6. In fixing @DataType=map normalization, this will also for the first time enable @FieldType=map normalization. This means that this issue also must consider @FieldType=map normalization: that normalization should make sense too.

   IMHO, the \Drupal\Tests\rest\Functional\EntityResource\EntityTest\EntityTestMapItemNormalizerTest test that we're adding is asserting a normalization that we DON'T want: the entire map is normalized under a value root key that is an irrelevant implementation detail for API clients. We just want the map we're storing, we don't want to see the data storage column name! And if we want to test this, we need to fix point 4.

   If others agree with this (I suspect they will), we'll also need a MapItemNormalizer.

Attached interdiff/patch fixes all of the above.

1. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
   @@ -168,9 +168,8 @@ public function getPropertyInstance(TypedDataInterface $object, $property_name,
   -    // Property path for the requested data object. When creating a list item,
   -    // use 0 in the key as all items look the same.
   -    $parts[] = $object->getPropertyPath() . '.' . (is_numeric($property_name) ? 0 : $property_name);
   +    // Property path for the requested data object.
   +    $parts[] = $object->getPropertyPath() . '.' . $property_name;
   

   Q: Do we really need these changes?
   A: We don't need it for propertyless @DataType=map, but we do need it for @DataType with defined properties. Reverting this change does cause \Drupal\Tests\serialization\Kernel\PropertylessMapNormalizerTest::testMapWithPropertiesNormalize() to fail.

   👍

2. +++ b/core/modules/link/tests/src/Kernel/LinkItemSerializationTest.php
   @@ -0,0 +1,80 @@
   +   * Modules to enable.
   +   *
   +   * @var array
   ...
   +  protected function setUp() {
   

   Nit: inheritdoc

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestMapItemNormalizerTest.php
   @@ -0,0 +1,91 @@
   + * Test that MapItem are correctly exposed in REST.
   

   Nit: Incomplete sentence.

4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestMapItemNormalizerTest.php
   @@ -0,0 +1,91 @@
   +  protected static $mapValue = [
   

   Nit: Missing doc.

5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestMapItemNormalizerTest.php
   @@ -0,0 +1,91 @@
   +  ];
   +
   +
   +  /**
   

   Nit: Too much whitespace.

6. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestMapItemNormalizerTest.php
   @@ -0,0 +1,91 @@
   +    // The 'non_exposed_value' property in test field type will not return in
   +    // normalization because setExposed(TRUE) was not called for this property.
   +    // @see \Drupal\entity_test\Plugin\Field\FieldType\ExposedPropertyTestFieldItem::propertyDefinitions
   

   Nit: Copy/paste remnant.

7. +++ b/core/modules/serialization/tests/src/Kernel/MapDataNormalizerTest.php
   @@ -0,0 +1,137 @@
   +class MapDataNormalizerTest extends KernelTestBase {
   

   Until the previous patch, this should've been named MapNormalizerTest, now it should be named PropertylessMapNormalizerTest.

8. +++ b/core/modules/serialization/tests/src/Kernel/MapDataNormalizerTest.php
   @@ -0,0 +1,137 @@
   +   */
   +
   +  public static $modules = ['system', 'serialization'];
   

   Nit: whitespace.

9. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/MapTestItem.php
   @@ -0,0 +1,30 @@
   +class MapTestItem extends MapItem {
   

   I forgot to delete this in #124!

10. +++ b/core/modules/serialization/tests/src/Kernel/MapDataNormalizerTest.php
    @@ -0,0 +1,137 @@
    +      ->setPropertyDefinition(0, DataDefinition::create('boolean'))
    +      ->setPropertyDefinition(1, DataDefinition::create('string'))
    +      ->setPropertyDefinition('key7', DataDefinition::create('string'))
    

    Actually, the indentation that I changed in #121 was correct before… but there was some weirdness with the closing parenthesis. Fixed that instead.

Since #121, Drupal\Tests\field\Kernel\FieldTypePluginManagerTest::testMainProperty() fails. Fixed.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/MapItem.php
@@ -22,8 +23,12 @@ class MapItem extends FieldItemBase {
    */
   public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
-    // The properties are dynamic and can not be defined statically.
-    return [];
+    return [
+      // All map fields store their data in a single 'value' property, but this
+      // itself expands to an arbitrary map.
+      // @see \Drupal\Core\TypedData\Plugin\DataType\Map
+      'value' => MapDataDefinition::create()->setLabel(t('Serialized array of values')),
+    ];
   }

As I already commented in the issues you referenced, I do not agree with this change.

The properties are the runtime properties that the object has. This suggests that I can do $entity->field->value, but that is wrong.

Core doesn't have a lot of code that is generic like that, but contrib absolutely does.

The whole point of MapItem is that you have a complex structure with properties at runtime that is stored into a single column, but that part is an implementation detail.

Yes we have documentation that claims properties = columns but I think that is too simplified. I do agree that I didn't spot the mess around MapItem/MapTestItem here and I need to look closer into the specific problem for dedicated tables, but I disagree that this is the solution.

In fixing @DataType=map normalization, this will also for the first time enable @FieldType=map normalization. This means that this issue also must consider @FieldType=map normalization: that normalization should make sense too.

IMHO, the \Drupal\Tests\rest\Functional\EntityResource\EntityTest\EntityTestMapItemNormalizerTest test that we're adding is asserting a normalization that we DON'T want: the entire map is normalized under a value root key that is an irrelevant implementation detail for API clients. We just want the map we're storing, we don't want to see the data storage column name! And if we want to test this, we need to fix point 4.

I also fully agree with this, but this is just looking at it from the normalization/serialization perspective and I need to make one small correction:

> the entire map is normalized under a value root key that is an irrelevant implementation detail for API clients

It should be an implementation detail for everyone. A MapItem is not a field with a map on its value property. It *is* a map.

You now changed propertyDefinitions() to be consistent with schema() but by doing so, made it inconsistent with how MapItem is actually used. It's not $entity->field->value->map_property, it is $entity->field->map_property.

So again, you did spot the problem that I missed (and thanks for that, I feel pretty blind now!), but I don't agree with your solution to it. I'll try to see if I can come up with something else.

I think we perhaps disagree less than you think! :)

#130:

The properties are the runtime properties that the object has.

This, 100%! That's why I made these changes:

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityTest/EntityTestMapItemNormalizerTest.php
@@ -44,9 +44,7 @@ protected function getExpectedNormalizedEntity() {
     $expected['field_map'] = [
-      [
-        'value' => static::$mapValue,
-      ],
+      static::$mapValue
     ];

@@ -85,9 +83,7 @@ protected function createEntity() {
     return parent::getNormalizedPostEntity() + [
       'field_map' => [
-        [
-          'value' => static::$mapValue,
-        ],
+        static::$mapValue,
       ],
     ];

+++ b/core/modules/serialization/src/Normalizer/MapItemNormalizer.php
@@ -0,0 +1,35 @@
+  public function normalize($field_item, $format = NULL, array $context = []) {
+    // Remove the level of indirection: pretend the arbitrary keys stored in the
+    // map are the properties on this field.
+    return parent::normalize($field_item, $format, $context)['value'];
+  }
...
+  protected function constructValue($data, $context) {
+    // Prepare for storage: all received data must be assigned to this field
+    // type's sole property: 'value'.
+    return ['value' => $data];
+  }

All of those are about exactly what you said: the properties are the ones that are stored for this particular map instance, not some hardcoded implementation detail.

The only reason I made that change is to make things work at all on a HTTP API/normalization level.

This suggests that I can do $entity->field->value, but that is wrong.

Well, that actually does work. But I agree it's wrong for the same reason that the way that @FieldType=map was normalized until before the changes I just quoted (which were made in #124), the HTTP API/normalization was wrong. I think we agree on that? We agree on that, per @Berdir's comment in #131 :)

Yes we have documentation that claims properties = columns but I think that is too simplified.

Sure! I have no opinion about that. That's an Entity/Field API implementation detail as far as I'm concerned, I don't know why those things are architected the way they currently work.

#131:

[…] one small correction […] It should be an implementation detail for everyone. A MapItem is not a field with a map on its value property. It *is* a map.

❤️ 🎉 Splendid!

I'll try to see if I can come up with something else.

AFAICT there are two possible explanations/solutions:

1. The API is incomplete/a subset of what it needs to be, because the current required matching between propertyDefinitions() + schema() is getting in the way: it prevents us from expressing what we need to express.
2. The API is fine, but both the docs and \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getDedicatedTableSchema()are incorrect.

I could see it being solved either way (or perhaps in other ways I don't yet see!), but IDK which is the correct approach — that requires a more complete understanding of Entity/Field/Typed Data API than I have! Looking forward to your proposal! :)

When this does reach RTBC again, I think we need a better IS. And an updated CR at this point.

Current TODOs:

1. missing hal_json test coverage pointed out in #122
2. change MapItem::propertyDefinitions() as being discussed in the preceding three comments

I decided to move the storage changes/fixes and my response to the last comment to #2563843: MapItem is broken for configurable fields, together with a kernel test that makes it much easier to test and debug those fixes.

I also changed EntityTestMapItemNormalizerTest a bit as a result of that, without the wrapping value property.

I'm only attaching an interdiff here on top of #121. It passes for me together with the patch from the other issue. I explicitly specified the delta 0 there in all places, that made it easier for me to read the structure and understand what we are working with. We also still need a test for hal_json.

That doesn't mean that we don't want the changes from the following patches from Wim, but it was far easier to test on top of #121 before it started to contain changes to MapItem and other places. We well have to combine all the improvements that we do want from those with my interdiff.

We have to either get the other issue in first *or* we create a test entity type with a map base field so we are not blocked on it. As mentioned, it is not relevant for normalization if it's a base or configurable field and all real use cases out there that are waiting on this are base fields (obviously, since configurable fields are not working at all).

The issue and the actual implementation (which is not yet complete and needs to work for hal as well) do not depend on the other issue, correct.

But the tests do. The only way to be able to test a map base field would be to define a new test entity type in a new module (I'd like to avoid adding more types to entity_test) and use that instead of a configurable field.

I followed @Wim Leers's suggestion in issue TypedData 'Any' can't be normalized to array if it stores an object:

Move all the change from TypedDataNormalizer to a standard alone normalizer: AnyNormalizer.

Then the TypedDataNormalizer won't have the ability to normalize some data like:

$objectExample = new Example();
['property1' => $objectExample,]

or

$objectExample = new Example();
[
'property1' => $objectExample,
'property2' => ['xxx' => $objectExample],
]

So the PropertylessMapNormalizer should extends AnyNormalizer After TypedData 'Any' can't be normalized to array if it stores an object be fixed.
Or we move AnyNormalizer's code to TypedDataNormalizer back.
And we should add test for it in this issue too.

2895532-126.patch can't be applied to drupal8.5.4

This is a re-roll patch, there's no any functional change.

This issue will be 1 year old in 2 days. 😢 The last major round of activity is from 4 months ago.

Let's get this going again. I finished re-reading the entire issue and related issues. Assigning this to me.

Summary of the current state of the issue and next steps

This issue was last RTBC in #119. I un-RTBC'd it in #120 + #121 + #124. #121 and especially #124 contain very detailed analyses of how we got to the current confusing place. @Berdir made corrections in #130 + #131 to my analysis. I agreed with him in #132.

I posted next steps in #133. @Berdir posted a proposed interdiff in #134. Those were never applied to the patch yet. That's what's next.

Of course, before we do that, the patch must apply to HEAD again. @caseylau thankfully already did that a month ago, in #139. That still applies cleanly.

@Wim: Yay. The problem is that the related issue that I mentioned in #134 is blocking the current patch. To avoid a dependency on that, we need to make the current configurable field a base field, either with a custom test entity type or with a base field alter hook based on a state flag because right now, MapItem only properly works as a base field.

Rerolled #2563843-69: MapItem is broken for configurable fields, that patch is now hopefully RTBC'able.

First trying to fix the failures that I introduced in my #124 patch.

1. The same lines I updated in #143, and which I introduced #124 are still causing failures, because in the HAL normalization, reference fields don't show up in the normalization as fields; instead they're under HAL's _links. The code in HEAD didn't suffer from this problem, but it suffered from a different one (hence these changes): accessing $entity->get('some_map-like_field_type')->getValue() puts them under a 'value' key.

   I couldn't get this to pass tests. So rather than continue to work on just making the latest patch pass tests, I think/hope Berdir's work will help us pass tests, by fixing the problem in a better way :)

   Besides, @Berdir kindly wrote:

   That doesn't mean that we don't want the changes from the following patches from Wim, but it was far easier to test on top of #121 before it started to contain changes to MapItem and other places. We will have to combine all the improvements that we do want from those with my interdiff.

   … but I'm not entirely sure that there are any changes worth keeping after #121, especially because they're all centered around changing MapItem in a way that per @Berdir's explanation does not make sense.

2. So … @Berdir's #134 to the rescue! 🙏 He did point out that for that interdiff to work, either #2563843: MapItem is broken for configurable fields needs to land, or this needs to use an entity type that has a @FieldType=map base field. My initial reaction was but then we leave all those who added a "map" field to suffer! — then I realized why @Berdir proposed this: because that field type has no_ui = TRUE in its annotation, meaning that site builders cannot ever set up a "map" field through the UI!

   I tried … and got very very very stuck on applying #134 on top of the latest patch. I thought I lifted the right pieces from #134. But couldn't get it to work. Even with #2563843-69: MapItem is broken for configurable fields applied.

3. So I tried 134-on-top-of-121: all EntityTestMapItemNormalizerTest tests fail with Undefined index: value
4. Next I tried 134-on-top-of-121 plus #2563843-69: MapItem is broken for configurable fields: works! Alright, let's take it from here. ✅ Now I just need to bring back the changes I made after #121.
5. … except that this is then actively blocked by #2563843. Ideally it wouldn't be. So we can take the other path that @Berdir suggested: test with a custom entity type that has a @FieldType=map base field.

So this patch:

1. starts from #121
2. applies @Berdir's #134
3. imports the custom entity type that has a @FieldType=map base field from #2563843-69: MapItem is broken for configurable fields
4. and then updates the test coverage to use that custom entity type instead of adding a @FieldType=map configurable field to an existing entity type

Points 3 and 4 show up in the interdiff.

Moved the test to a proper test base class (also in a different location, per #2910883: Move all entity type REST tests to the providing modules), with a subclass for json (this is what we had in the previous patch).

This then makes it easy to add another subclass for hal_json (necessary per #133.1).

Created HAL test coverage. Noticed that the langcode key on the entity_test_map_field entity type was apparently necessary, so added it. Without making changes to normalizers, @FieldType=map is not being serialized correctly.