Allow specifying metadata on JSON:API objects

Working on this ticket will update soon.

#11 patch not apply with d9 updated code that's why not uploading interdiff.

Changes look good, i would probably not use Drupal\jsonapi\Events\JsonapiEvents but Drupal\jsonapi\Normalizer\MetaDataEvents. But it is fine either way.

We do need tests though.

+++ b/core/modules/jsonapi/src/Events/JsonapiEvents.php
@@ -0,0 +1,11 @@
+final class JsonapiEvents {
+
+  const COLLECT_RESOURCE_OBJECT_META = 'jsonapi.collect_resource_object_meta';
+
+  const COLLECT_RELATIONSHIP_META = 'jsonapi.collect_relationship_meta';
+
+}

Symfony is moving to remove the event constants in favor of full namespaced event class name. Event constants are creating also other side effects. See #2825358: Event class constants for event names can cause fatal errors when a module is installed (especially, check @catch comment #25). Let's remove them and use the class names instead

Rerolled and fixed a fatal. This does not take into account #19 as it seems the matter is not settled yet over there.

Fixed CS error.

Well done! We do still need tests though :)

Ok, so I've written some simple tests for this issue and fixed an error in CollectResourceObjectMetaEvent where the context wasn't actually set in the object.

Now we need a new review though :)

1. +++ b/core/modules/jsonapi/src/Events/JsonapiEvents.php
   @@ -0,0 +1,11 @@
   +  const COLLECT_RESOURCE_OBJECT_META = 'jsonapi.collect_resource_object_meta';
   +
   +  const COLLECT_RELATIONSHIP_META = 'jsonapi.collect_relationship_meta';
   

   These need documentation.

2. +++ b/core/modules/jsonapi/tests/modules/jsonapi_test_meta_events/src/EventSubscriber/MetaEventSubscriber.php
   @@ -0,0 +1,30 @@
   +    $event->setMeta('resource_meta', 'testing value');
   

   Could/should the test also cover adding meta data to only a specific resource?

More generally, this looks like an API. Given the rest of jsonapi module shouts VERY LOUDLY that NONE of it is an API, this new API should document that it IS an API -- on both the events constants class and the event classes, I'd say.

Thanks for having a look! I'll have a look at how we communicate that in the event to see how things are handled there. And we do need documentation yeah. Will get back to this soon.

Retargeted to 9.5.x (or is that too soon? :x)

I've added the proper docblocks. Also i changed the JsonApiEvents.php file to MetaDataEvents.php since its only metadata events in there. I'll make a related issue in documentation so that this even will be document (as seen here).

The fact its a public api is the fact is isn't @internal and will be documented in the documentation. As with the ResourceTypeBuildEvent. Also I'll write a CR right after posting the patch files.

I will work on #24.2 also (test a single resource), which is a good test.

CR was added, expanding the tests i haven't finished yet.

Back 9.4.x, close to finishing tests. Just need some cachability tests.

After writing some proper test I did end up finding the cache tags do not bubble for the metadata in the relationship. So that still needs some work.

And I'm reminded how much I dislike working with patch files :)

Patch file was borked. Reuploading. Also forgot my helper testgroup. Also added @joachim credit

I've been poking around why caching doesn't bubble up, but cant find the reason right now. I've also added a test for the single resource endpoint on the relationship metadata event. I'm gonna walk away for now.

I found the fault and fixed it yay! The normalization return with the cacheable dependency wasn't saved to the same variable.

The CR I posted earlier I apparently didn't save, added a new one.

I'm not sure how this is passing CI, as there are a number of docs issues which I thought our CI picked up:

1. +++ b/core/modules/jsonapi/src/Events/CollectRelationshipMetaEvent.php
   @@ -0,0 +1,98 @@
   + * An event used for collecting resource object metadata of a JSON:API resource type relation.
   

   This line is too long.

