Cacheability of normalized computed fields' properties is not captured during serialization

Thanks @jibran!

This will need to land against the 2.x branch. Let's see if it applies.

Thanks for the patch @jibran!

I agree that this will work, but it feels that we are abusing the $access variable to pass down cacheability information. Maybe we can add the RefinableCacheableDependencyTrait in the FieldNormalizerValue so we can generate the value object and then add the cacheable dependency from the field item.

An alternative would be to pass additional cacheability information via the FieldNormalizerValue constructor.

+++ b/src/Normalizer/RelationshipNormalizer.php
@@ -200,14 +201,9 @@ class RelationshipNormalizer extends NormalizerBase implements DenormalizerInter
-    // If this is called, access to the Relationship field is allowed. The
-    // cacheability of the access result is carried by the Relationship value
-    // object. Therefore, we can safely construct an access result object here.
-    // Access to the targeted related resources will be checked separately.
-    // @see \Drupal\jsonapi\Normalizer\EntityReferenceFieldNormalizer::normalize()
-    // @see \Drupal\jsonapi\Normalizer\RelationshipItemNormalizer::normalize()
-    $relationship_access = AccessResult::allowed()->addCacheableDependency($relationship);
-    return new RelationshipNormalizerValue($relationship_access, $normalizer_items, $cardinality, $link_context);

Nice! It seems we were already abusing the $access object in other parts of the codebase.

This looks great! Let's hear from the test runner to see a green.

To try this patch, where I need implements CacheableDependencyInterface? In my field type?

I'm sorry for the noob question :_ D

