1. All the code in the Drupal\jsonapi\Context, Drupal\jsonapi\Query and Drupal\jsonapi\Routing namespaces looks okay, precisely because it's only not exposing APIs, it's not interacting with key APIs: it's all just logic to support the JSON API spec. Which means it's exactly the kind of code that is likely to be refactored. Marking it @internal would signal to developers that they should not be extending or even interacting with these objects. They're implementation details, that can change at any time.
  2. Also, why are \Drupal\jsonapi\Relationship(Interface) and \Drupal\jsonapi\EntityCollection(Interface) not in a similar subnamespace?
CommentFileSizeAuthor
#15 interdiff.txt1.18 KBwim leers
#15 2831187-15.patch21.5 KBwim leers
#14 interdiff.txt755 byteswim leers
#14 2831187-14.patch20.99 KBwim leers
#12 2831187-12.patch20.89 KBwim leers
#7 2831187-7.patch9.59 KBwim leers
#6 2831187--internal-apis--6.patch18.63 KBe0ipso
#4 2831187--internal-apis--4.patch14.68 KBe0ipso

Comments

Wim Leers created an issue. See original summary.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
  1. +1
  2. No particular reason. They both relate to the normalization process. One allows us to have virtual relationships that are not backed by a stored ER. The other one is a wrapper to normalize collections with multiple entities. Maybe move them to the Normalizer namespace?
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Assigned: Unassigned » e0ipso
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Assigned: e0ipso » Unassigned
Status: Active » Needs review
StatusFileSize
new 2831187--internal-apis--4.patch14.68 KB

Is this what you meant @Wim Leers?

Status: Needs review » Needs work

The last submitted patch, 4: 2831187--internal-apis--4.patch, failed testing.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs work » Needs review
StatusFileSize
new 2831187--internal-apis--6.patch18.63 KB

Fixed tests.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 2831187-7.patch9.59 KB

LOL I wanted to apply your patch, only to then notice that I already rolled a patch myself that I forgot to upload. Dammit!

This is what I had locally.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Ignore #7, I just uploaded that for completeness.

Reviewed #6. Yes, that's exactly what I meant :)

Just one thing:

+++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
similarity index 94%
rename from src/Relationship.php

rename from src/Relationship.php
rename to src/Normalizer/Relationship.php

+++ b/src/Normalizer/Relationship.php
similarity index 95%
rename from src/RelationshipInterface.php

rename from src/RelationshipInterface.php
rename to src/Normalizer/RelationshipInterface.php

I think these classes would benefit from a comment explaining their purpose, just like you did here in #2.2.

Status: Needs review » Needs work

The last submitted patch, 7: 2831187-7.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

#8 would be nice to have happen, but in the mean time, #6 is still a step forward.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » wim leers
Status: Reviewed & tested by the community » Needs work

The patch here no longer applies. Rerolling…

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new 2831187-12.patch20.89 KB

Straight reroll of @e0ipso's patch in #6. Conflicted with #2831185: Rename DocumentWrapper to JsonApiDocumentTopLevel and DocumentRootNormalizer to JsonApiDocumentTopLevelNormalizer.

Status: Needs review » Needs work

The last submitted patch, 12: 2831187-12.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new 2831187-14.patch20.99 KB
new interdiff.txt755 bytes

Made one small mistake in rebasing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 2831187-15.patch21.5 KB
new interdiff.txt1.18 KB

Now I addressed my own feedback in #8: I added the explanation you posted in #2.2 to the code docs.

However, upon doing so, I noticed that the documentation lives on Relationship, i.e. the implementation, not the interface. So I did the same for EntityCollection.

I think it's clear that in this case, having interfaces is pointless: there will NEVER BE multiple implementations. These are value objects used only in JSON API's normalization process. So it's fine to NOT have interfaces. Doing that is out of scope of this issue though, I think. So not doing that just yet.

I think this patch is ready again! I only rebased it, and added e0ipso's comments from #2.2. I think it's fine for me to re-RTBC this.

  • e0ipso committed 040ec79 on 8.x-1.x 
    docs(Maitainability) Mark internal things as @internal (#2831187 by Wim...
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Reviewed & tested by the community » Fixed

I did a small re-roll during the merge. This is fixed now.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Next step: #2840948: Remove interfaces of JSON API-specific normalizers.

Status: Fixed » Closed (fixed)

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