FieldItemNormalizer to not flatten if one property and getMainPropertyName is NULL

Thanks for filing this!

Only D9-only bug fixes and major-only allowed changes during the beta phase should be filed against 9.0.x. New features, API additions, and deprecations should be filed against 9.1.x.

So, moving to the 9.1.x-dev branch.

@mglaman What's a good way to manually test this patch?

This is a bug, IMO. Let's have a test only patch as well.

1. +++ b/core/modules/jsonapi/src/Normalizer/FieldItemNormalizer.php
   @@ -71,7 +72,8 @@ public function normalize($field_item, $format = NULL, array $context = []) {
   +      $flatten = count($field_properties) === 1 && $field_item::mainPropertyName() !== NULL;
   

   Given that we only flatten a single property field what about the fields with multiple properties but still has mainProperty. Do we want to test that here as well or would that be out of scope here?

2. +++ b/core/modules/jsonapi/tests/modules/jsonapi_test_field_type/src/Plugin/Field/FieldType/NoMainPropertyNameTestFieldItem.php
   @@ -0,0 +1,27 @@
   +class NoMainPropertyNameTestFieldItem extends StringItem {
   
   +++ b/core/modules/jsonapi/tests/modules/jsonapi_test_field_type/src/Plugin/Field/FieldType/SingleInternalPropertyTestFieldItem.php
   @@ -0,0 +1,45 @@
   +class SingleInternalPropertyTestFieldItem extends StringItem {
   

   These should be added to the entity test module so that other modules can also use them if needs be.

My feedback has beend addressed and patch looks great to me so setting it to RTBC.

+++ b/core/modules/jsonapi/tests/src/Kernel/Normalizer/FieldItemNormalizerTest.php
@@ -0,0 +1,129 @@
+    $this->container->get('state')->set('entity_test.additional_base_field_definitions', $definitions);

WoW, I didn't know that this is amazing.

is there a way to tell the test bot to not retest the test only patch every two days?

Yes, if you upload test only patch first and the actual patch as last. The order matters here.

1. +++ b/core/modules/jsonapi/tests/src/Kernel/Normalizer/FieldItemNormalizerTest.php
   @@ -0,0 +1,129 @@
   +  protected static $modules = [
   +    'system',
   +    'user',
   +    'field',
   +    'text',
   +    'link',
   +    'entity_test',
   +    'serialization',
   +    'jsonapi',
   +  ];
   

   🤓 We don't need to specify all of these.

2. +++ b/core/modules/jsonapi/tests/src/Kernel/Normalizer/FieldItemNormalizerTest.php
   @@ -0,0 +1,129 @@
   +  protected function setUp(): void {
   

   🤓 Missing @inheritdoc?

3. +++ b/core/modules/jsonapi/tests/src/Kernel/Normalizer/FieldItemNormalizerTest.php
   @@ -0,0 +1,129 @@
   +   * Tests a field item which has no properties returned.
   

   🤔 "has no properties returned"

4. +++ b/core/modules/jsonapi/tests/src/Kernel/Normalizer/FieldItemNormalizerTest.php
   @@ -0,0 +1,129 @@
   +    $this->assertEquals('Direct call to getValue', $result->getNormalization());
   

   🤓 Why not assertSame()?

5. +++ b/core/modules/jsonapi/tests/src/Kernel/Normalizer/FieldItemNormalizerTest.php
   @@ -0,0 +1,129 @@
   +  /**
   +   * Tests items with
   +   */
   

   🐛 🤓 with?

6. +++ b/core/modules/jsonapi/tests/src/Kernel/Normalizer/FieldItemNormalizerTest.php
   @@ -0,0 +1,129 @@
   +    // Verify a field with one property is flattened.
   +    $result = $this->normalizer->normalize($entity->get('name')->first());
   +    assert($result instanceof CacheableNormalization);
   +    $this->assertEquals('Test entity', $result->getNormalization());
   +
   +    // Verify a field with multiple public properties has all of them returned.
   +    $result = $this->normalizer->normalize($entity->get('links')->first());
   +    assert($result instanceof CacheableNormalization);
   +    $this->assertEquals([
   +      'uri' => 'https://www.drupal.org',
   +      'title' => 'Drupal.org',
   +      'options' => [
   +        'query' => 'foo=bar',
   +      ],
   +    ], $result->getNormalization());
   +
   +    // Verify a field with one public property and one internal only returns the
   +    // public property, and is flattened.
   +    $result = $this->normalizer->normalize($entity->get('internal_property_value')->first());
   +    assert($result instanceof CacheableNormalization);
   +    // Property `internal_value` will not exist.
   +    $this->assertEquals('Internal property testing!', $result->getNormalization());
   

   👍 These are adding explicit unit (well, kernel) tests for pre-existing logic. That's why these assertions pass even in the test-only patch.

7. +++ b/core/modules/jsonapi/tests/src/Kernel/Normalizer/FieldItemNormalizerTest.php
   @@ -0,0 +1,129 @@
   +    // Verify a field with one public property but no main property is not
   +    // flattened.
   +    $result = $this->normalizer->normalize($entity->get('no_main_property_value')->first());
   +    assert($result instanceof CacheableNormalization);
   +    $this->assertEquals([
   +      'value' => 'No main property testing!',
   +    ], $result->getNormalization());
   

   👍 This is testing the fix.

8. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/NoMainPropertyNameTestFieldItem.php
   @@ -0,0 +1,27 @@
   +/**
   + * Defines the 'No Main Property Name' entity test field type.
   + *
   + * This provides a field type that has one property but does not define a main
   + * property name. When normalized, the value should not be flattened.
   + *
   + * @FieldType(
   + *   id = "no_main_property_name_test",
   + *   label = @Translation("No Main Property Name (test)"),
   + *   category = @Translation("Test"),
   + *   default_widget = "string_textfield",
   + *   default_formatter = "string"
   + * )
   + */
   +class NoMainPropertyNameTestFieldItem extends StringItem {
   +
   +  public static function mainPropertyName() {
   +    return NULL;
   +  }
   +
   +}
   

   🤔 Why not just reuse \Drupal\Core\Field\Plugin\Field\FieldType\MapItem?

#29.1 Fixed

2. Fixed

3. Fixed (I think 😅)

4. Fixed

5. We're asserting multiple cases here, so changed the method name and description to be more generic.

6. Makes sense! Let's do that.

🚢

Random/unrelated fail in 1) Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest::testCustomBlock.

Moving back to RTBC; unrelated test failure.

Committed/pushed to 9.2.x and cherry-picked to 9.1.x.

Marking this fixed against 9.1.x - if you feel strongly this should be backported to 8.9.x please re-open, but it's not clear to me if a client might potentially be relying on the existing broken behaviour.