2. +++ b/core/modules/jsonapi/src/Events/CollectResourceObjectMetaEvent.php
   @@ -0,0 +1,97 @@
   + * An event used for collecting resource object metadata of a JSON:API resource types.
   

   Same here -- and several more.

3. +++ b/core/modules/jsonapi/src/Events/CollectResourceObjectMetaEvent.php
   @@ -0,0 +1,97 @@
   +   *   The meta
   

   Unfinished description?

This in the README won't be true any more, so needs changing to say that MOST of the module is internal. Are all classes definitely marked @internal -- if there are some that aren't, removing this blanket warning will make them look non-internal!

> * The JSON:API module provides *no PHP API to modify its behavior.* It is
> * designed to have zero configuration.

I've checked for @internal flag with grep. The only classes (outside test classes) not @internal:

./jsonapi.links.task.yml
./src/ResourceType/ResourceTypeBuildEvents.php
./src/ResourceType/ResourceTypeBuildEvent.php
./src/Routing/ReadOnlyModeMethodFilter.php
./src/Events/CollectRelationshipMetaEvent.php
./src/Events/CollectResourceObjectMetaEvent.php
./src/Events/MetaDataEvents.php
./schema.json
./jsonapi.services.yml
./jsonapi.info.yml
./jsonapi.routing.yml
./jsonapi.api.php
./jsonapi.links.menu.yml
./config/schema/jsonapi.schema.yml
./config/install/jsonapi.settings.yml
./jsonapi.module
./jsonapi.install

That looks good i think.

Regarding codestyle, I always run the codestyle check that is provided in core, which apperantly does not check for line length. I've fixed those.

The unfinished description is technically what it returns. But i've updated it.

Regarding the internal api comment, you are entirely right. But I'd like to do that in a follow up. There are already some public API surfaces, so we are not introducing a new issue here. I will make the follow up issue.

Aww testbot... Will update when i see green tests

I think a adressed all feedback. Setting back to nr.

Documentation issue with written documentation: #3281597: Update "Customizing Resources". Thought i'd wait for this to land to put it in the docs. :)

Very related to #3186628: (use case #2) Decide on an API response format for menu data

Adding a ref to #3227824: Move the linkset functionality from the decoupled menus contributed module to core's system module just to keep track, not a prerequisite.

Yes and no. In a way this might be helpfull, but I would you would want something in the form of a hyperlink in the data, not extra data in the metadata.

Also; looking for a review ^^

While reviewing #3257608: Allow opt-out of automatic meta.drupal_internal__target_id on entity relationships i went though this issue also and found a flaw. I actually found that this MR appearantly doesn't handle the /jsonapi/node/article/id/relationships/field_tags kind of endpoint, so it only does so for relationships on the resource itself. Setting back to needs work.

Note to self;

EntityResource::getRelationShi does fire the event, but doesn't ad the meta to Relationship::createFromEntityReferenceField. We cannot just add it there, since the actual endpoint wraps the data there resulting in a response like this:

 'jsonapi' =>
  array (
    'version' => '1.0',
    'meta' =>
    array (
      'links' =>
      array (
        'self' =>
        array (
          'href' => 'http://jsonapi.org/format/1.0/',
        ),
      ),
    ),
  ),
  'data' =>
  array (
    0 =>
    array (
      'type' => 'taxonomy_term--tags',
      'id' => 'dae64951-607c-40eb-ae1a-0dcd43c06d65',
      'meta' =>
      array (
        'drupal_internal__target_id' => 1,
      ),
    ),
    1 =>
    array (
      'type' => 'taxonomy_term--tags',
      'id' => '5d756d64-285b-4995-97d3-6ffd6e69761d',
      'meta' =>
      array (
        'drupal_internal__target_id' => 4,
      ),
    ),
    2 =>
    array (
      'type' => 'taxonomy_term--tags',
      'id' => '62a69bcd-cf8e-4fdf-aea9-2d31c17678da',
      'meta' =>
      array (
        'drupal_internal__target_id' => 2,
      ),
    ),
    3 =>
    array (
      'type' => 'taxonomy_term--tags',
      'id' => '029ac65b-a2c8-41e7-b282-33e1e8658dae',
      'meta' =>
      array (
        'drupal_internal__target_id' => 5,
      ),
    ),
  ),
  'meta' =>
  array (
    'relationship_meta_name' =>
    array (
      0 => 'fy1KMvbZ',
      1 => 'm3ud47pS',
      2 => 'aoJRiFR5',
      3 => 'ePOgnluT',
    ),
  ),
  'links' =>
  array (
    'related' =>
    array (
      'href' => 'https://drupal-core-9-5-x.swisdev.nl/jsonapi/node/article/47b8e4b3-0a89-4c72-b4f0-c169365b4768/field_tags?resourceVersion=id%3A1',
    ),
    'self' =>
    array (
      'href' => 'https://drupal-core-9-5-x.swisdev.nl/jsonapi/node/article/47b8e4b3-0a89-4c72-b4f0-c169365b4768/relationships/field_tags',
    ),
  ),
)

