From 725e36091e1c4693822174b6204d62bb5f55266c Mon Sep 17 00:00:00 2001 From: Nathaniel Catchpole Date: Thu, 16 Aug 2018 11:35:24 +0900 Subject: [PATCH] Issue #2895532 by caseylau, Wim Leers, tedbow, dawehner, DamienMcKenna, gabesullice, Berdir, mistermoper, skyredwang, larowlan, bojanz: @DataType=map cannot be normalized, affects @FieldType=link, @FieldType=map --- .../Core/TypedData/TypedDataManager.php | 5 +- .../src/Normalizer/FieldItemNormalizer.php | 5 +- ...NotExistingInternalConstraintValidator.php | 2 +- .../src/Kernel/LinkItemSerializationTest.php | 83 ++++++++++ .../Rest/MenuLinkContentResourceTestBase.php | 23 ++- .../EntityResource/EntityResourceTestBase.php | 57 ++++--- .../src/Normalizer/ComplexDataNormalizer.php | 5 +- .../src/Kernel/MapDataNormalizerTest.php | 136 ++++++++++++++++ .../Rest/ShortcutResourceTestBase.php | 7 +- .../src/Entity/EntityTestMapField.php | 39 +++++ .../Hal/EntityTestMapFieldHalJsonAnonTest.php | 103 ++++++++++++ .../Rest/EntityTestMapFieldJsonAnonTest.php | 24 +++ .../EntityTestMapFieldResourceTestBase.php | 152 ++++++++++++++++++ 13 files changed, 610 insertions(+), 31 deletions(-) create mode 100644 core/modules/link/tests/src/Kernel/LinkItemSerializationTest.php create mode 100644 core/modules/serialization/tests/src/Kernel/MapDataNormalizerTest.php create mode 100644 core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMapField.php create mode 100644 core/modules/system/tests/modules/entity_test/tests/src/Functional/Hal/EntityTestMapFieldHalJsonAnonTest.php create mode 100644 core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestMapFieldJsonAnonTest.php create mode 100644 core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestMapFieldResourceTestBase.php diff --git a/core/lib/Drupal/Core/TypedData/TypedDataManager.php b/core/lib/Drupal/Core/TypedData/TypedDataManager.php index dbf0d5fec5..228722fdf0 100644 --- a/core/lib/Drupal/Core/TypedData/TypedDataManager.php +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php @@ -168,9 +168,8 @@ public function getPropertyInstance(TypedDataInterface $object, $property_name, // a shorter string than the serialized form, so array access is faster. $parts[] = json_encode($settings); } - // 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; $key = implode(':', $parts); // Create the prototype if needed. diff --git a/core/modules/hal/src/Normalizer/FieldItemNormalizer.php b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php index 6d10b06f32..f5b2ec07e8 100644 --- a/core/modules/hal/src/Normalizer/FieldItemNormalizer.php +++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php @@ -92,7 +92,10 @@ protected function normalizedFieldValues(FieldItemInterface $field_item, $format // We normalize each individual property, so each can do their own casting, // if needed. /** @var \Drupal\Core\TypedData\TypedDataInterface $property */ - foreach (TypedDataInternalPropertiesHelper::getNonInternalProperties($field_item) as $property_name => $property) { + $field_properties = !empty($field_item->getProperties(TRUE)) + ? TypedDataInternalPropertiesHelper::getNonInternalProperties($field_item) + : $field_item->getValue(); + foreach ($field_properties as $property_name => $property) { $normalized[$property_name] = $this->serializer->normalize($property, $format, $context); } diff --git a/core/modules/link/src/Plugin/Validation/Constraint/LinkNotExistingInternalConstraintValidator.php b/core/modules/link/src/Plugin/Validation/Constraint/LinkNotExistingInternalConstraintValidator.php index 6e2f0cff65..73895a3283 100644 --- a/core/modules/link/src/Plugin/Validation/Constraint/LinkNotExistingInternalConstraintValidator.php +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkNotExistingInternalConstraintValidator.php @@ -30,7 +30,7 @@ public function validate($value, Constraint $constraint) { if ($url->isRouted()) { $allowed = TRUE; try { - $url->toString(); + $url->toString(TRUE); } // The following exceptions are all possible during URL generation, and // should be considered as disallowed URLs. diff --git a/core/modules/link/tests/src/Kernel/LinkItemSerializationTest.php b/core/modules/link/tests/src/Kernel/LinkItemSerializationTest.php new file mode 100644 index 0000000000..dde7dc117f --- /dev/null +++ b/core/modules/link/tests/src/Kernel/LinkItemSerializationTest.php @@ -0,0 +1,83 @@ +installEntitySchema('user'); + $this->serializer = \Drupal::service('serializer'); + + // Create a generic, external, and internal link fields for validation. + FieldStorageConfig::create([ + 'entity_type' => 'entity_test', + 'field_name' => 'field_test', + 'type' => 'link', + ])->save(); + + FieldConfig::create([ + 'entity_type' => 'entity_test', + 'field_name' => 'field_test', + 'bundle' => 'entity_test', + 'settings' => ['link_type' => LinkItemInterface::LINK_GENERIC], + ])->save(); + } + + /** + * Tests the serialization. + */ + public function testLinkSerialization() { + // Create entity. + $entity = EntityTest::create(); + $url = 'https://www.drupal.org?test_param=test_value'; + $parsed_url = UrlHelper::parse($url); + $title = $this->randomMachineName(); + $class = $this->randomMachineName(); + $entity->field_test->uri = $parsed_url['path']; + $entity->field_test->title = $title; + $entity->field_test->first() + ->get('options') + ->set('query', $parsed_url['query']); + $entity->field_test->first() + ->get('options') + ->set('attributes', ['class' => $class]); + $entity->save(); + $serialized = $this->serializer->serialize($entity, 'json'); + $deserialized = $this->serializer->deserialize($serialized, EntityTest::class, 'json'); + $options_expected = [ + 'query' => $parsed_url['query'], + 'attributes' => ['class' => $class], + ]; + $this->assertSame($options_expected, $deserialized->field_test->options); + } + +} diff --git a/core/modules/menu_link_content/tests/src/Functional/Rest/MenuLinkContentResourceTestBase.php b/core/modules/menu_link_content/tests/src/Functional/Rest/MenuLinkContentResourceTestBase.php index 6a6a7ede26..e77a74f230 100644 --- a/core/modules/menu_link_content/tests/src/Functional/Rest/MenuLinkContentResourceTestBase.php +++ b/core/modules/menu_link_content/tests/src/Functional/Rest/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' => [ diff --git a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php index 2814e42487..4d870c26a2 100644 --- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php @@ -617,7 +617,10 @@ public function testGet() { // Only run this for fieldable entities. It doesn't make sense for config // entities as config values are already casted. They also run through the // ConfigEntityNormalizer, which doesn't deal with fields individually. - if ($this->entity instanceof FieldableEntityInterface) { + // Also exclude entity_test_map_field — that has a "map" base field, which + // only became normalizable since Drupal 8.6, so its normalization + // containing non-stringified numbers or booleans does not break BC. + if ($this->entity instanceof FieldableEntityInterface && static::$entityTypeId !== 'entity_test_map_field') { // Test primitive data casting BC (strings). $this->config('serialization.settings')->set('bc_primitives_as_strings', TRUE)->save(TRUE); // Rebuild the container so new config is reflected in the addition of the @@ -921,17 +924,7 @@ public function testPost() { $created_entity = $this->entityStorage->loadUnchanged(static::$firstCreatedEntityId); $created_entity_normalization = $this->serializer->normalize($created_entity, static::$format, ['account' => $this->account]); $this->assertSame($created_entity_normalization, $this->serializer->decode((string) $response->getBody(), static::$format)); - // Assert that the entity was indeed created using the POSTed values. - foreach ($this->getNormalizedPostEntity() as $field_name => $field_normalization) { - // Some top-level keys in the normalization may not be fields on the - // entity (for example '_links' and '_embedded' in the HAL normalization). - if ($created_entity->hasField($field_name)) { - // Subset, not same, because we can e.g. send just the target_id for the - // bundle in a POST request; the response will include more properties. - $this->assertArraySubset(static::castToString($field_normalization), $created_entity->get($field_name) - ->getValue(), TRUE); - } - } + $this->assertStoredEntityMatchesSentNormalization($this->getNormalizedPostEntity(), $created_entity); } $this->config('rest.settings')->set('bc_entity_resource_permissions', TRUE)->save(TRUE); @@ -1182,16 +1175,7 @@ public function testPatch() { $updated_entity = $this->entityStorage->loadUnchanged($this->entity->id()); $updated_entity_normalization = $this->serializer->normalize($updated_entity, static::$format, ['account' => $this->account]); $this->assertSame($updated_entity_normalization, $this->serializer->decode((string) $response->getBody(), static::$format)); - // Assert that the entity was indeed created using the PATCHed values. - foreach ($this->getNormalizedPatchEntity() as $field_name => $field_normalization) { - // Some top-level keys in the normalization may not be fields on the - // entity (for example '_links' and '_embedded' in the HAL normalization). - if ($updated_entity->hasField($field_name)) { - // Subset, not same, because we can e.g. send just the target_id for the - // bundle in a PATCH request; the response will include more properties. - $this->assertArraySubset(static::castToString($field_normalization), $updated_entity->get($field_name)->getValue(), TRUE); - } - } + $this->assertStoredEntityMatchesSentNormalization($this->getNormalizedPatchEntity(), $updated_entity); // Ensure that fields do not get deleted if they're not present in the PATCH // request. Test this using the configurable field that we added, but which // is not sent in the PATCH request. @@ -1516,4 +1500,33 @@ protected function assertResourceNotAvailable(Url $url, array $request_options) } } + /** + * Asserts that the stored entity matches the sent normalization. + * + * @param array $sent_normalization + * An entity normalization. + * @param \Drupal\Core\Entity\FieldableEntityInterface $modified_entity + * The entity object of the modified (PATCHed or POSTed) entity. + */ + protected function assertStoredEntityMatchesSentNormalization(array $sent_normalization, FieldableEntityInterface $modified_entity) { + foreach ($sent_normalization as $field_name => $field_normalization) { + // Some top-level keys in the normalization may not be fields on the + // entity (for example '_links' and '_embedded' in the HAL normalization). + if ($modified_entity->hasField($field_name)) { + $field_type = $modified_entity->get($field_name)->getFieldDefinition()->getType(); + // Fields are stored in the database, when read they are represented + // as strings in PHP memory. The exception: field types that are + // stored in a serialized way. Hence we need to cast most expected + // field normalizations to strings. + $expected_field_normalization = ($field_type !== 'map') + ? static::castToString($field_normalization) + : $field_normalization; + // Subset, not same, because we can e.g. send just the target_id for the + // bundle in a PATCH or POST request; the response will include more + // properties. + $this->assertArraySubset($expected_field_normalization, $modified_entity->get($field_name)->getValue(), TRUE); + } + } + } + } diff --git a/core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php b/core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php index e4fcf52b79..cd14cc014d 100644 --- a/core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php +++ b/core/modules/serialization/src/Normalizer/ComplexDataNormalizer.php @@ -34,7 +34,10 @@ public function normalize($object, $format = NULL, array $context = []) { // Other normalizers that extend this class may only provide $object that // implements \Traversable. if ($object instanceof ComplexDataInterface) { - $object = TypedDataInternalPropertiesHelper::getNonInternalProperties($object); + // If there are no properties to normalize, just normalize the value. + $object = !empty($object->getProperties(TRUE)) + ? TypedDataInternalPropertiesHelper::getNonInternalProperties($object) + : $object->getValue(); } /** @var \Drupal\Core\TypedData\TypedDataInterface $property */ foreach ($object as $name => $property) { diff --git a/core/modules/serialization/tests/src/Kernel/MapDataNormalizerTest.php b/core/modules/serialization/tests/src/Kernel/MapDataNormalizerTest.php new file mode 100644 index 0000000000..32dc082652 --- /dev/null +++ b/core/modules/serialization/tests/src/Kernel/MapDataNormalizerTest.php @@ -0,0 +1,136 @@ +serializer = \Drupal::service('serializer'); + $this->typedDataManager = \Drupal::typedDataManager(); + } + + /** + * Tests whether map data can be normalized. + */ + public function testMapNormalize() { + $typed_data = $this->buildExampleTypedData(); + $data = $this->serializer->normalize($typed_data, 'json'); + $expect_value = [ + 'key1' => 'value1', + 'key2' => 'value2', + 'key3' => 3, + 'key4' => [ + 0 => TRUE, + 1 => 'value6', + 'key7' => 'value7', + ], + ]; + $this->assertSame($expect_value, $data); + } + + /** + * Test whether map data with properties can be normalized. + */ + public function testMapWithPropertiesNormalize() { + $typed_data = $this->buildExampleTypedDataWithProperties(); + $data = $this->serializer->normalize($typed_data, 'json'); + $expect_value = [ + 'key1' => 'value1', + 'key2' => 'value2', + 'key3' => 3, + 'key4' => [ + 0 => TRUE, + 1 => 'value6', + 'key7' => 'value7', + ], + ]; + $this->assertSame($expect_value, $data); + } + + /** + * Builds some example typed data object with no properties. + */ + protected function buildExampleTypedData() { + $tree = [ + 'key1' => 'value1', + 'key2' => 'value2', + 'key3' => 3, + 'key4' => [ + 0 => TRUE, + 1 => 'value6', + 'key7' => 'value7', + ], + ]; + $map_data_definition = MapDataDefinition::create(); + $typed_data = $this->typedDataManager->create( + $map_data_definition, + $tree, + 'test name' + ); + return $typed_data; + } + + /** + * Builds some example typed data object with properties. + */ + protected function buildExampleTypedDataWithProperties() { + $tree = [ + 'key1' => 'value1', + 'key2' => 'value2', + 'key3' => 3, + 'key4' => [ + 0 => TRUE, + 1 => 'value6', + 'key7' => 'value7', + ], + ]; + $map_data_definition = MapDataDefinition::create() + ->setPropertyDefinition('key1', DataDefinition::create('string')) + ->setPropertyDefinition('key2', DataDefinition::create('string')) + ->setPropertyDefinition('key3', DataDefinition::create('integer')) + ->setPropertyDefinition('key4', MapDataDefinition::create() + ->setPropertyDefinition(0, DataDefinition::create('boolean')) + ->setPropertyDefinition(1, DataDefinition::create('string')) + ->setPropertyDefinition('key7', DataDefinition::create('string')) + ); + + $typed_data = $this->typedDataManager->create( + $map_data_definition, + $tree, + 'test name' + ); + + return $typed_data; + } + +} diff --git a/core/modules/shortcut/tests/src/Functional/Rest/ShortcutResourceTestBase.php b/core/modules/shortcut/tests/src/Functional/Rest/ShortcutResourceTestBase.php index eb7d33dad2..fa1e4fa491 100644 --- a/core/modules/shortcut/tests/src/Functional/Rest/ShortcutResourceTestBase.php +++ b/core/modules/shortcut/tests/src/Functional/Rest/ShortcutResourceTestBase.php @@ -58,6 +58,9 @@ protected function createEntity() { 'weight' => -20, 'link' => [ 'uri' => 'internal:/admin/content/comment', + 'options' => [ + 'fragment' => 'new', + ], ], ]); $shortcut->save(); @@ -96,7 +99,9 @@ protected function getExpectedNormalizedEntity() { [ 'uri' => 'internal:/admin/content/comment', 'title' => NULL, - 'options' => [], + 'options' => [ + 'fragment' => 'new', + ], ], ], 'weight' => [ diff --git a/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMapField.php b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMapField.php new file mode 100644 index 0000000000..eb72fa55fc --- /dev/null +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMapField.php @@ -0,0 +1,39 @@ +setLabel(t('Data')) + ->setDescription(t('A serialized array of additional data.')); + + return $fields; + } + +} diff --git a/core/modules/system/tests/modules/entity_test/tests/src/Functional/Hal/EntityTestMapFieldHalJsonAnonTest.php b/core/modules/system/tests/modules/entity_test/tests/src/Functional/Hal/EntityTestMapFieldHalJsonAnonTest.php new file mode 100644 index 0000000000..af1ef4ceaa --- /dev/null +++ b/core/modules/system/tests/modules/entity_test/tests/src/Functional/Hal/EntityTestMapFieldHalJsonAnonTest.php @@ -0,0 +1,103 @@ +applyHalFieldNormalization($default_normalization); + + $author = User::load(0); + return $normalization + [ + '_links' => [ + 'self' => [ + 'href' => '', + ], + 'type' => [ + 'href' => $this->baseUrl . '/rest/type/entity_test_map_field/entity_test_map_field', + ], + $this->baseUrl . '/rest/relation/entity_test_map_field/entity_test_map_field/user_id' => [ + [ + 'href' => $this->baseUrl . '/user/0?_format=hal_json', + 'lang' => 'en', + ], + ], + ], + '_embedded' => [ + $this->baseUrl . '/rest/relation/entity_test_map_field/entity_test_map_field/user_id' => [ + [ + '_links' => [ + 'self' => [ + 'href' => $this->baseUrl . '/user/0?_format=hal_json', + ], + 'type' => [ + 'href' => $this->baseUrl . '/rest/type/user/user', + ], + ], + 'uuid' => [ + [ + 'value' => $author->uuid(), + ], + ], + 'lang' => 'en', + ], + ], + ], + ]; + } + + /** + * {@inheritdoc} + */ + protected function getNormalizedPostEntity() { + return parent::getNormalizedPostEntity() + [ + '_links' => [ + 'type' => [ + 'href' => $this->baseUrl . '/rest/type/entity_test_map_field/entity_test_map_field', + ], + ], + ]; + } + + /** + * {@inheritdoc} + */ + protected function getExpectedCacheContexts() { + return [ + 'url.site', + 'user.permissions', + ]; + } + +} diff --git a/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestMapFieldJsonAnonTest.php b/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestMapFieldJsonAnonTest.php new file mode 100644 index 0000000000..0440680325 --- /dev/null +++ b/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestMapFieldJsonAnonTest.php @@ -0,0 +1,24 @@ + 'value', + 'key2' => 'no, val you', + 'π' => 3.14159, + TRUE => 42, + 'nested' => [ + 'bird' => 'robin', + 'doll' => 'Russian', + ], + ]; + + /** + * {@inheritdoc} + */ + protected function setUpAuthorization($method) { + $this->grantPermissionsToTestedRole(['administer entity_test content']); + } + + /** + * {@inheritdoc} + */ + protected function createEntity() { + $entity = EntityTestMapField::create([ + 'name' => 'Llama', + 'type' => 'entity_test_map_field', + 'data' => [ + static::$mapValue, + ], + ]); + $entity->setOwnerId(0); + $entity->save(); + return $entity; + } + + /** + * {@inheritdoc} + */ + protected function getExpectedNormalizedEntity() { + $author = User::load(0); + return [ + 'uuid' => [ + [ + 'value' => $this->entity->uuid(), + ], + ], + 'id' => [ + [ + 'value' => 1, + ], + ], + 'name' => [ + [ + 'value' => 'Llama', + ], + ], + 'langcode' => [ + [ + 'value' => 'en', + ], + ], + 'created' => [ + $this->formatExpectedTimestampItemValues((int) $this->entity->get('created')->value), + ], + 'user_id' => [ + [ + 'target_id' => (int) $author->id(), + 'target_type' => 'user', + 'target_uuid' => $author->uuid(), + 'url' => $author->toUrl()->toString(), + ], + ], + 'data' => [ + static::$mapValue, + ], + ]; + } + + /** + * {@inheritdoc} + */ + protected function getNormalizedPostEntity() { + return [ + 'name' => [ + [ + 'value' => 'Dramallama', + ], + ], + 'data' => [ + 0 => static::$mapValue, + ], + ]; + } + + /** + * {@inheritdoc} + */ + protected function getExpectedUnauthorizedAccessMessage($method) { + if ($this->config('rest.settings')->get('bc_entity_resource_permissions')) { + return parent::getExpectedUnauthorizedAccessMessage($method); + } + + return "The 'administer entity_test content' permission is required."; + } + + /** + * {@inheritdoc} + */ + protected function getExpectedCacheContexts() { + return ['user.permissions']; + } + +} -- 2.24.1