Allow specifying `meta` data on JSON:API objects

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:

2. This requires updating jsonapi.api.php, tagging Needs documentation for that.
3. .
4. ) 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 📌 [policy, no patch] Decide on policy regarding use of Events class with string constants for event names or class names instead Active 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.

bradjones1 → changed the visibility of the branch 3100732-rebase to hidden.

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.

bbrala → closed merge request !2794

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: 🐛 User logout is vulnerable to CSRF Fixed . 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.

bbrala → changed the visibility of the branch 3100732-11.x to hidden.

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.