Per #2843147-110: Add JSON:API to core as a stable module, we have some changes that need to happen before JSON:API 2.x can go into core.

CommentFileSizeAuthor
#7 suggestion-gabe-do-not-test.patch7.07 KBwim leers
#6 3032468-6-do-not-test.patch15.37 KBwim leers
#6 interdiff.txt1.58 KBwim leers
#4 interdiff-for-3015438-89.txt13.6 KBgabesullice
#2 3032468-2-do-not-test.patch613 byteswim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
Related issues: +#3032451: Compatibility with upcoming JSON:API 2.2 release: JSON:API Extras 3.4
StatusFileSize
new 3032468-2-do-not-test.patch613 bytes

I can't run tests here, because HEAD's tests are failing on DrupalCI. Locally, they're passing just fine though. Furthermore, this needs #3032451: Compatibility with upcoming JSON:API 2.2 release: JSON:API Extras 3.4 to be committed first; there's AFAIK no way for us to test this against JSON:API 2.x-dev + JSON:API Extras with that patch applied.

#3031210: If do_not_use_removal_imminent isn't true, then rename the service ID and tag landed, Consumer Image Styles needs updating. This adds #3031210-9: If do_not_use_removal_imminent isn't true, then rename the service ID and tag to this patch!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

See #3015438-85: Wrap entity objects in a ResourceObject which carries a ResourceType.6 — @gabesullice will be doing one more reroll of this :)

gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new interdiff-for-3015438-89.txt13.6 KB

This adds support to the patch in #2 for #3015438-89: Wrap entity objects in a ResourceObject which carries a ResourceType.

Note, there is a static method on LinkCollectionNormalizer called addLinkRels. This is there to keep the output exactly as it is in HEAD. However, I think this can/should be removed. Consumer Image Styles jumped ahead of JSON:API by providing link rels. JSON:API does not currently normalize link rels because there isn't spec support for it. We're waiting for something like https://github.com/json-api/json-api/pull/1348 to land.

To prevent from having to maintain BC support for meta.rel, I recommend that ^ method be removed from this module before a stable release of 3.x is made.

gabesullice’s picture

gabesullice
Primary language English
commented 
  1. +++ b/src/Normalizer/LinkCollectionNormalizer.php
@@ -68,130 +60,114 @@ class ImageEntityNormalizer extends NormalizerBase implements DenormalizerInterf
-  public function __construct(NormalizerInterface $subject, Negotiator $consumer_negotiator, ImageStylesProvider $imageStylesProvider, ImageFactory $image_factory) {
...
+  public function __construct(NormalizerInterface $inner, Negotiator $consumer_negotiator, ImageStylesProvider $imageStylesProvider, ImageFactory $image_factory, RequestStack $request_stack) {
...
+    $this->requestStack = $request_stack;

    Injecting the request stack means that this normalizer does not rely on $context['request'], which is a hidden dependency on JSON:API internals. 

  2. +++ b/src/Normalizer/LinkCollectionNormalizer.php
@@ -68,130 +60,114 @@ class ImageEntityNormalizer extends NormalizerBase implements DenormalizerInterf
+      return static::addLinkRels($normalization, $variant_links);

    This is what I recommend be removed.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new interdiff.txt1.58 KB
new 3032468-6-do-not-test.patch15.37 KB

Fixing two nits in #4.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new suggestion-gabe-do-not-test.patch7.07 KB

#4's diffstat: 3 files changed, 98 insertions, 123 deletions.. It uses the facilities that have been introduced by #2994193: [META] The Last Link: A Hypermedia Story, and ends up making consumer_image_styles significantly simpler! 🥳

If #5.2 is done by @e0ipso, then it even becomes 5 files changed, 160 insertions(+), 226 deletions(-)! Patch attached that implements the suggestion in #5.2 by Gabe (it applies on top of #6), tests still are green ✅

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

Thanks so much for the patches!

We're waiting for something like https://github.com/json-api/json-api/pull/1348 to land.

There hasn't been any kind of movement (except for me trying to generate GH notifications to draw attention to it) in the issue. Do you reckon this will land soon now that you are an editor of the spec?

To prevent from having to maintain BC support for meta.rel, I recommend that ^ method be removed from this module before a stable release of 3.x is made.

I think I will leave it like this and wait for things to settle before tagging a RC. If it takes too long, I'll keep the current structure around and deprecate it in the next version.

gabesullice’s picture

gabesullice
Primary language English
commented

I think I will leave it like this and wait for things to settle before tagging a RC. If it takes too long, I'll keep the current structure around and deprecate it in the next version.

👍

gabesullice’s picture

gabesullice
Primary language English
commented

There hasn't been any kind of movement (except for me trying to generate GH notifications to draw attention to it) in the issue. Do you reckon this will land soon now that you are an editor of the spec?

@ethan.resnick has promised to post a review soon. Since it targets v1.2, I think it will still take some time (months, hopefully) before any link stuff lands in the spec.

Once there are some initial thoughts on it and the direction doesn't seem to have shifted in a totally different direction, we could make a profile out of it and implement it right away. BC for the profile's vs the spec-level links could be maintained by having a different LinkCollection normalizer for each.

  • e0ipso committed 55a3e48 on 8.x-3.x authored by Wim Leers 
    Issue #3032468 by Wim Leers, gabesullice, e0ipso: Compatibility with...
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs review » Fixed

Thanks so much for this!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.