That won't do, we need to find a way to do this a little different there.

I've looked into this a little more. The result is expected, this is exactly how the relationship is renderen in the resource also. This makes sense. Double checked the spec, and all good. Added a test for the relationship endpoint that is renders correctly.

I was worries a bit more :)

Rebased onto 10.1.x, lets see if things become green again.

Because reasons. My rebase was screwed over and decided to cherrypick to a fresh 10.1.x branch. MR !2794 is the correct one. Let's see if tests are still green.

Hopefully it will show up at some point.

Link to mr

Posted a first review round!

I think the change record should be expanded with a concrete example, to make it clear what kinds of problems you can solve with this 🤓

Rebased the current MR as it was not mergeable, still needs work for feedback posted on MR

I've tried to go through the feedback. I agree on a good example as mentioned in #51. Still setting to NR though ^^

@ravi.shankar your rebase was a bit of a mess. Please don't do random merges as rebase. That is not a rebase. Please have a look here.

I rebased, and fixed the comments made.

Regarding #51. The example in the tests adds some specific entity data to the request. I kinda feel that is a pretty good way to understand how this works. This information could be anything related to the entity that might not be a field.

A really concrete example will probably be something related to commerce. Lets see if i can get @mglaman to give us one.

MR needs to be updated to be mergeable

Rebased as asked for by @_nod, even though it kinda felt unneeded. I'll have a look at the CR.

Added a proper example to the CR based on the post of mglaman. :)

I think this is looking great, but to ensure this does not cause unexpected side effects in performance or existing caching infrastructure, I think some more work is needed 🤓

1. Change record looks great — just added formatting tweaks.
2. This requires updating jsonapi.api.php, tagging Needs documentation for that.
3. I'm missing test coverage for meta that varies across request context (different user/different role/different language/whatever).
4. This MR introduces event dispatching in various performance-critical pieces of JSON:API. I'm fine with that costing something for sites that choose to implement this (although I would like to see numbers). But did we measure that there's no significant overhead for the >90% of sites that don't use this?

Ok, i'm mentally getting there after a few different tries. Might just do a cache context of a user for something like the author of the entity. This could work. Caching is always a fun area to explore. Think I could loose a week in that subsystem :x

Also, I've profiled with blackfire and on a site with a silly amount of relations (resulting in 16k events being despatched) it added 1k extra. Which was milliseconds.

When I get a little farther on this in regards to the caching and such I will do a new profile run. But it seems the event dispatching is pretty free which is to be expected since it is pretty much a few checks if a key is set in an array (which php does pretty fast nowadays ;)).

Added patch against #33 in 10.1.x

@Gunjan, thanks for the effort at helping, however this seems like an on-sequitur and not a constructive contribution. The work on this issue is being done in an MR, and re-rolling something from 30+ comments ago isn't particularly relevant. Can you help us understand what you're trying to achieve with the patch file you uploaded? Thanks.

