ResourceType should provide related ResourceTypes

@gabesullice thanks for creating this.

Adding blocker tag, since the IS says blocks #2937279: [META] Introduce concept of internal resource types.

Kicking off tests and not reviewing until @gabesullice says so.

Back to needs work as per #5.

WOAH 60K patch!

Most important question before I dive into a review (also because it contributes significantly to the patch size): why introduce an interface? We explicitly removed the interface for ResourceType before, because it communicates "you can swap this for your own implementation". It also means we have to commit to that interface, and have to keep supporting it.

I do see that you marked it @deprecated already. But not @internal

But is it even necessary?

👌

Much better! Almost 50% smaller patch, nice!

1. +++ b/src/ResourceType/ResourceType.php
   @@ -15,32 +22,64 @@ namespace Drupal\jsonapi\ResourceType;
   +   * @var \Drupal\Core\Entity\EntityTypeInterface|\Drupal\Core\Entity\FieldableEntityInterface
   

   These interfaces are for very different things. I think the latter is the wrong one?

2. +++ b/src/ResourceType/ResourceType.php
   @@ -145,21 +184,140 @@ class ResourceType {
   +   * The return value from this method is a multi-dimensional array keyed by the
   +   * public field name, where each value is an array of relatable resource
   +   * for that particular field. There may be duplicates across keys, but not
   +   * per key.
   +   *
   +   * @return array
   +   *   The relatable resource types, keyed by relationship field names.
   

   I'd expect the paragraph above the @return to be part of the docs under @return.

3. +++ b/src/ResourceType/ResourceType.php
   @@ -145,21 +184,140 @@ class ResourceType {
   +  public function getRelatableResourceTypes() {
   

   As a non-native speaker, I wonder if relatable is the correct adjective here?

   relatable |rəˈlādəb(ə)l|
   adjective
   1 able to be related to something else: the growth of the welfare state will be clearly relatable to the growth of democracy.
   2 enabling a person to feel that they can relate to someone or something: Mary-Kate's problems make her more relatable.
   

   I instantly think of the second meaning, but it's of course the first meaning. Sounds right.

4. +++ b/src/ResourceType/ResourceType.php
   @@ -145,21 +184,140 @@ class ResourceType {
   +      throw new \LogicException("calculateRelatableResourceTypes must be called before getting relatable resource types.");
   

   Why can't we call this automatically?

5. +++ b/src/ResourceType/ResourceType.php
   @@ -145,21 +184,140 @@ class ResourceType {
   +  public function getRelatableResourceTypesByField($field_name) {
   +    if (!isset($this->relatableResourceTypes)) {
   +      throw new \LogicException("calculateRelatableResourceTypes() must be called before getting relatable resource types.");
   +    }
   +    return isset($this->relatableResourceTypes[$field_name]) ?
   +      $this->relatableResourceTypes[$field_name] :
   +      [];
   +  }
   

   I'd expect this to just call getRelateableResourcetTypes() rather than duplicating some of the logic.

6. +++ b/src/ResourceType/ResourceType.php
   @@ -145,21 +184,140 @@ class ResourceType {
   +  public function calculateRelatableResourceTypes(ResourceTypeRepositoryInterface $resource_type_repository) {
   

   Per the above, IMHO this should be protected.

7. +++ b/tests/src/Kernel/Controller/EntityResourceTest.php
   @@ -872,8 +873,15 @@ class EntityResourceTest extends JsonapiKernelTestBase {
   +    $resource_type = $this->prophesize(ResourceType::class);
   +    $resource_type->getEntityTypeId()->willReturn($entity_type_id);
   +    $resource_type->getBundle()->willReturn($bundle);
   +    $resource_type->getTypeName()->willReturn($entity_type_id . '--' . $bundle);
   +    $resource_type->includeCount()->willReturn(TRUE);
   +    $resource_type->getInternalName(Argument::type("string"))->willReturnArgument(0);
   +
        return new EntityResource(
   -      new ResourceType($entity_type_id, $bundle, NULL),
   +      $resource_type->reveal(),
   

   This feels like a regression. ResourceType used to be pure value objects. Now they no longer are, so we need to resort to mocking.

   This is the consequence of injecting services into ResourceType.

   Have you considered turning a ResourceTypeRepository into a factory (or even introducing a separate factory), so that all logic can be contained there, and ResourceType can continue to be a value object?

   Then we can have a kernel test for the factory, rather than for the value object. All tests that merely consume ResourceType value objects could then remain far simpler.

   It would also keep the codebase easier to reason about.

Whew, I had no idea this issue would end up being such a time sink. I'm not surprised though, the issue of related resource types has eaten up a ton of brain power in every other issue that runs into it (see the FieldResolver). Hopefully this will set us up to solve those issues for good though, so I think it's worth really getting this right :)

+1 — and think of how much time getting this right (or at least better) will save us once this is in core!

I think we've cut the patch in half again!

You've been exponentially decreasing the size of the patch … could you do the same with Drupal core please? :P

OMG THIS BECAME 100X SIMPLER, love it now! I have only 3 nits. Then I will RTBC, and will assign to @e0ipso for final review + commit.

1. +++ b/src/ResourceType/ResourceType.php
   @@ -158,8 +158,56 @@ class ResourceType {
        $this->deserializationTargetClass = $deserialization_target_class;
   -
        $this->typeName = sprintf('%s--%s', $this->entityTypeId, $this->bundle);
   

   Supernit: unnecessary change.

2. +++ b/src/ResourceType/ResourceType.php
   @@ -158,8 +158,56 @@ class ResourceType {
   +   * Get all resource types with which the given field may have a relationship.
   +   *
   +   * Gets all the JSON API resource types that may be related by the given,
   +   * public field name.
   +   *
   

   I don't see much additional information in the second sentence here? Can we remove it? Or merge it into the @return docs?

3. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -104,4 +122,91 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +  public function calculateRelatableResourceTypes(ResourceType $resource_type) {
   

   Should be protected — and AFAICT you even meant it to be :)

This looks good to me. Some clarifications before committing.

1. +++ b/src/ResourceType/ResourceTypeRepository.php
   @@ -104,4 +122,91 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
   +    $entity_type_id = $item_definition->getSetting('target_type');
   +    $handler_settings = $item_definition->getSetting('handler_settings');
   

   Can we ensure that all relationship fields follow this pattern? Probably not. This will work with entity reference (and cousins).

   Should we document this contract somehow?

2. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -16,7 +16,13 @@ use Drupal\Tests\jsonapi\Kernel\JsonapiKernelTestBase;
   +    'user',
   

   Why this addition?

TBH, I think this is really begging for an interface in core that would do a lot of the work in this patch for us.

This is a really long winded of saying, maybe the best way to handle this is simply to do nothing and simply wait for the bug report/support request that says "my custom field type isn't handled as a relationship, why?"

Yes, the metadata infrastructure for "field that references other entities" is completely missing from core, we can't fix that. The only choice we have is to support entity_reference fields, until core provides richer metadata.

Given this is blocking a whole bunch of next steps, and @e0ipso already said in #24:

This looks good to me. Some clarifications before committing.

… implying that this was ready, definitely no major concerns, with just some clarifications desired, which @gabesullice posted in #25, I think it's time to get this in to unblock those next steps.

(Commit was delayed because git.drupal.org was down, I could not push!)

@gabesullice this one gets me excited! It makes me wonder if the resource type should also be aware of the attributes. Would that make the schema generation for Schemata super simple?