Problem/Motivation

With #3015438: Wrap entity objects in a ResourceObject which carries a ResourceType done, renaming EntityCollection to ResourceObjectCollection will make the code simpler to understand and hence maintain.

Because EntityCollection is used in quite a few parts of the code base, and EntityCollection containing ResourceObject is a clear case of a confusing mismatch.

Proposed resolution

Rename it.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#33 3032679-33.patch42.86 KBgabesullice
#24 jsonapi-extras-3032679-23.patch997 bytesgabesullice
#20 3032679-20.patch42.96 KBgabesullice
#20 interdiff.txt8.36 KBgabesullice
#19 3032679-19.patch38.84 KBgabesullice
#5 3032679-5.patch41.06 KBwim leers
#3 3032679-3.patch42.22 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Clean-up: rename EntityCollection to ResourceObjectCollection » [PP-1] Clean-up: rename EntityCollection to ResourceObjectCollection
Status: Active » Postponed

Postponed on #3015438: Wrap entity objects in a ResourceObject which carries a ResourceType.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 3032679-3.patch42.22 KB

Contrib modules JSON:API Extras and JSON:API Defaults have been verified to still work without any changes after this is applied.

gabesullice’s picture

gabesullice
Primary language English
commented
Title: [PP-1] Clean-up: rename EntityCollection to ResourceObjectCollection » [PP-1] Clean-up: rename EntityCollection to PrimaryData

If we're gonna rename it, let's just rename it to PrimaryData, because that is the JSON:API member it represents.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new 3032679-5.patch41.06 KB

Actually, "(resource) collection" is also mentioned in the spec twenty times. Naming it PrimaryData only makes sense in some use cases. Naming it PrimaryData results in:

+   * @param \Drupal\jsonapi\JsonApiResource\PrimaryData $includes
+   *   The resources to be included in the document.
    * @param int $response_code
    *   The response code.
    * @param array $headers
