ResourceTypes should be internal when EntityType::isInternal is TRUE

There is a little wonkiness when dealing with related routes and relationships. How should we handle those?

JSON API Extras must already be dealing with this, because it too allows one to remove entity types. Which strategy does it use? And why? We should document it in jsonapi.api.php.

Regardless of what we decide above, I realized that EntityDataDefinition has isInternal and setInternal as available methods (thanks @tedbow). Perhaps we should just be using that instead of this hook mechanism.

I'd love to see that explored here indeed, so we can remove several of core's hook_rest_resource_alter() implementations in favor of that, and then core's metadata will also affect the JSON API module. Which means we won't need duplicate hook implementations, as the current patch necessitates.

1. +++ b/jsonapi.api.php
   @@ -87,3 +87,28 @@
   + * *Warning* this is *not* a replacement for proper entity access checking.
   

   Good call :)

2. +++ b/jsonapi.api.php
   @@ -87,3 +87,28 @@
   +function hook_jsonapi_internal_resources() {
   

   We should mark this @internal, because we know it will change in the future, so that entity types' metadata can declare whether they're internal or not. (As you already mentioned at the bottom of #4.)

   This is just a temporary work-around, mirrored after hook_rest_resource_alter(), and with the same limitations.

3. +++ b/src/Normalizer/RelationshipNormalizer.php
   @@ -72,7 +72,9 @@ class RelationshipNormalizer extends NormalizerBase {
   -      $normalizer_items[] = $this->serializer->normalize($relationship_item, $format, $context);
   +      if ($relationship_item->getTargetResourceType()) {
   +        $normalizer_items[] = $this->serializer->normalize($relationship_item, $format, $context);
   +      }
   

   I'm surprised we need to modify this — I'd have thought https://www.drupal.org/project/jsonapi_extras already had to handle this? Perhaps it forgot to handle this portion?

👍

Installed it and verified that it does not handle these cases. I also looked through the tests and found nothing that indicates that it should.

This is totally a bug in JSON API Extras. Although not very important, it needs to be fixed regardless.

Disabling resources was always intended to be an option only at the terminal leaves of the data model tree. The reason is that JSON API (with good criteria IMO) requires you to have full linkage, so intermediate resource types are necessary to reach the desired resource type in the tree. In addition to that, we want all paths to reach from A to B (A ➔ B, A ➔ C ➔ C ➔ A ➔ D ➔ B, …) because they can yield a different set of results.

The problem is that there is no way to determine if a resource type is terminal from the module's perspective. Hence we're allowing disabling any resource, but that requires the good judgement of the implementor. This is even highlighted in the UI in JSON API Extras.

unfortunately, this will be yet another dependency on FieldResolver *sigh*

I'm more optimistic. The field resolver is at the core of everything. Just like the resource types are. It should become ubiquitous, that means that we no longer have coupling between: API public names, field names, Entity Query columns and field properties. To me, that is great news!

Given that perspective, I wonder if there's a way for us to purify that abstraction, make it more explicit, and thereby simplify the rest of the module. To make it blatantly obvious that the module is built on Resources and Fields (which makes sense that it would be... http://jsonapi.org/format/#document-resource-object-fields).

I'm OK with that. I guess we can explore what that would look like. The challenge is not obvious due to the enormous surface we have (think of all the data model combinations possible we are exposing multiplied by the number of interactions among them we support), so I expect a big amount of special cases.

#9–#13 all sound great, and very insightful :) I hadn't heard of "full linkage", despite having read the entire spec. Just reading it results in less understanding than implementing the spec of course!

And further simplifying sounds like music to my ears!

#7 + #9 was fixed by @e0ipso in #2933615: Fix related and relationships endpoints for disabled resources (jsonapi) + #2933354: Disabled resources are still accessible via related routes and includes (jsonapi_extras) AFAICT :)

And further simplifying sounds like music to my ears!

I'm not sure than purify that abstraction, make it more explicit will result on and thereby simplify the rest of the module. But I'm interested to see what Gabe has in mind once he's acquainted with the module's gotchas.

#7 + #9 was fixed by @e0ipso in […]

Yep! It was about time we had tests in JSON API Extras. I had hoped that someone would step in for that, but it's been too long and that wasn't happening.

+++ b/tests/src/Kernel/ResourceType/ResourceTypeRepositoryTest.php
@@ -50,6 +52,11 @@ class ResourceTypeRepositoryTest extends KernelTestBase {
+    // Filter the 'node--article' resource out.
...
+    $module_handler->invokeAll('jsonapi_internal_resources')->willReturn(['user--user']);

I think the comment is outdated.

Please take a look at JSON API Extras to learn about the intricacies of disabling resources. You'll find that there are cases where you want the resource type to get info from it, even if it's disabled.

@gabesullice can you please explain why this is blocked?

@gabesullice I still don't see why we need to be blocked waiting on core (potentially it can take months to solve the simplest of things there).

The title says Provide mechanism to exclude "internal" entities until a more generic option is supported by core but then we are postponing waiting on core to support it. It's weird.

We should have an API to disable a resource (see the pointer on #20) and trigger that based on whatever. For now it can be the same hook core uses currently. Whenever a more appropriate solution is provided, we can come back and iterate the trigger.

#26 sounds like a good plan! Can I summarize it as:

1. be pragmatic while #2936725: EntityDataDefinition::create() does not respect derived entity definitions is getting fixed in core
2. introduce the infrastructure that #2936725 will allow the JSON API module to use
3. … and then use this infrastructure to let JSON API Extras already use it, which at the same time validates this infrastructure ahead of time, before #2936725 lands, which means that when #2936725 lands, it'll be a trivial addition

?

Cool :)

So should this issue then not be unpostponed? Or do we want to keep this issue for doing which means that when #2936725 lands, it'll be a trivial addition?

Whew, what a sentence!

😅

I of course should have said Should this issue then be no longer marked as postponed?

#2936725: EntityDataDefinition::create() does not respect derived entity definitions landed!

#2939704: Add ResourceType::isInternal() landed in JSON API! This is still blocked on:

1. #2936714: Entity type definitions cannot be set as 'internal' in core
2. #2937961: ResourceType should provide related ResourceTypes in JSON API

It looks like the "PP" number was only tracking the core blockers, not the JSON API blockers. Updating to reflect the current state.

Although … I'm not sure that it makes sense to block this on #2936714: Entity type definitions cannot be set as 'internal', since that will at best be available in 8.5, maybe only 8.6, but definitely not 8.4. We don't want to wait for 8.5 to update JSON API to do this. So … we won't be able to rely on EntityType::isInternal() on 8.4 sites anyway.

Can't we let JSON API hardcode the assumptions for Drupal 8.4 sites? There's only one entity type that would be considered "internal": content_moderation_state. That'd allow us to move forward here without #2936714 landing. Although I guess the point is that we wait for #2936714 to land so that we can already be forward-compatible with 8.5/8.6. That'd be ideal!

👌

#2936714: Entity type definitions cannot be set as 'internal' landed in core this morning!

I committed #2937961: ResourceType should provide related ResourceTypes yesterday!

Which means this is now unblocked :)

There now is a sister issue for this against core's rest module: #2941622: Make REST's EntityResource deriver exclude internal entity types and mark ContentModerationState entity type as internal instead of the current hack.

#2936714: Entity type definitions cannot be set as 'internal' landed in core this morning!

Should we worry about using core fixes in +8.5 in the contrib codebase? This will break sites running supported core versions. I'm still not sure how to proceed in these cases (this has come up previously).

Yep.

Can we get a reroll here that is reviewable?

1. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -80,10 +81,12 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +            static::shouldBeInternalResourceType($entity_type)
   

   This is the key change: that's now setting the new optional $internal parameter! It uses the API that #2939704: Add ResourceType::isInternal() added.

2. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -122,6 +125,23 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +    // @todo: remove when minimum supported core version is >= 8.5.
   +    if (method_exists(EntityTypeInterface::class, 'isInternal')) {
   +      return $entity_type->isInternal();
   +    }
   +    return $entity_type->id() === 'content_moderation_state';
   

   I'd flip this around, so that we will only need to delete lines in the future, rather than modifying each of them:

   if (!method_exists(…))
    {
     return $… == 'content_moderation_state';
   }
   
   return $entity_type->isInternal();
   

   … oh wait, we'll be able to remove this protected helper method altogether once JSON API requires 8.5! Never mind me :)

3. Also, no need for documentation, since this is not adding any new temporary API.

But I do think the @todo should be moved for clarity.

Also, the issue title is no longer accurate.