This is a sub-task of #2937279: [META] Introduce concept of internal resource types.

This purely introduces a new isInternal() method to ResourceType and adds a way to set that.

Removing internal resource types from routes, links, and includes will be a separate issue to make reviewing this simpler.

CommentFileSizeAuthor
#6 2939704-6.patch2.26 KBgabesullice
#6 interdiff-2939704-6.txt1.17 KBgabesullice
#2 2939704-2.patch1.46 KBgabesullice

Comments

gabesullice created an issue. See original summary.

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new 2939704-2.patch1.46 KB
gabesullice’s picture

gabesullice
Primary language English
commented
Issue tags: +blocker
gabesullice’s picture

gabesullice
Primary language English
commented

Updating credits

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

I'd expect this to @see the core interface where this will also be added, and similar docs.

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2939704-6.txt1.17 KB
new 2939704-6.patch2.26 KB

@Wim Leers, the @see is intentionally absent, per #2932035-31: ResourceTypes should be internal when EntityType::isInternal is TRUE.

JSON API Extras might make a resource type internal for entirely different reasons than TypedData::isInternal. So they're related concepts but not the same thing.

When we start to use the core method to define internal resource types, we should simply treat it as a "sensible default". Meaning the documentation and @see should be next to the relevant code (probably in the ResourceTypeRepository).

Nonetheless, this could use more thorough docs.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

When we start to use the core method to define internal resource types, we should simply treat it as a "sensible default".

Completely agreed!

Meaning the documentation and @see should be next to the relevant code

That sounds better than what I proposed indeed :) In what issue should that happen then?

gabesullice’s picture

gabesullice
Primary language English
commented

That sounds better than what I proposed indeed :) In what issue should that happen then?

#2932035-30: ResourceTypes should be internal when EntityType::isInternal is TRUE ;)

wim leers’s picture

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

The IS says:

This depends on #2937961: ResourceType should provide related ResourceTypes (unfortunately).

But I didn't see why. I checked with @gabesullice and he confirmed it can go in in parallel with #2937961.

Both provide infrastructure necessary for #2932035.

  • Wim Leers committed 71e39f5 on 8.x-1.x authored by gabesullice 
    Issue #2939704 by gabesullice, Wim Leers: Add ResourceType::isInternal()
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Fixed
gabesullice’s picture

gabesullice
Primary language English
commented
Issue summary: View changes
Status: Fixed » Needs review

Woohoo!

Fixed the IS, FTR.

And for the benefit of any future Drupal.org archivists and archeologists... this actually was blocked by that patch, but after many revisions it was no longer the case.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Fixed
gabesullice’s picture

gabesullice
Primary language English
commented

Concurrent editing fail FTW

Status: Fixed » Closed (fixed)

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