I've done some updates and improved the testing. Hopefully it is enough like this.

Mostly a question open if we should return CacheableNormalisation in the events 'getMeta' method.

Another possible open question is if the current context testing is enough or we need a more complex case.

Also from #61:

1. Change record looks great — just added formatting tweaks.
2. This requires updating jsonapi.api.php, tagging Needs documentation for that.
3. I'm missing test coverage for meta that varies across request context (different user/different role/different language/whatever).
4. This MR introduces event dispatching in various performance-critical pieces of JSON:API. I'm fine with that costing something for sites that choose to implement this (although I would like to see numbers). But did we measure that there's no significant overhead for the >90% of sites that don't use this? ) few ms, see #64

Added Wim for reviewing

Removed patches as there's an MR
Adding issue credits, and removing credits for simple re-rolls/rebases
Updating issue summary and tags
Makes the job of reviewers easier ❤️

Left a review

Added #3375453: [policy, no patch] Decide on policy regarding use of Events class with string constants for event names or class names instead for discussion

Trying to get this moving forward again 😄

Merged in 11.x, addressed phpcs violations, left a review, and … there's still @larowlan's review to address.

(Trying to only touch trivial things to make things easier but keep myself distant enough to be able to RTBC this!)

The tests now confirm the failure reported by @ptmkenney on the MR 👍 The failure also happens in kernel tests, which means it should be an easy fix 😄

Sorry to add a little noise to this ticket. I wanted to create a patch to test this via composer patches.

Ok, i've tried to adjust to the comments on the MR. Only not sure about my context test, think it tests the context switch, but perhaps its not nested enough?

Rest seems to be adressed now, or at least explained ;)

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The change record draft needs to be updated because MetaDataEvents was removed. I would update it myself but I don't understand all the new changes.

Updated the changerecord to reflect the removal of the MetaDataEvents class. Fixed codestyle issues.

I'll wait for ci ;)

Fixed spellcheck by rebasing (wonder if we should automate that...), tests are running.