+++ b/src/Normalizer/Value/FieldNormalizerValue.php
@@ -54,10 +55,16 @@ class FieldNormalizerValue implements FieldNormalizerValueInterface {
+   * @param \Drupal\Core\Cache\CacheableDependencyInterface $field_cacheability
...
-  public function __construct(AccessResultInterface $field_access_result, array $values, $cardinality, $property_type) {
+  public function __construct(AccessResultInterface $field_access_result, array $values, $cardinality, $property_type, CacheableDependencyInterface $field_cacheability) {

Instead of adding an argument, let's just rename $field_access_result to $cacheability. IIRC, that argument was only added to capture cacheability in the first place. It has no other use in the constructor.

@jibran, this looks really good! Thank you SO much for all taking this on :)

The only thing this needs now is tests. I think adding one to JsonApiRegressionTest would be the easiest place to do that.

Looking good!

1. +++ b/src/Normalizer/FieldNormalizer.php
   @@ -50,7 +52,12 @@ class FieldNormalizer extends NormalizerBase implements DenormalizerInterface {
   +      $cacheability = new CacheableMetadata();
   +      $cacheability->addCacheableDependency($access);
   

   Slightly cleaner:

   $cacheability = CacheableMetadata::createFromObject($access);
   

2. +++ b/src/Normalizer/RelationshipNormalizer.php
   @@ -200,14 +200,9 @@ class RelationshipNormalizer extends NormalizerBase implements DenormalizerInter
   -    // If this is called, access to the Relationship field is allowed. The
   -    // cacheability of the access result is carried by the Relationship value
   -    // object. Therefore, we can safely construct an access result object here.
   -    // Access to the targeted related resources will be checked separately.
   -    // @see \Drupal\jsonapi\Normalizer\EntityReferenceFieldNormalizer::normalize()
   -    // @see \Drupal\jsonapi\Normalizer\RelationshipItemNormalizer::normalize()
   

   Why are we removing this detailed comment?

3. +++ b/src/Normalizer/RelationshipNormalizer.php
   @@ -200,14 +200,9 @@ class RelationshipNormalizer extends NormalizerBase implements DenormalizerInter
   +    $cacheability = new CacheableMetadata();
   +    $cacheability->addCacheableDependency($relationship);
   

   Same here.

NW for tests.

#16.2: Oh, I get it now! Makes sense :) Also: YAY for less complexity!

The tests failed because HEAD was broken.

+++ b/src/Normalizer/Value/FieldNormalizerValue.php
@@ -46,8 +46,12 @@ class FieldNormalizerValue implements FieldNormalizerValueInterface {
+   *   cacheability and then passes it to this value object.

s/passes/pass

But that can be fixed on commit or when tests are added.

Cool :) Back to NW for tests!

Crediting @ylynfatt. Thanks both of you!

Attempted to fix some of the Codesniffer issues.

This fixes that last CS violation. Hopefully this will now actually show the test failure, not "Build Successful".

Looking into the test failure now.

Yay, this is the first time that #3000299: Let phpcs violations fail the build on DrupalCI is helping us prevent from committing a patch that has CS violations!

@axle_foley00 asked me to help debug this.

Specifically, why we're not getting a cache lifetime of 8000, instead we're getting UNCACHEABLE.

I tried to dig into this, but I did not reach a conclusion. I did a little clean up to make the test failures more informative too.

Perhaps this is already correct, but I'm just not sure.

@Wim Leers would you take a look?

+++ b/tests/src/Functional/JsonApiRegressionTest.php
@@ -506,33 +508,25 @@ class JsonApiRegressionTest extends JsonApiFunctionalTestBase {
+    $this->assertSame('_MISS', $response->getHeader('X-Drupal-Cache')[0]);
+    $this->assertSame('_UNCACHEABLE', $response->getHeader('X-Drupal-Dynamic-Cache')[0]);

I prefixed these with _ to intentionally fail the tests because I'm not sure that these are the correct values.

Specifically, why we're not getting a cache lifetime of 8000, instead we're getting UNCACHEABLE.

Because #2352009: Bubbling of elements' max-age to the page's headers and the page cache. i.e. a long-existing limitation in Drupal core. Added a @todo for this.

Perhaps this is already correct, but I'm just not sure.

It is :)

1. +++ b/tests/modules/jsonapi_test_computed_field/src/JsonApiComputedFieldItemList.php
   @@ -0,0 +1,60 @@
   +  protected $cacheabilityMetadata;
   ...
   +    $this->cacheabilityMetadata = (new CacheableMetadata())
   +      ->setCacheContexts(['user'])
   +      ->setCacheTags(['field:jsonapi_test_computed_field'])
   +      ->setCacheMaxAge(8000);
   

   This would be simpler if it used CacheableDependencyTrait :)

2. +++ b/tests/src/Functional/JsonApiRegressionTest.php
   @@ -498,4 +501,32 @@ class JsonApiRegressionTest extends JsonApiFunctionalTestBase {
   +   * Regression test for 2997123.
   

   Needs better comment.

3. +++ b/tests/src/Functional/JsonApiRegressionTest.php
   @@ -498,4 +501,32 @@ class JsonApiRegressionTest extends JsonApiFunctionalTestBase {
   +  public function testAllowComputedFieldsToSetCacheablityMetaData() {
   +    // Set up data model.
   +    $this->assertTrue($this->container->get('module_installer')->install(['jsonapi_test_computed_field'], TRUE), 'Installed modules.');
   +
   +    // Ensure the anonymous user can view the entity.
   +    Role::load(RoleInterface::ANONYMOUS_ID)->grantPermission('view test entity')->save();
   +
   +    $entity_test = EntityTest::create([
   +      'name' => 'Test Entity for Computed Field',
   +      'type' => 'entity_test',
   +    ]);
   +
   +    $entity_test->save();
   +
   

   Nit: This can be made more consistent with other regression tests.

Fixed all these nits myself. Patch looks great otherwise, so: RTBC! 🎉

I was just about to commit this, when I realized something.

Why is this only a problem for computed fields? Why didn't this come up before? Because for example text fields also bubble cacheability: they bubble at least the used text format's cache tag. See \Drupal\Tests\jsonapi\Functional\CommentTest::getExpectedCacheTags() for example.

Well, all those fields have cacheability one level lower: at the property level. interface FieldItemListInterface extends ListInterface, AccessibleInterface is nothing more than a list of things, it's a container. The actual data sits one level lower. And so it's that lower level's cacheability that matters.

That level's cacheability is already respected! Look at \Drupal\jsonapi\Normalizer\Value\FieldNormalizerValue::__construct():

  public function __construct(AccessResultInterface $field_access_result, array $values, $cardinality, $property_type) {
    assert($property_type === 'attributes' || $property_type === 'relationships');
    $this->setCacheability(static::mergeCacheableDependencies(array_merge([$field_access_result], $values)));
…

IOW: it's $values (i.e. field properties) whose cacheability is bubbled. This is how it works for all of Drupal core. Sorry for not realizing this sooner.

I realize that @jibran wrote:

Just as a sanity check I pinged @amateescu on slack and he agreed that any *ItemList class can implement CacheableDependencyInterface if needed. So here is the patch.

But actually, nothing in Drupal core does this. Both core REST and JSON API (and probably much other code too) assumes that cacheability for fields lives at the field property level!

So: what does your custom computed field look like? Can't the cacheability be moved from the field level to the field property level?

I agree that cacheable metadata at the FieldItem or FieldItemList level is a smell and doing this would snow-flake jsonapi.

IMO no other system expects metadata to be available at this level and I think the reason for that is: cacheability is defined by presentation and those things are natively only concerned with storage. For example, the formatter system couldn't just use FieldItem cacheability, because the tags are based on the markup rendered by each formatter (ie, an image style formatter will have different config entity tags based on the users configuration).

This seems to be a similar ethos driving #2926507: Discourage @FieldType-level normalizers, encourage @DataType-level normalizers, to strengthen the API-First ecosystem.

REST based normalizers are able to respect and use metadata at the DataType level because those are the primitives that reliably appear embedded in actual resources, the lists and items don't necessarily have a bearing on the presentation of the resource.

@Sam152: Thanks for chiming in, and elaborating on it more! ❤️ 👌

What we are saying here is if a 'string' field has cacheablity data to bubble then we should create a new DataType which should bubble the cacheablity.

Correct. Because that is the level that actually contains data. Also, there is the case of exposing/processing/interacting/DOING SOMETHING with individual field properties; if cacheability is not carried by EACH property, then it's impossible to do something with each property while respecting cacheability.
IOW: cacheability ought to live at the level where the data lives, otherwise certain use cases are made impossible. Therefore it needs to live at the @DataType level.

Example: \Drupal\text\TextProcessed.

or fix this issue as a stop-gap solution

Fixing this issue will not fix it in core REST nor in GraphQL.

Again: I'm sorry for not noticing this sooner, and I'm sorry for the annoying inconvenience of asking you to change your code :( But alas Entity/Field/Typed Data API are not perfect. This is is one of those cases where imperfections and lack of clarity leads to wasted time for all involved! 😞

I had a requirement for exactly this functionality recently which might serve as an example use case for field-level cacheability.

The requirement was to have a computed (pseudo-)entity reference field on an entity that referenced different content according to data stored against the current user, e.g. a group or region that the user belongs to.

We had already created a cache context that used the concatenated id's of the user's region and group that would ensure correct content for the user with a much higher hit-rate then per-user cacheing.

The challenge was add the cache context to the request if and only if the given field was part of the response. In the end we used an event subscriber which listened for the `kernal.response` event and used some slightly hacky techniques to work out whether the field should be in the response and then add the cache context.

This would have been a lot easier and more robust if we could have simply added the cache metadata at the level of the computed field, and being able to add it at the data type level would not have helped as it is not the referenced entity itself that varies from user to user but the combination of referenced entities which belongs one-level up.

I'd imagine there would probably be a lot of other example of cases where the thing that needs cache metadata is the list of values itself rather than the items that are in the list (if that makes sense).

Can we please change this test to demonstrate how to add string field which takes cacheablity into account.

+1!

Wanted to work on this right away, but then noticed #37 is still waiting for the branch to pass, because JSON API 2.x HEAD is failing because Drupal core 8.7 changed something. Investigating and fixing at #3001193: CommentTest::testPostIndividualDxWithoutCriticalBaseFields() fails on 8.7 since #2885809.

Fix CS violations.

Can we please change this test to demonstrate how to add string field which takes cacheablity into account.

+1

Actually, on second thought, -1.

That would be adding a test + test class for something that is entirely independent from JSON API; something that belongs in Drupal core. That being said, I'm fine with being pragmatic here :)

+++ b/tests/modules/jsonapi_test_computed_field/src/JsonApiComputedFieldItemList.php
@@ -0,0 +1,33 @@
+    $value->setCacheability($cacheability);

This is calling a protected method. That will not work.

EDIT: cross-post :(

#55: I had many of the same changes. Next time please assign the issue to yourself to prevent duplicate effort! 🙏 🙂

What's still missing, is a functional test. And once we have that, IMHO that kernel test can go away. As you just experienced yourself, kernel tests are … tedious and brittle. And in this case, it seems to prove things are working correctly … but what matters is the end result, and for that you need a functional test, and that's proving that it does not work (yet).

The reason this particular computed property doesn't work yet is because it implements PrimitiveInterface, which causes \Drupal\serialization\Normalizer\PrimitiveDataNormalizer to be used. Because CacheableStringData extends StringData extends TypedData implements PrimitiveInterface. Contrast this with TextProcessed extends TypedData. Is this confusing and counter-intuitive? Yes. Much in TypedData is confusing… How did I find out? By writing a functional test that failed and then stepping through it with a debugger.

To get the functional test to pass, I also had to switch from the user cache context to something that will still allow Dynamic Page Cache to cache the result.

Attached is the interdiff + patch that add a functional test. The functional test should fail.

There were 2 failures:

1) Drupal\Tests\jsonapi\Functional\EntityTestTest::testGetIndividual
Failed asserting that Array &0 (
    0 => 'entity_test:1'
    1 => 'http_response'
) is identical to Array &0 (
    0 => 'entity_test:1'
    1 => 'field:jsonapi_test_computed_field'
    2 => 'http_response'
).

/var/www/html/modules/contrib/jsonapi/tests/src/Functional/ResourceTestBase.php:687
/var/www/html/modules/contrib/jsonapi/tests/src/Functional/ResourceTestBase.php:966

2) Drupal\Tests\jsonapi\Functional\EntityTestTest::testCollection
Failed asserting that Array &0 (
    0 => 'entity_test:1'
    1 => 'entity_test:2'
    2 => 'entity_test_list'
    3 => 'http_response'
) is identical to Array &0 (
    0 => 'entity_test:1'
    1 => 'entity_test:2'
    2 => 'entity_test_list'
    3 => 'field:jsonapi_test_computed_field'
    4 => 'http_response'
).

Failed as expected and described in #60: the computed field's property's cache tags and contexts did not bubble to the response!

The reason this particular computed property doesn't work yet is because it implements […]

This implements that fix.

Time to remove all the out-of-scope changes, now that this issue has been descoped to be about tests only. (See #46 + #48.)

@PQ: I didn't forget to reply to you! I just wanted to finish this other thing first. I see what you're saying. But:

This would have been a lot easier and more robust if we could have simply added the cache metadata at the level of the computed field, and being able to add it at the data type level would not have helped as it is not the referenced entity itself that varies from user to user but the combination of referenced entities which belongs one-level up.

In the ideal case, you're right. But this will require changes in many places of the Field API and normalizers for fields. There are more important problems to deal with.

You're right that conceptually the cacheability then is associated with the list of things. But really, you can associate that cache context to describe that variation on every item in the list too, that still captures the same thing: "this item varies by context X". It achieves the same end result, with equal accuracy, but with a slightly inferior/less perfect data model.

So I'd ask you to be pragmatic for now and associate the cacheability with each item in the list :)

You're writing comments faster than I can; you're asking the exact questions/rolling the exact patches I was about to, time and time again! 😄 👌 🤜 🤛

#62 and #63 are green! We proved it works!

CacheableStringData extends StringData IMO, this makes sense.

On the one hand I agree. On the other hand I don't. Because a string with cacheability data is arguably no longer a primitive.

Is there are a reason why are we not fixing this upstream in PrimitiveDataNormalizer? We do need PrimitiveInterface method for CacheableStringData object so that field api can work properly.

There is some debatability here (see above), but … I agree! And that's where I quote yourself back at you :D From #58:

After working on the patch and looking at the latest fail I agree with you, this not a JSON API bug. As I mentioned in #57, to make the test green we need to fix the core. Should we move it to core and demonstrate that this bug exists in serializer?

I agree!

But before doing so, I wanted to have a green patch on this issue to ensure your concerns were addressed.

Moving this issue to Drupal core. Next steps:

1. Decide whether PrimitiveNormalizer should also respect cacheability. I think yes. A simple addition of $this->addCacheableDependency($context, $object); should do the trick.
2. Remove all mentions of "JSON API" from the test field type.
3. Rename the test field type's class to ComputedCacheableStringItem(List).
4. Update \Drupal\Tests\entity_test\Functional\Rest\EntityTestResourceTestBase like JSON API's EntityTestTest was updated.

Wanna take that on? :) I can promise fast reviews!

And this of course also serves as a case of documentation, as an example implementation. So tagging for that.

🎉

Coming to this issue from the perspective of cacheability of computed fields in general, i.e. not necessarily in the Rest/API-First context. I.e. #2924384: Add support for cacheable fields, field item lists..

I didn't read through the entire issue, but I noticed that while the issue summary mentions supporting cacheability metadata on the field level, the patch implements it on the typed-data / property level.

I then found #38, which as far as I can tell is the reason for the different approach, but I'd like to argue against it. In particular

interface FieldItemListInterface extends ListInterface, AccessibleInterface is nothing more than a list of things, it's a container. The actual data sits one level lower. And so it's that lower level's cacheability that matters.

While for non-computed fields that is true and, thus, it's good and important that we support cacheability metadata on the property level. This does not hold for computed fields. In particular since the introduction of ComputedItemListTrait the logic on how a field value for a computed field is computed lives on the field item list level. And, thus, the field item list solely knows the cacheability metadata for this computation, i.e. under which circumstances the result of the computation may change. So I think it would in fact make sense to support cacheability metadata on the field item list level. Otherwise, computed fields have to implement ::computeValue() on the field item list level and then additionally provide overridden typed data classes for any of their properties to declare the cacheability metadata for the result of the computation - possibly for many different properties.

I may be missing something, of course, as that is generally the case, but also in particular because I'm coming from a completely different view point, but I'd love to hear your thoughts. Would be also great to get some other Entity / Field API maintainer's thoughts.

Ahh missed that @jibran. Thanks! So looking forward to what @Wim Leers says to this.

Thanks, I didn't know about #2924384: Add support for cacheable fields, field item lists..

I already explained my perspective in #38, #43 and #64. Also see @Sam152's comment at #40.

Hi @Wim Leers,

Thanks for getting back on this. I totally agree that my example might be an edge case and not the best use of effort at this time.

So I'd ask you to be pragmatic for now and associate the cacheability with each item in the list :)

Will do, thanks.

Re #73: Hmm... I disagree with #40, at least when talking about computed fields for the same reason explained #70. #64 sounds like you tend to agree in principle but don't want to solve that whole issue in the scope of this issue. That's fine with me in general, I just wanted to make sure that we're not setting some kind of precedent here, but seeing that we're pretty much all broadly in agreement (except for #40), I guess that's not the case. So don't mind me...

Re: #70, great points, if decisions are made which impacts things like if an item exists or not or potentially computes to one or multiple deltas, list based cacheability would seem to be an API deficiency that could be addressed, I hadn't considered this at all. The muddy part (and probably the reason for the current state of the api) is lists are varied in their presentation whereas individual properties are pretty much serialized as-is.

For a computed list, with a single item and computed property, cacheability at the list level doesn't really matter, the list itself never really changes. In the examples so far the same field item is always created, if no decisions about the items instantiation are made, a single property being dynamic is better described with cacheability at the data type level imo.

So I totally agree in the former use case list based cacheability would be a hard requirement, but it doesn't seem to me like the tests really illustrate that point. Perhaps it needs something like the "dice roll" example but for the number of items in the list?

Rebased on jsonapi ad50c207 (Last stable: 8.x-2.0)

1. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestComputedField.php
   @@ -23,7 +25,7 @@
   - *     "add-form" = "/entity_test_computed_field/add",
   + *     "canonical" = "/entity_test_computed_field/{entity_test_computed_field}",
   

   🤔 This can theoretically break some core/contrib tests. Any way we could avoid that?

2. +++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/ComputedTestCacheableStringItemList.php
   @@ -0,0 +1,30 @@
   +      ->setCacheContexts(['url.query_args:computed_test_cacheable_string_field'])
   +      ->setCacheTags(['field:computed_test_cacheable_string_field'])
   
   +++ b/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestComputedFieldNormalizerTest.php
   @@ -0,0 +1,104 @@
   +    return Cache::mergeContexts(parent::getExpectedCacheContexts(), ['url.query_args:computed_test_cacheable_string_field']);
   ...
   +    return Cache::mergeTags(parent::getExpectedCacheTags(), ['field:computed_test_cacheable_string_field']);
   

   This is the proof, and this is what should fail in HEAD.

   Could you also upload a failing test-only patch?

3. +++ b/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestResourceTestBase.php
   @@ -59,12 +58,14 @@ protected function setUpAuthorization($method) {
   -    $this->container->get('state')->set('entity_test.internal_field', TRUE);
   +    \Drupal::state()->set('entity_test.internal_field', TRUE);
   

   🤔 This change looks wrong?

👏 Looks great @jibran, only one typo that I think can be fixed on commit.

I think this is a bug TBH. Updated, the issue metadata.

+++ b/core/modules/system/tests/modules/entity_test/src/Plugin/Field/FieldType/ComputedTestCacheableStringItem.php
@@ -0,0 +1,37 @@
+ *   label = @Translation("Test Text (plain with cacheablity)"),
+ *   description = @Translation("A test field containing a plain string value and cacheablity metadata."),

Nit & typo: "plain string" and s/cacheablty/cacheability on both lines.

Can be fixed on commit.

Rerolled and fixed the failure by adding $defaultTheme.

+++ b/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestComputedFieldNormalizerTest.php
index b1b376eaa7..7eeddd2cc4 100644
--- a/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestResourceTestBase.php

--- a/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestResourceTestBase.php
+++ b/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestResourceTestBase.php

It's interesting we're not changing the JSONAPI equivalent test. base class.

+++ b/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestComputedFieldNormalizerTest.php
@@ -0,0 +1,100 @@
+/**
+ * Test normalization of computed field.
+ *
+ * @group rest
+ */
+class EntityTestComputedFieldNormalizerTest extends EntityTestResourceTestBase {

Do we need to add an equivalent test for JSONAPI in core?

Setting to needs review to get an answer - if the answer is no that's not needed then we can set this back to rtbc.

Thanks go to jibran for this patch.

Thanks for the review. After studying it I see that I missed, which I sort of knew anyway but family duties were calling me. Mostly, it confirms that when I am learning something new I really need to take a walk before trying to apply the knowledge.

Going to review this today! We need this in the Commerce API as we have computed fields using resolved pricing and a computed field which embeds entities directly, so we need to bubble their cacheability.

+++ b/core/modules/serialization/src/Normalizer/PrimitiveDataNormalizer.php
@@ -21,6 +21,9 @@ class PrimitiveDataNormalizer extends NormalizerBase {
   public function normalize($object, $format = NULL, array $context = []) {
+    // Add cacheability if applicable.
+    $this->addCacheableDependency($context, $object);

+++ b/core/modules/serialization/tests/src/Kernel/EntitySerializationTest.php
@@ -365,4 +369,19 @@ public function testDenormalizeStringValue() {
+  /**
+   * Tests normalizing cacheable computed field.
+   */
+  public function testCacheableComputedField() {

I have some serious concerns if this method actually works, at least with JSON:API.

In the test we do

$context[CacheableNormalizerInterface::SERIALIZATION_CONTEXT_CACHEABILITY] = new CacheableMetadata();

So I wanted to see where this is set in the normalization and serialization stack.

The REST module adds it to the context when rendering the response body (+1, means it is used throughout)

\Drupal\rest\EventSubscriber\ResourceResponseSubscriber::renderResponseBody

For JSON:API it is only set during the FieldItemNormalizer, not the entire normalization process

\Drupal\jsonapi\Normalizer\FieldItemNormalizer::normalize

It actually creates a new CacheableMetadata for each field item and then unsets it from the context (but manually collects into CacheableNormalization)

So maybe this is OK.

I know HAL is basically unused, but it only does this in \Drupal\hal\LinkManager\LinkManagerBase::getLinkDomain.

Also, I don't see any tests with JSON:API, which is the main use case I would use this in, to make it sure works with the CacheableNormalization paradigm. @alexpott raised this in #102.

Per #103, I still think we need explicit testing since JSON:API uses an extended serialization system.

I'm not comfortable with this patch without explicit JSON:API testing due to its usage of CacheableNormalization objects.

rerolls

Fixed Custom Commands Failed.

Bit confused on the status here. @jibran in #109 puts in on "needs work" since there is missing tests. Then @larowan in #111 puts it in needs review with only a reroll. Not sure what is correct right now.

Ok, talked about this with larowlan. Should be needs work since tests are still missing.

Perhaps I'm wrong here but we have problems also with cacheability of computed fields that contain an array of values, not a simple string.

If I look at core/modules/jsonapi/src/Normalizer/FielditemNormlizer.php, this hints as cacheability but does not actively allow so:

public function normalize($field_item, $format = NULL, array $context = []) {
    /** @var \Drupal\Core\TypedData\TypedDataInterface $property */
    $values = [];
    $context[CacheableNormalizerInterface::SERIALIZATION_CONTEXT_CACHEABILITY] = new CacheableMetadata();
    if (!empty($field_item->getProperties(TRUE))) {
      // We normalize each individual value, so each can do their own casting,
      // if needed.
      $field_properties = TypedDataInternalPropertiesHelper::getNonInternalProperties($field_item);
      foreach ($field_properties as $property_name => $property) {
        $values[$property_name] = $this->serializer->normalize($property, $format, $context);
      }
      // Flatten if there is only a single property to normalize.
      $values = static::rasterizeValueRecursive(count($field_properties) == 1 ? reset($values) : $values);
    }
    else {
      $values = $field_item->getValue();
    }
    $normalization = new CacheableNormalization(
      $context[CacheableNormalizerInterface::SERIALIZATION_CONTEXT_CACHEABILITY],
      $values
    );
    unset($context[CacheableNormalizerInterface::SERIALIZATION_CONTEXT_CACHEABILITY]);
    return $normalization;
}

To make matters worse, jsonapi explicitly does not allow custom jsonapi_normalizers:

core/modules/jsonapi/src/Serializer/Serializer.php

public function __construct(array $normalizers = [], array $encoders = []) {
    foreach ($normalizers as $normalizer) {
      if (strpos(get_class($normalizer), 'Drupal\jsonapi\Normalizer') !== 0) {
        throw new \LogicException('JSON:API does not allow adding more normalizers!');
      }
    }
    parent::__construct($normalizers, $encoders);
}

For now we solved this with a simple core patch that removes the serrializer construct and a extended class that adds appropriate cache tags to specific computed fields at field_item level using its parent field_item_list name.

I've created the test for the EntityTestComputedField in the same manner as the EntityTestMapField. I've verified some tests fail when the expected cache tags are not provided, also when the field is removed as sparse fieldset it also fails as expected.

We could optimize this test by not testing everything which would save a bit of time, but not sure if we should really do that.

I've chosen not to fill the computed_reference_field<code> since that was a lot of hassle since it also needs the <code>EntityTest entity to be fully set up.

Accidentally kept a method that wasn't needed anymore (thank you cspell).

Fixed some naming issues.

JSON:API needs an UUID to be able to use entities. The computed doesn't have that so i added that. This breaks EntityTestComputedFieldNormalizerTest so i added uuid to the expected output.

Not sure why the taxonoy test was failing.

#friday-afternoon

I've gone through the changes since my last review in #108. I'm happy with the JSON:API test coverage! I think it is safe to remove the Needs Tests tag. It's also helped add test coverage of computed fields in JSON:API regardless of the cacheability needs.

Thanks @mglaman :)

Committed 5ff249f and pushed to 9.3.x. Thanks!

Will backport to 9.2.x is the test run is successful there.

Thanks Alex. The failing test is probably the fact we added drupal_internal__target_id to the meta tag in 9.3.x in jsonapi.

@alexpott: backported the patch to 9.2.x. As i already thought, the addition of the drupal_internal__target_id in the meta information of jsonapi was the culprit.

The fail was a result of #3036593: Add 'drupal_internal__target_id' to JSON:API representation of entity reference fields, because that can't be retrieved from the target resource for target entity types without corresponding resources

Hi, please help. Could someone take a look at this ticket: https://www.drupal.org/project/drupal/issues/3252278 ? Thanks!

I never got round to the backport. Thanks to @bbrala for reminding me. 9.2.x is security only now so the backport will never occur.