@@ -858,7 +856,7 @@ class EntityResource {
    * @return \Drupal\jsonapi\ResourceResponse
    *   The response.
    */
-  protected function buildWrappedResponse($data, Request $request, EntityCollection $includes, $response_code = 200, array $headers = [], LinkCollection $links = NULL, array $meta = []) {
+  protected function buildWrappedResponse($data, Request $request, PrimaryData $includes, $response_code = 200, array $headers = [], LinkCollection $links = NULL, array $meta = []) {

That doesn't quite make sense either?

That probably indicates that we're using one value object for different semantical purposes.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-1] Clean-up: rename EntityCollection to PrimaryData » Clean-up: rename EntityCollection to PrimaryData
Status: Postponed » Active

#3015438: Wrap entity objects in a ResourceObject which carries a ResourceType just landed.

gabesullice’s picture

gabesullice
Primary language English
commented

What about ResourceData?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Why is that preferable to #3?

gabesullice’s picture

gabesullice
Primary language English
commented

I'm trying to draw the connection to the data member of the top-level object.

"Collection" just feels strange to me because it can have a cardinality of 1 (yes, "data" is also plural, but it works for both "the node data" too).

Also, it can technically carry ResourceIdentifiers and EntityAccessDeniedExceptions in addition to ResourceObjects

effulgentsia’s picture

effulgentsia commented

I'm trying to draw the connection to the data member of the top-level object.

Per #5, is the same class also used for the included member?

effulgentsia’s picture

effulgentsia commented

Also, is this class ever used for a member that isn't data or included? For example, is it ever used for relationships.foo?

gabesullice’s picture

gabesullice
Primary language English
commented

#10, yep. However, from the spec's perspective, there's no behavioral difference between the included member and the data member when cardinality is >1. IOW, included is just "secondary/tertiary/n-ary data" depending on how many relationships have been traversed by an include.

effulgentsia’s picture

effulgentsia commented
Title: Clean-up: rename EntityCollection to PrimaryData » Clean-up: rename EntityCollection to ResourceData

@gabesullice explained to me that this class would eventually represent the data that goes into one of the following document members:

  • data
  • included (which has the same syntax and semantics as data)
  • (data|included).relationships.foo.data (which ends in .data)

Therefore, I like the name containing the word Data.

Per the spec, this data can be any one of:

  • null
  • an empty array
  • a ResourceObject
  • a ResourceIdentifier
  • an array of ResourceObjects
  • an array of ResourceIdentifiers

Therefore, I don't like the name containing the word Collection (as it misleads with respect to some of those cases).

So, +1 to ResourceData.

effulgentsia’s picture

effulgentsia commented
Title: Clean-up: rename EntityCollection to ResourceData » Clean-up: rename EntityCollection to ResourceOrCollectionData or Data

Therefore, I don't like the name containing the word Collection (as it misleads with respect to some of those cases).

But, I just realized that the word Resource also misleads, since the Data can be about either a resource or a collection of resources.

Therefore, I propose ResourceOrCollectionData, since that uses the spec's terminology of being data about a resource or a collection of resources.

If people are -1 to the word "Or" in a class name, then I think Data is a reasonable alternative as well, since there's nothing in JSON:API other than resources and resource collections that have data.

effulgentsia’s picture

effulgentsia commented
Title: Clean-up: rename EntityCollection to ResourceOrCollectionData or Data » Clean-up: rename EntityCollection to Data

Upon further reflection, I prefer simply Data, since per the last paragraph of #14, "resource or collection" is redundant.

I wonder if we even want to make Data an abstract class, and then have PrimaryData, IncludedData and RelationshipData extend from it? Those child classes might not need to implement or override anything, but could just be there for naming clarity?

gabesullice’s picture

gabesullice
Primary language English
commented

I wonder if we even want to make Data an abstract class, and then have PrimaryData, IncludedData and RelationshipData extend from it?

It like this idea the most. This is using class extension right by providing "specializations". The one thing I would have them do is have an assert() in the constructors ensuring that the includes and primary data only contain ResourceObjects and that RelationshipData only includes ResourceIdentifiers. Also, IncludedData could always set the $cardinality argument to -1 (unlimited).

gabesullice’s picture

gabesullice
Primary language English
commented

Let's leave the "specialization" of the Data class to another issue though.

effulgentsia’s picture

effulgentsia commented

+1: #3033123: Add ResourceObjectData, IncludedData, and RelationshipData specializations of Data.

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 3032679-19.patch38.84 KB
gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new interdiff.txt8.36 KB
new 3032679-20.patch42.96 KB

Forgot a few things.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Hm. The downside of Data is that it is pretty much the most abstract imaginable. Will need to think a bit more about this.

gabesullice’s picture

gabesullice
Primary language English
commented

@Wim Leers, does your opinion change if we commit #3033123: Add ResourceObjectData, IncludedData, and RelationshipData specializations of Data in conjunction with this issue? That makes the abstract Data class abstract 😇

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#22: that issue does make it much better IMHO yes :) I'm fine with doing it in two steps knowing that's the next step.

The only thing preventing me from RTBC'ing: what's the impact of this issue and the combined patch on JSON:API Extras and Consumer Image Styles?

gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new jsonapi-extras-3032679-23.patch997 bytes

There's a one-line change required for JSON:API Extras. Consumer Image Styles is unaffected AFAICT.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Great! And the interesting is that if we convert JSON:API Extras EntityToJsonApi service/API to using subrequests, that this change wouldn't even be necessary. @e0ipso has indicated he's fine with that, so I'll work on a patch to make that happen.

Status: Needs review » Needs work

The last submitted patch, 24: jsonapi-extras-3032679-23.patch, failed testing. View results

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs work » Needs review

It was the Extras patch that failed.

The last submitted patch, 5: 3032679-5.patch, failed testing. View results

The last submitted patch, 3: 3032679-3.patch, failed testing. View results

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#3035045: Make EntityToJsonApi use subrequests so that it never breaks again

I did what I described in #25: #3035045-4: Make EntityToJsonApi use subrequests so that it never breaks again.

gabesullice’s picture

gabesullice
Primary language English
commented

Cool!

Are you proposing that we wait for that vs. the one line change?

wim leers’s picture

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

… that is a very good point! We can leave the choice to @e0ipso to go with either #3053045: Author username and publish date bug or #24.

Still, as soon as we commit this, we should create an issue in JSON:API Extras' issue queue for JSON:API 2.4.

I wrote this in #23:

The only thing preventing me from RTBC'ing: what's the impact of this issue and the combined patch on JSON:API Extras and Consumer Image Styles?

@gabesullice addressed that in #24.

gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new 3032679-33.patch42.86 KB

Needs a reroll. Will commit if tests pass.

The last submitted patch, 20: 3032679-20.patch, failed testing. View results

  • gabesullice committed 3ad0a18 on 8.x-2.x 
    Issue #3032679 by gabesullice, Wim Leers, effulgentsia: Clean-up: rename...
gabesullice’s picture

gabesullice
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Opened #3035134: Compatibility with upcoming JSON:API 2.4 release: JSON:API Extras 3.5

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#36: 👍 — was gonna ask for that, but you already did, thanks!

Status: Fixed » Closed (fixed)

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