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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gabesullice created an issue. See original summary.

gabesullice’s picture

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

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

gabesullice
Primary language English
CreditAttribution: gabesullice at Acquia commented

Updating credits

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia 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
CreditAttribution: gabesullice at Acquia commented
Status: Needs work » Needs review
FileSize
interdiff-2939704-6.txt1.17 KB
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 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia 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
CreditAttribution: gabesullice at Acquia 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 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia 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 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Reviewed & tested by the community » Fixed
gabesullice’s picture

gabesullice
Primary language English
CreditAttribution: gabesullice at Acquia 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 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Fixed
gabesullice’s picture

gabesullice
Primary language English
CreditAttribution: gabesullice at Acquia commented

Concurrent editing fail FTW

Status: Fixed » Closed (fixed)

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