Do not provide routes for internal resource types

This was a bit of a deep dive. Since we now have a solid way to get relationship data from a resource type, it no longer makes sense to let the relationship routes to be as dynamic (and therefore complex) as they are.

This patch defines a route for every valid relationship and removes the related route parameter.

That is a very big change, which means we'll definitely need sign-off from @e0ipso.

Too late ATM to start reviewing a >40K patch. Will do so tomorrow.

This in #3

This was a bit of a deep dive. Since we now have a solid way to get relationship data from a resource type, it no longer makes sense to let the relationship routes to be as dynamic (and therefore complex) as they are.

and this in #5

That is a very big change, which means we'll definitely need sign-off from @e0ipso.

Make me wonder if we want to take a different approach to the patch: split in two to get the MVP in (Do not provide routes for internal resource types), and then add a follow up for the maintainability bit (Define related and relationship routes statically).

Thoughts?

I agree that @e0ipso's proposal would make this much easier to review and commit. But I suspect that the "no routes for internal resource types" bit becomes much simpler if "define related and relationship routes" lands first. So the latter may need to be a blocker rather than a follow-up.

So @e0ipso was right: statically defined routes (well, explicit routes, really) can be a follow-up.

I think #14 is much easier to understand and hence land. The explicit routes portion can then be a pure refactoring-for-maintainability-reasons follow-up. This issue is in fact the necessary task, the bugfix. The follow-up isn't.

Understanding/analysis:

1. +++ b/src/Controller/EntityResource.php
   @@ -453,30 +455,6 @@ class EntityResource {
   -      if (!$referenced_resource_type) {
   -        throw new NotFoundHttpException(sprintf(
   
   @@ -842,15 +820,20 @@ class EntityResource {
   -  protected function isRelationshipField($entity_field) {
   ...
   +  protected function isRelationshipField(FieldItemListInterface $entity_field) {
   

   👍 All this logic now can elegantly live in this helper method.

2. +++ b/src/Controller/EntryPoint.php
   @@ -70,7 +70,12 @@ class EntryPoint extends ControllerBase {
   +      $resources = array_filter($this->resourceTypeRepository->all(), function ($resource) {
   +        return !$resource->isInternal();
   +      });
   +
   
   +++ b/src/Routing/Routes.php
   @@ -100,6 +100,10 @@ class Routes implements ContainerInjectionInterface {
   +      if ($resource_type->isInternal()) {
   +        continue;
   +      }
   +
   

   👍 These are the essential changes: they solve the problem described in the title.

3. --- a/tests/src/Unit/Routing/RoutesTest.php
   +++ b/tests/src/Unit/Routing/RoutesTest.php
   

   👍 This is testing the essential changes.

Remarks:

1. +++ b/tests/src/Kernel/Controller/EntityResourceTest.php
   @@ -854,7 +947,7 @@ class EntityResourceTest extends JsonapiKernelTestBase {
       * @return \Drupal\jsonapi\Controller\EntityResource
       *   The resource.
       */
   -  protected function buildEntityResource($entity_type_id, $bundle) {
   +  protected function buildEntityResource($entity_type_id, $bundle, $relatable_resource_types = [], $internal = FALSE) {
   

   Docblock not updated.

2. +++ b/tests/src/Kernel/Controller/EntityResourceTest.php
   @@ -872,8 +965,11 @@ class EntityResourceTest extends JsonapiKernelTestBase {
   +    $resource_type = new ResourceType($entity_type_id, $bundle, NULL, $internal);
   +    $resource_type->setRelatableResourceTypes($relatable_resource_types);
   

   If we're changing this ::buildEntityResource() helper anyway, shouldn't we just let it receive a single ResourceType parameter, i.e. basically receiving this $resource_type object that we're manually constructing here?

   Nah, never mind, that is again expanding scope. That'd be another nice small refactoring issue that could go in easily later.

3. +++ b/tests/src/Kernel/Controller/EntryPointTest.php
   @@ -4,6 +4,8 @@ namespace Drupal\Tests\jsonapi\Kernel\Controller;
   @@ -27,24 +29,72 @@ class EntryPointTest extends JsonapiKernelTestBase {
   

   Are the changes to this test actually necessary?

   It's effectively changing this from a kernel test to a unit test. I think the whole point was that this was testing what a particular set of modules was resulting in?

Also, now that #2932035: ResourceTypes should be internal when EntityType::isInternal is TRUE is in, this should be adding an integration test similar to core's \Drupal\Tests\content_moderation\Kernel\ContentModerationStateResourceTest.

1. This fixed #16.1, but not #16.3?

2. +++ b/tests/src/Functional/InternalEntitiesTest.php
   @@ -66,16 +73,49 @@ class InternalEntitiesTest extends BrowserTestBase {
   +      $this->assertEquals(
   

   Nit: Can be assertSame().

3. +++ b/tests/src/Kernel/Controller/EntityResourceTest.php
   @@ -943,6 +943,10 @@ class EntityResourceTest extends JsonapiKernelTestBase {
   +   * @param array $relatable_resource_types
   

   Nit: Should get a more specific typehint than array.

Sorry, for #20.3, I meant ResourceTypeInterface[].

#16.3 Quickly, yes. We should not be adding invalid links to the entry point, that really breaks the point of HATEOAS. For that reason, I think we need to be testing that we are adding the proper links there. Maybe I'm missing your point though?

I think it'd be better to keep it as a functional test and instead add the jsonapi_entity_test module that you added in #2939705: Do not include internal resource types under `include`. Then we're not testing a mocked world, but the real world.

EDIT: Because that module includes an entity type that is internal and should therefore NOT show up in the entry point!

#2939705: Do not include internal resource types under `include` was reverted because it broke JSON API on Drupal 8.6, I reverted it, so this is now blocked again.

#23 looks great!

Let's hear what @e0ipso thinks.

This looks good to me 🎊.

+++ b/tests/src/Kernel/Controller/EntryPointTest.php
@@ -29,72 +27,24 @@ class EntryPointTest extends JsonapiKernelTestBase {
-   * Provides test cases to test entry point route generation.
...
-   * Ensures that internal resources are not present in the entry point.

Nit: Now that the tests have been collapsed to a single test, let's add these comments inline.

#2939705: Do not include internal resource types under `include` was recommitted. Having @e0ipso RTBC this is wonderful :) Now retesting…

That didn't work — patch no longer applies obviously.

This is a rebase automatically performed by git.

Green! Two unused statements and @e0ipso's remark in #26, but I can fix those upon commit :)

Except … those lines @e0ipso pointed out don't exist in the patch! So rather than waiting for Mateu to clarify, I'm deferring that to a follow-up if necessary. Because more likely than not, this was something on Mateu's local instance.

Ah, that's totally fine. What I meant was to add the intention of those comments in the new relevant parts of the code. I seem to recall a similar code was elsewhere. I don't think this deserves a single more second of consideration, let's move on to important things.

🎉❗️

Guess what, I failed to commit the commit interdiff from #29 😂😅

#32: happy to still do that though :) But the second comment you mention actually already exists at \Drupal\Tests\jsonapi\Functional\InternalEntitiesTest::testEntryPoint()!