Possible todos (from #61):

This MR introduces event dispatching in various performance-critical pieces of JSON:API. I'm fine with that costing something for sites that choose to implement this (although I would like to see numbers). But did we measure that there's no significant overhead for the >90% of sites that don't use this?

As per #64, it adds about 1k extra events (on 16k currently) and adds a few ms uncached, which i would say is fine. Forgot to remove the tag there, sorry.

Tests failed, needs work.

Fixed event dispatching, also fixed the fact the code tried to add cachability data to non-cachable reponses. Added an extra check in the testing module so we get not warning on a non existant array key.

Ready for review.

I don't understand enough about the internals of JSON:API to do a code review, but I can do an end user review because I have a mobile app that uses JSON:API as a backend. I checked out the latest version of this MR and ran my app's automated tests; everything passed. I then poked around in the app for a few minutes and the metadata is added correctly as I expected. I use the meta info to return lots of extra details about user entities, so this is critical functionality for my app.

Just one comment on the change record. I would suggest adding comments to the functions as per the coding standards so that it's more clear that addPaymentInfoToResourceObjectMeta() is an arbitrary name for a custom method, whereas getSubscribedEvents() is the event subscriber method.

Something like:

  /**
    * {@inheritDoc}
    */ 
  public static function getSubscribedEvents() {
    return [
      CollectResourceObjectMetaEvent::class => 'addPaymentInfoToResourceObjectMeta'
    ];
  }

  /**
    * Custom method to addd payment info to the order object.
    *
    * @param \Drupal\jsonapi\Events\CollectResourceObjectMetaEvent $event
    *   The event to add meta resources for.
    *
    * @return void
    */ 
  public function addPaymentInfoToResourceObjectMeta(CollectResourceObjectMetaEvent $event) {
    if ($event->getResourceObject()->getTypeName() !== 'order--order') {
      return;
    }

    $event->setMeta('payment_iframe_url', \Drupal::service('custom_payment.payment_data')->getIframeUrl());
  }

Thank you, i've updated the changerecord. And thank you for the real world testing :)

Sorry to do it, but can we have two MRs now

10.3.x with the deprecation
11.x with the items removed or required

Thanks.

Bjorn's MR will need its target changed but added one against 11.x that can stay that way, with deprecations removed.

Updating IS covering last "remaining tasks." Profiling was cleared up in #82 and I think it's enough to add @Event to the new events for API docs generation.

By way of cross-promotion, API docs are really hard to generate and maintain. Meta for better docs at #3441158: [Meta, Plan] Move official Drupal docs into repository, host from static site CI artifact.

Did the dance, unfortunately a true rebase onto the other branch did not work, so force pushed the diff. Setting to NR, pipeline is pending.

Not sure if something in gitlab got stuck? But seeing a 10.3.x test failure in jsonApi

I dunno about stuck but it looked like it could be a random, but just retried and same outcome. https://git.drupalcode.org/issue/drupal-3100732/-/pipelines/159676/test_...

I'll try to take a look today to help keep this moving.

There was an issue with user login which broken 10.3 head. Which after also has a few other things fail. It might be something there. Too tired to check today though.

I've fixed the pipeline by using drupalResetSession in the test, which logs out the user through code.

It is kinda weird though, that loggin out didn't work through the interface, but that is isolated from this issue.

Did a quick fix for the logOut issue bases on the changes here: #144538: User logout is vulnerable to CSRF. There where changes in how we logout, we need those changes in this test also.

I tried removing the mink session reset, but that was needed, so put it back in.

Thanks for fixing! Left some small comments looking good!

11.x is updated, ill push the same changes to the 10.3 mr (excluding the BC removal)

Update 10.3 mr also, all done <3

Feedback appears to be addressed

Reran the kernel test and it was random failure.

Hi Folks, unfortunately we've missed the window for 11.0.x and 10.3.x now.
I think we need to re-do this 10.4.x as the 'from' in the deprecation notice and 12.0.x for removal.
The only consolation is at least then there's only one MR.

Thanks for all the effort here.

Simple rebase, nothing changed but the deprecation message. Updated the CR and target for MR. Think those are small anough changes for a direct RTBC

I read the issue summary and the MR. I skimmed the comments and didn't see any unanswered questions, although like I said I only skimmed (It is noisy here and hard to concentrate).

I left comments in the MR about the comments. I also think that this should not use $rootUser. I did not do a code review.

Setting to NW.

Resolved all feedback.

1. Add an admin user and check for an admin role in the testing
2. Improve some comments to help developers use this feature.
3. Update tests.

All good I think :)

Can the MR 7828 be updated for 11.x please

Rebased onto 11.x and fixed a small issue with a missing return type.

MR for 11.x is showing as all green

All threads appear to have been addressed

CR reads well to me and even better has examples (love those)

Running the test-only feature gets https://git.drupalcode.org/issue/drupal-3100732/-/jobs/2141544 showing the coverage

Code review wise nothing seems to be off that hasn't been fixed already
Deprecation message appears correct.
Everything appears to be typehinted.

Believe this one is good to go.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

In #113, needs-review-queue-bot only reported a missing strict types declaration. I added this and am setting back to RTBC since it is an extremely minor change and all tests are passing.

Rebasing.

Typo at L43 core/modules/jsonapi/tests/modules/jsonapi_test_meta_events/src/EventSubscriber/MetaEventSubscriber.php, also L87, "// Only continue if the recourse type is enabled.", presumably recourse should be resource.
https://git.drupalcode.org/project/drupal/-/merge_requests/7828/diffs#3e...

Fixed the spelling mistake identified by @jonathan_hunt in #116. Since this was just a single word, I don't think we need to change the RTBC status.

Committed to 11.x - great work folks

Published the CR

Amending attribution.