- Merge request !2794Issue #3100732: Allow specifying `meta` data on JSON:API objects → (Closed) created by bbrala
- last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago Custom Commands Failed - last update
over 1 year ago 29,485 pass, 2 fail - Status changed to Needs review
over 1 year ago 8:11pm 16 June 2023 - 🇳🇱Netherlands bbrala Netherlands
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.
- 🇳🇱Netherlands bbrala Netherlands
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
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 ❤️ - Status changed to Needs work
over 1 year ago 9:04pm 17 July 2023 - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
- last update
over 1 year ago 29,790 pass, 16 fail - last update
over 1 year ago Custom Commands Failed - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Trying to get this moving forward again 😄
Merged in
11.x
, addressedphpcs
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!)
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
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 😄
- 🇺🇸United States jrockowitz Brooklyn, NY
Sorry to add a little noise to this ticket. I wanted to create a patch to test this via composer patches.
- Status changed to Needs review
7 months ago 9:42am 12 April 2024 - 🇳🇱Netherlands bbrala Netherlands
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 ;)
- Status changed to Needs work
7 months ago 12:08pm 12 April 2024 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.
- 🇯🇵Japan ptmkenny
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.
- 🇳🇱Netherlands bbrala Netherlands
Updated the changerecord to reflect the removal of the MetaDataEvents class. Fixed codestyle issues.
I'll wait for ci ;)
- Status changed to Needs review
7 months ago 9:16am 14 April 2024 - 🇳🇱Netherlands bbrala Netherlands
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.
- Status changed to Needs work
7 months ago 10:33am 14 April 2024 - Status changed to Needs review
7 months ago 11:11am 19 April 2024 - 🇳🇱Netherlands bbrala Netherlands
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.
- 🇯🇵Japan ptmkenny
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()); }
- 🇳🇱Netherlands bbrala Netherlands
Thank you, i've updated the changerecord. And thank you for the real world testing :)
- Status changed to Needs work
7 months ago 4:51pm 28 April 2024 - 🇺🇸United States smustgrave
Sorry to do it, but can we have two MRs now
10.3.x with the deprecation
11.x with the items removed or requiredThanks.
- 🇺🇸United States bradjones1 Digital Nomad Life
bradjones1 → changed the visibility of the branch 3100732-rebase to hidden.
- 🇺🇸United States bradjones1 Digital Nomad Life
Bjorn's MR will need its target changed but added one against 11.x that can stay that way, with deprecations removed.
- 🇺🇸United States bradjones1 Digital Nomad Life
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 → .
- Merge request !7828Add changes from earlier branch to 10.4.x, rebase onto 11.x → (Open) created by bbrala
- 🇳🇱Netherlands bbrala Netherlands
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.
- Status changed to Needs review
7 months ago 11:32am 29 April 2024 - Status changed to Needs work
7 months ago 1:27pm 29 April 2024 - 🇺🇸United States smustgrave
Not sure if something in gitlab got stuck? But seeing a 10.3.x test failure in jsonApi
- 🇺🇸United States bradjones1 Digital Nomad Life
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.
- 🇳🇱Netherlands bbrala Netherlands
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.
- 🇳🇱Netherlands bbrala Netherlands
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.
- Status changed to Needs review
7 months ago 8:18am 30 April 2024 - 🇳🇱Netherlands bbrala Netherlands
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.
- Status changed to Needs work
7 months ago 2:08pm 30 April 2024 - 🇺🇸United States smustgrave
Thanks for fixing! Left some small comments looking good!
- Status changed to Needs review
7 months ago 2:39pm 30 April 2024 - 🇳🇱Netherlands bbrala Netherlands
11.x is updated, ill push the same changes to the 10.3 mr (excluding the BC removal)
- Status changed to RTBC
7 months ago 3:10pm 30 April 2024 - 🇺🇸United States smustgrave
Feedback appears to be addressed
Reran the kernel test and it was random failure.
- Status changed to Needs work
6 months ago 10:15pm 19 May 2024 - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
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.
- Status changed to RTBC
6 months ago 5:56am 20 May 2024 - 🇳🇱Netherlands bbrala Netherlands
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
- Status changed to Needs work
6 months ago 3:32am 28 May 2024 - 🇳🇿New Zealand quietone
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.
- Status changed to Needs review
5 months ago 11:01am 28 June 2024 - 🇳🇱Netherlands bbrala Netherlands
Resolved all feedback.
- Add an admin user and check for an admin role in the testing
- Improve some comments to help developers use this feature.
- Update tests.
All good I think :)
- Status changed to Needs work
4 months ago 2:11pm 16 July 2024 - Status changed to Needs review
4 months ago 8:05am 17 July 2024 - 🇳🇱Netherlands bbrala Netherlands
Rebased onto 11.x and fixed a small issue with a missing return type.
- Status changed to RTBC
4 months ago 3:06pm 17 July 2024 - 🇺🇸United States smustgrave
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.
- Status changed to Needs work
about 1 month ago 4:00pm 8 October 2024 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.
- 🇯🇵Japan ptmkenny
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.
- 🇳🇿New Zealand jonathan_hunt
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.", presumablyrecourse
should beresource
.
https://git.drupalcode.org/project/drupal/-/merge_requests/7828/diffs#3e...