Refactor the JSON:API FieldResolver to use a resource type instead of an entity type ID and bundle ID pair

So weird, I can't see comment #7.

+++ b/core/modules/jsonapi/src/Context/FieldResolver.php
@@ -251,10 +251,8 @@ public static function resolveInternalIncludePath(ResourceType $resource_type, a
-   * @param string $entity_type_id
-   *   The type of the entity for which to resolve the field name.
-   * @param string $bundle
-   *   The bundle of the entity for which to resolve the field name.
+   * @param \Drupal\jsonapi\ResourceType\ResourceType $resource_type
+   *   The JSON:API resource type from which to resolve the field name.

➕This makes the method more cohesive with the rest of the code to abstract away the concepts of entity types and bundles to just resource types and resource objects.

This looks good. It's a nice little refactor with real world impact. I'm good with the code as is.

I'll let @mglaman RTBC this since he'll be using it in the jsonapi_cross_bundles module.

Per my thoughts in #9, pushing the RTBC button :).

1. The patch in #7 does not apply to Drupal 8.8. Even if this is cherry-picked to Drupal 8.7, it'll first need to go into Drupal 8.8. Due to #3059090: Deprecate \Drupal\Tests\PhpunitCompatibilityTrait::setExpectedException() there is a need for two different patches.

   Marking Needs work for this.

2. +++ b/core/modules/jsonapi/src/Context/FieldResolver.php
   @@ -251,10 +251,8 @@ public static function resolveInternalIncludePath(ResourceType $resource_type, a
   +   *   The JSON:API resource type from which to resolve the field name.
   

   🙈 It was "for", now it is "from". I'll let a native speaker decide :) Curious to learn what's correct!

3. +++ b/core/modules/jsonapi/src/Context/FieldResolver.php
   @@ -263,9 +261,8 @@ public static function resolveInternalIncludePath(ResourceType $resource_type, a
   -    $resource_type = $this->resourceTypeRepository->get($entity_type_id, $bundle);
   

   🥳 Nice, this is a small performance improvement.

4. +++ b/core/modules/jsonapi/tests/src/Kernel/Query/FilterTest.php
   @@ -76,7 +84,7 @@ public function setUp() {
   -    $resource_type = new ResourceType('node', 'painting', NULL);
   +    $resource_type = $this->resourceTypeRepository->get('node', 'painting');
   

   🤔This is more complicated than before. Why do we need to change this? Why was the field resolver previously satisfied, but not anymore?
   👍 Ahh, because the field resolver was itself loading the resource type from the repository again, where it did contain all the necessary information rather than only this thin, incomplete stub of it.

I forgot to say: I love the simplification here! 🤩

I have done this patch on Drupal 8.8

RTBC++ :)

+++ b/core/modules/jsonapi/src/Context/FieldResolver.php
@@ -263,9 +261,8 @@ public static function resolveInternalIncludePath(ResourceType $resource_type, a
-  public function resolveInternalEntityQueryPath($entity_type_id, $bundle, $external_field_name) {
+  public function resolveInternalEntityQueryPath(ResourceType $resource_type, $external_field_name) {
     $cacheability = (new CacheableMetadata())->addCacheContexts(['url.query_args:filter', 'url.query_args:sort']);
-    $resource_type = $this->resourceTypeRepository->get($entity_type_id, $bundle);

should we be providing a BC layer here.

Yes its marked internal but this is a hard-break that could cause issues for people during the minor update.

We can do this in a BC fashion using func_get_args and instanceof checks, and trigger a deprecation error if the old signature was called

Everything about JSON:API in every release has been creating breaking changes for anyone daring to touch it's PHP "API". As an outsider who has been fiddling for a while, it has been a constant following of BC as I touched @internal classes and methods. I don't think we need to add backward compatibility layer code debt.

Agreed with #19 and #20 🙂

FWIW, I'm not a huge fan of adding BC just for the sake of it but I think in this case ignoring it won't hurt.

As per http://grep.xnddx.ru/search?text=Drupal\jsonapi\Context\FieldResolver and http://grep.xnddx.ru/search?text=resolveInternalEntityQueryPath, the only other module extending this class or using this function is https://www.drupal.org/project/jsonapi_cross_bundles which is not even overriding this method and maintained by API first team anyway.

Everything about JSON:API in every release has been creating breaking changes for anyone daring to touch it's PHP "API".

This exact point goes against module being stable in core. As contrib maintainer myself, I haven't extended JSON:API to provide new functionality but I do maintain DER and changes in entity system API only breaks tests for that module and not the actual functionality and wherever it breaks the functionality entity system API maintainers are generous enough to provide BC. @mglaman my point here is that you shouldn't have to do these changes anymore because JSON:API has been added as a stable module in the core.

I do understand \Drupal\jsonapi\Context\FieldResolver is marked as an internal class but given we have a case in contrib extending this class and in future, there are chances to have more modules like this popping up we should consider removing internal here. This can be a follow-up discussion and in no way holds this issue back.

A module being stable does not mean every single implementation detail is public API surface. Modules that choose to override internal APIs consciously do that and accept the responsibility of having to update. This is not special, this is not Drupal-specific. Every operating system has this concept.

In fact, this issue is a stepping stone towards the JSON:API module providing a public PHP API (see #3032787: [META] Start creating the public PHP API of the JSON:API module), because it removes some coupling of JSON:API to entities. We need JSON:API to be able to expose non-entity data eventually, and this helps with that.

For the BC question

This looks like a case where it would be easy to provide bc via func_get_args(), at the cost of not introducing the type hint. However I think @jibran in #22 is saying that the only contrib module that might have overridden or called this method isn't doing so. If that's the case we could probably skip it but it does seem easy to add compared to the other two issues.

Committed 12b4761 and pushed to 8.8.x. Thanks!

+++ b/core/modules/jsonapi/src/Context/FieldResolver.php
@@ -263,9 +261,21 @@ public static function resolveInternalIncludePath(ResourceType $resource_type, a
+    $function_args = func_get_args();
+    // @todo Remove this conditional block in drupal:9.0.0 and add a type hint
+    // to the first argument of this method.
+    // @see https://www.drupal.org/project/drupal/issues/3078045
+    if (count($function_args) === 3) {
+      @trigger_error('Passing the entity type ID and bundle to ' . __METHOD__ . ' is deprecated in drupal:8.8.0 and will throw a fatal error in drupal:9.0.0. Pass a JSON:API resource type instead. See https://www.drupal.org/node/3078036', E_USER_DEPRECATED);
+      list($entity_type_id, $bundle, $external_field_name) = $function_args;
+      $resource_type = $this->resourceTypeRepository->get($entity_type_id, $bundle);
+    }

Discussed the lack of test with @catch we agreed that this is okay because the new code path is tested and since this method is @internal BC is provided on a best effort basis.

diff --git a/core/modules/jsonapi/src/Context/FieldResolver.php b/core/modules/jsonapi/src/Context/FieldResolver.php
index d8b640441c..ef58a086dd 100644
--- a/core/modules/jsonapi/src/Context/FieldResolver.php
+++ b/core/modules/jsonapi/src/Context/FieldResolver.php
@@ -271,7 +271,7 @@ public function resolveInternalEntityQueryPath($resource_type, $external_field_n
       list($entity_type_id, $bundle, $external_field_name) = $function_args;
       $resource_type = $this->resourceTypeRepository->get($entity_type_id, $bundle);
     }
-    else if (!$resource_type instanceof ResourceType) {
+    elseif (!$resource_type instanceof ResourceType) {
       throw new \InvalidArgumentException("The first argument to " . __METHOD__ . " should be an instance of \Drupal\jsonapi\ResourceType\ResourceType, " . gettype($resource_type) . " given.");
     }
 

Fixed the coding standard on commit.