Make JSON API Extras 2.x (next release: 2.10) compatible with JSON API 2.0-beta2

If one installs jsonapi 1.23 and jsonapi_extras 2.9, then updates from 1.23 to 2.0-beta2 and runs core/rebuild.php, one sees:

Warning: Declaration of Drupal\jsonapi_extras\ResourceType\ConfigurableResourceTypeRepository::isMutableResourceType(Drupal\Core\Entity\EntityTypeInterface $entity_type) should be compatible with Drupal\jsonapi\ResourceType\ResourceTypeRepository::isMutableResourceType(Drupal\Core\Entity\EntityTypeInterface $entity_type, $bundle) in /Users/wim.leers/Work/d8/modules/jsonapi_extras/src/ResourceType/ConfigurableResourceTypeRepository.php on line 17

Fatal error: Uncaught TypeError: Argument 4 passed to Drupal\jsonapi_extras\ResourceType\ConfigurableResourceTypeRepository::__construct() must implement interface Drupal\Core\Entity\EntityRepositoryInterface, instance of Drupal\Core\Cache\MemoryBackend given, called in /Users/wim.leers/Work/d8/core/lib/Drupal/Component/DependencyInjection/Container.php on line 286 and defined in /Users/wim.leers/Work/d8/modules/jsonapi_extras/src/ResourceType/ConfigurableResourceTypeRepository.php:76 Stack trace: #0 /Users/wim.leers/Work/d8/core/lib/Drupal/Component/DependencyInjection/Container.php(286): Drupal\jsonapi_extras\ResourceType\ConfigurableResourceTypeRepository->__construct(Object(Drupal\Core\Entity\EntityTypeManager), Object(Drupal\Core\Entity\EntityTypeBundleInfo), Object(Drupal\Core\Entity\EntityFieldManager), Object(Drupal\Core\Cache\MemoryBackend), Object(Drupal\Core\Entity\EntityRepository), Object(Drupal\jsonapi_extras\Plugin\ResourceFieldEnhancerManager), Object(Drupal\Core\Config\ConfigFactory)) #1 /Users/wim.leers/ in /Users/wim.leers/Work/d8/modules/jsonapi_extras/src/ResourceType/ConfigurableResourceTypeRepository.php on line 76

Let's look at that warning first. #2995111: shouldBeInternalResourceType et al. should receive the resource type, not the entity type added a new $bundle parameter to isMutableResourceType() (specifically for jsonapi_extras!), but jsonapi_extras was not updated accordingly in #2992557: Inject additional services manually to ConfigurableResourceType.

Easy fix. Compatible with both jsonapi 1.x & 2.x.

Now we're down to

Fatal error: Uncaught TypeError: Argument 4 passed to Drupal\jsonapi_extras\ResourceType\ConfigurableResourceTypeRepository::__construct() must implement interface Drupal\Core\Entity\EntityRepositoryInterface, instance of Drupal\Core\Cache\MemoryBackend given, called in /Users/wim.leers/Work/d8/core/lib/Drupal/Component/DependencyInjection/Container.php on line 286 and defined in /Users/wim.leers/Work/d8/modules/jsonapi_extras/src/ResourceType/ConfigurableResourceTypeRepository.php:76 Stack trace: #0 /Users/wim.leers/Work/d8/core/lib/Drupal/Component/DependencyInjection/Container.php(286): Drupal\jsonapi_extras\ResourceType\ConfigurableResourceTypeRepository->__construct(Object(Drupal\Core\Entity\EntityTypeManager), Object(Drupal\Core\Entity\EntityTypeBundleInfo), Object(Drupal\Core\Entity\EntityFieldManager), Object(Drupal\Core\Cache\MemoryBackend), Object(Drupal\Core\Entity\EntityRepository), Object(Drupal\jsonapi_extras\Plugin\ResourceFieldEnhancerManager), Object(Drupal\Core\Config\ConfigFactory)) #1 /Users/wim.leers/ in /Users/wim.leers/Work/d8/modules/jsonapi_extras/src/ResourceType/ConfigurableResourceTypeRepository.php on line 76

When running /core/rebuild.php.

This one is a bit trickier. #2984886: Trigger route rebuild when new bundles/fields are added/removed added a new parameter to ResourceTypeRepository in jsonapi's 2.x branch.

jsonapi_extras' ConfigurableResourceTypeRepository, which decorates that same class, must therefore also pass this parameter.

Fortunately, jsonapi 1.x only expects 3 parameters, and will continue to do so. We have two possible solutions here:

1. Avoid the need to keep the constructors in sync by having the additional services that jsonapi_extras' override ConfigurableResourceTypeRepository needs be injected via setters rather than via the constructor.
2. Or use Symfony's service decorator feature. This does mean ConfigurableResourceTypeRepository will need to implement all methods required by the interface. It already implements one, but it would then have to start implementing the other two too. This would also require injecting all services that ConfigurableResourceTypeRepository uses, which includes many of those in ResourceTypeRepository. But worse, it'd require mapping every ResourceType object to a ConfigurableResourceType object.

The first option is vastly simpler, even though the second may arguably be better in the long term. I also tried the second approach, but it'd require massive code changes. Perhaps we want to do it in a follow-up issue. But I think we prefer the minimal set of changes to get to a cross-compatible jsonapi_extras module? :)

So, here's that first approach.

Still passing on JSON API 1.x ✅

On 2.x, it's now failing with this output:

Notice: Undefined property: Drupal\jsonapi_extras\ResourceType\ConfigurableResourceTypeRepository::$all in /Users/wim.leers/Work/d8/modules/jsonapi_extras/src/ResourceType/ConfigurableResourceTypeRepository.php on line 86

Fatal error: Uncaught Error: Call to a member function get() on null in /Users/wim.leers/Work/d8/modules/jsonapi_extras/src/ResourceType/ConfigurableResourceType.php:121 Stack trace: #0 /Users/wim.leers/Work/d8/modules/jsonapi_extras/src/ResourceType/ConfigurableResourceType.php(216): Drupal\jsonapi_extras\ResourceType\ConfigurableResourceType->getResourceFieldConfiguration('type', 'fieldName') #1 /Users/wim.leers/Work/d8/modules/jsonapi_extras/src/ResourceType/ConfigurableResourceType.php(41): Drupal\jsonapi_extras\ResourceType\ConfigurableResourceType->translateFieldName('type', 'fieldName', 'publicName') #2 /Users/wim.leers/Work/d8/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php(316): Drupal\jsonapi_extras\ResourceType\ConfigurableResourceType->getPublicName('type') #3 /Users/wim.leers/Work/d8/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php(117): Drupal\jsonapi\ResourceType\ResourceTypeRepository->calculateRelatableResourceTypes(Object(Drupal\jsonapi_extras\ResourceType\ConfigurableResourceType) in /Users/wim.leers/Work/d8/modules/jsonapi_extras/src/ResourceType/ConfigurableResourceType.php on line 121

#2977669: Spec Compliance: some entity types have an "id", "type" or "uuid" field, this violates the spec causes this failure, because it made this change:

diff --git a/src/ResourceType/ResourceTypeRepository.php b/src/ResourceType/ResourceTypeRepository.php
index 7b3f3eb..9a17ddd 100644
--- a/src/ResourceType/ResourceTypeRepository.php
+++ b/src/ResourceType/ResourceTypeRepository.php
@@ -192,11 +294,17 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
         $resource_type->getBundle()
       );
 
-      return array_map(function ($field_definition) {
+      $relatable_internal = array_map(function ($field_definition) {
         return $this->getRelatableResourceTypesFromFieldDefinition($field_definition);
       }, array_filter($field_definitions, function ($field_definition) {
         return $this->isReferenceFieldDefinition($field_definition);
       }));
+
+      $relatable_public = [];
+      foreach ($relatable_internal as $internal_field_name => $value) {
+        $relatable_public[$resource_type->getPublicName($internal_field_name)] = $value;
+      }
+      return $relatable_public;
     }
     return [];
   }

That was added in #2977669-69: Spec Compliance: some entity types have an "id", "type" or "uuid" field, this violates the spec. This was a bugfix to ensure that relatable resource types are discovered using their public name (the "alias") rather than the internal name. Without this, JSON API Extras also didn't work correctly!

This means that \Drupal\jsonapi_extras\ResourceType\ConfigurableResourceType::getPublicName() gets called earlier than it did before. If we look at \Drupal\jsonapi_extras\ResourceType\ConfigurableResourceTypeRepository::all(), then:

  public function all() {
    if (!$this->all) {
      $all = parent::all(); <--- NOW IT IS CALLED HERE
      array_walk($all, [$this, 'injectAdditionalServicesToResourceType']); <--- PREVIOUSLY IT WAS CALLED HERE
      $this->all = $all;
    }
    return $this->all;
  }

Because of this, ConfigurableResourceType objects do not yet have the additional services injected and hence fail.

Of course, ideally jsonapi_extras would use the built-in aliasing capabilities provided by jsonapi since #2977669: Spec Compliance: some entity types have an "id", "type" or "uuid" field, this violates the spec rather than layering on its own, as proposed in #2986408: Use aliasing capabilities provided by JSON API 2.x, but doing that would not be compatible with JSON API 1.x.

Solution: still TBD.

So, let's say we comment out that early invocation of ConfigurableResourceType::getPublicName:

diff --git a/src/ResourceType/ResourceTypeRepository.php b/src/ResourceType/ResourceTypeRepository.php
index 359523f..8ee7aa1 100644
--- a/src/ResourceType/ResourceTypeRepository.php
+++ b/src/ResourceType/ResourceTypeRepository.php
@@ -313,7 +313,7 @@ class ResourceTypeRepository implements ResourceTypeRepositoryInterface {
 
       $relatable_public = [];
       foreach ($relatable_internal as $internal_field_name => $value) {
-        $relatable_public[$resource_type->getPublicName($internal_field_name)] = $value;
+//        $relatable_public[$resource_type->getPublicName($internal_field_name)] = $value;
       }
       return $relatable_public;
     }

Where do we get then?

Well … something that is literally impossible to solve:

The website encountered an unexpected error. Please try again later.
LogicException: JSON API does not allow adding more normalizers! in Drupal\jsonapi\Serializer\Serializer->__construct() (line 34 of modules/jsonapi/src/Serializer/Serializer.php).
Drupal\jsonapi\Serializer\Serializer->__construct(Array, Array) (Line: 266)
Drupal\Component\DependencyInjection\Container->createService(Array, 'jsonapi.serializer_do_not_use_removal_imminent') (Line: 171)
Drupal\Component\DependencyInjection\Container->get('jsonapi.serializer_do_not_use_removal_imminent', 1) (Line: 480)
Drupal\Component\DependencyInjection\Container->resolveServicesAndParameters(Array) (Line: 230)
Drupal\Component\DependencyInjection\Container->createService(Array, 'jsonapi.exception_subscriber') (Line: 171)
Drupal\Component\DependencyInjection\Container->get('jsonapi.exception_subscriber') (Line: 105)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.exception', Object) (Line: 228)
Symfony\Component\HttpKernel\HttpKernel->handleException(Object, Object, 1) (Line: 79)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 129)
Drupal\cdn\StackMiddleware\DuplicateContentPreventionMiddleware->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 669)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

You see, #2936754: Avoid using the Serialization component for JSON API specific tasks explicitly started disallowing custom normalizers:

  public function __construct(array $normalizers = [], array $encoders = []) {
    foreach ($normalizers as $normalizer) {
      if (strpos(get_class($normalizer), 'Drupal\jsonapi\Normalizer') !== 0) {
        throw new \LogicException('JSON API does not allow adding more normalizers!');
      }
    }
    parent::__construct($normalizers, $encoders);
  }

I don't see any way we can fix that. @e0ipso +1'd this at #2936754-26: Avoid using the Serialization component for JSON API specific tasks, but in #32 of that issue, he writes:

This issue purposely breaks JSON API Extras for now. However, we will need to fix this once we come up with the best pattern.

I … am not sure what this means. @e0ipso, can you explain what you meant?

This … doesn't sound very reliable and definitely not easy to understand/maintain. I do very much appreciate that you're trying to help to move this along though :)

Another idea is to provide a class map in composer.json that will allow us to have classes namespaced by \Drupal\jsonapi\Normalizers inside the JSON API Extras module.

WOAH

I'm not sure we can have a creative/clever solution without being astonishing.

😂

I was personally looking into how we could do the work we want to do in a different phase. I was looking at \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber::renderResponseBody(), whrere we're dealing with a JsonApiDocumentTopLevelNormalizerValue instance. That feels like it could be the place where we can reliably do this. I don't know how yet exactly though.

I think my proposal is a stop-gap and we can solve this in the right way when we refactor or replace the JSON API serialization system.

It is!

But before embarking on that path I'd just want all three of us to have spent a day or two pondering the problem. See my suggestion in #10 for a different direction (although unlike your direction, it doesn't have a concrete working proposal yet).

#14:

In essence, your suggestion will force JSON API Extras to work only on the "rasterized" array representing the entire JSON API document.

That is one possibility. The other possibility is that jsonapi_extras runs its own response subscriber before jsonapi's, so it can modify the JsonApiDocumentTopLevel object before it gets normalized into a JsonApiDocumentTopLevelNormalizerValue object by \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber::renderResponseBody(). That seems simpler/more reliable for jsonapi_extras.

#16:

I think this makes it clear that we want to have a dedicated branch jsonapi_extras-3.x for compatibility with jsonapi-2.x. Unless anyone disagrees we can move forward with that assumption. @Wim Leers thanks for finding the exact thing that makes us do the thing we suspected we'd have to do.

I don't think it's a hard requirement, but I do think it'll make jsonapi_extras a lot easier to maintain. Because #2977669: Spec Compliance: some entity types have an "id", "type" or "uuid" field, this violates the spec effectively moved a significant portion of the infrastructure that jsonapi_extras needs into jsonapi itself, which means it has to do far less work!

So yes, I personally do think it's worth creating a new branch of jsonapi_extras, because it will make it a lot easier to maintain.

Been working on this all day. Update later tonight, and if not tonight, then tomorrow.

I had something workable for solving #5's first problem earlier today, but it was 2.x-only. I could make it work with 1.x too, but then we'd need 1.x to change, which then still means you need to update more things, which still means no nice smooth update path.

I now managed to get something to work that allows jsonapi_extras 2.x to continue to work with any existing 1.x site, and start working with 2.x. It will require a small change in JSON API 2.x. Super small. Super trivial. And it'll allow JSON API Extras to become simpler in the future, when it drops 1.x support.

I did not yet solve the normalizer problem though. Still, exciting to see that I might be able to not require a new branch!

Need to call it a day now though. More tomorrow!

We can solve the first problem described in #5 by making it possible for ConfigurableResourceTypeRepository (JSON API Extras' override) to create the "correct" ResourceType value objects the first time, rather than relying on decoration first.
To do that, we also need to provide a $field_mapping parameter, which the big hunk in #2986408-2: Use aliasing capabilities provided by JSON API 2.x gets us.

Now, the early call to $resource_type->getPublicName() no longer causes a problem :)

Best of all: I can make this work on both 1.x and 2.x if we either apply the attached jsonapi_* patches to the respective branches, or if we keep the 2.x detection logic in this interdiff. Keeping the 2.x detection logic is preferable because it significantly simplifies the update path for sites on JSON API 1.x today: they can first update JSON API Extras and then update JSON API from 1.x to 2.x at any time of their choosing.

Compatible with 1.x and 2.x.

New problem (on 2.x):

The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">LogicException</em>: The database connection is not serializable. This probably means you are serializing an object that has an indirect reference to the database connection. Adjust your code so that is not necessary. Alternatively, look at DependencySerializationTrait as a temporary solution. in <em class="placeholder">Drupal\Core\Database\Connection-&gt;__sleep()</em> (line <em class="placeholder">1481</em> of <em class="placeholder">core/lib/Drupal/Core/Database/Connection.php</em>). <pre class="backtrace">serialize(Array) (Line: 109)
Drupal\Core\Cache\MemoryBackend-&gt;set(&#039;jsonapi.resource_types&#039;, Array, -1, Array) (Line: 110)
Drupal\jsonapi\ResourceType\ResourceTypeRepository-&gt;all() (Line: 112)
Drupal\jsonapi\Routing\Routes-&gt;routes()
call_user_func(Array) (Line: 146)
Drupal\Core\Routing\RouteBuilder-&gt;rebuild() (Line: 83)
Drupal\Core\ProxyClass\Routing\RouteBuilder-&gt;rebuild() (Line: 1158)
drupal_flush_all_caches() (Line: 192)
Drupal\system\Form\PerformanceForm-&gt;submitCacheClear(Array, Object)
call_user_func_array(Array, Array) (Line: 111)
Drupal\Core\Form\FormSubmitter-&gt;executeSubmitHandlers(Array, Object) (Line: 51)
Drupal\Core\Form\FormSubmitter-&gt;doSubmitForm(Array, Object) (Line: 589)
Drupal\Core\Form\FormBuilder-&gt;processForm(&#039;system_performance_settings&#039;, Array, Object) (Line: 318)
Drupal\Core\Form\FormBuilder-&gt;buildForm(&#039;system_performance_settings&#039;, Object) (Line: 93)
Drupal\Core\Controller\FormController-&gt;getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;Drupal\Core\EventSubscriber\{closure}() (Line: 582)
Drupal\Core\Render\Renderer-&gt;executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber-&gt;Drupal\Core\EventSubscriber\{closure}() (Line: 151)
Symfony\Component\HttpKernel\HttpKernel-&gt;handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel-&gt;handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session-&gt;handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle-&gt;handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware-&gt;handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware-&gt;handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel-&gt;handle(Object, 1, 1) (Line: 669)
Drupal\Core\DrupalKernel-&gt;handle(Object) (Line: 19)
</pre>

Because as of #2995111: shouldBeInternalResourceType et al. should receive the resource type, not the entity type, the (Configurable)ResourceType objects are being stored in a static cache. Doing this results in the services injected into ConfigurableResourceType to be serialized as well. (Because it's not a true value object anymore due to those services.) use DependencySerializationTrait; to the rescue!

Several methods can be removed when only 2.x is supported by jsonapi_extras. But that's explicitly not the goal of this issue!

Still, while we're working on this, we should add @todos to mark those things for removal to make things simpler in the future. It also gives us a sense of how much simpler things become once jsonapi_extras only supports jsonapi 2.x.

One method already was 2.x-only, and is no longer necessary in a world where there is a ::createResourcetype(). :) Slight simplification, yay!

My \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber::renderResponseBody()-based proposal for solving the remaining problem (normalizers) actually only solves half the problem: it only works for normalization, not denormalization.

So, I started to implement the work-around @gabesullice proposed in #6. One thing he didn't foresee is that it's actually not possible to remove the serializer.normalizer.field_item.jsonapi serializer, because serializer.normalizer.field_item.jsonapi_extras decorates it! And similarly, we need JSON API Extras' subclasses to pass all necessary parameters to the other two overridden normalizers, which means they need to be inherited to be able to work on both 1.x and 2.x. That means that rather than removing, we can make them private untagged services.

His other key prediction, about us somehow needing to make sure JSON API Extras' EntityNormalizer uses JSON API's serializer did come true though. I solved that by inheriting jsonapi's FieldItemList normalizer into the core's serializer, with a similarly high priority. This means it gets selected in lieu of core's ListNormalizer, and hence keeps things working fine.

Best of all: while this approach is necessary for jsonapi 2.x, it also works fine for 1.x. So we don't need conditional logic between 1.x and 2.x here!

Finally: since #2955615: Field properties are not being denormalized, the signature of the protected ::prepareInput() changed. It makes sense to not have warnings on 2.x, especially because it actually uses that additional data. There can be warnings on JSON API 1.x, but they're harmless.

Fix CS violations.

I need to run now. Sadly, with this patch still red.

I never ran these tests locally, I instead actually used JSON API, both 1.x and 2.x, with the following JSON API Extras customizations turned on:

• custom base path
• collection count enabled
• a field aliased

And it works!

It'd be great if somebody else could fix those last few failures. I'd LOVE to see this committed in the next few days and hopefully even today!

Forgot to mention: the jsonapi patches in #21 have a jsonapi issue: #3006270: Add ResourceTypeRepository::createResourceType() for easier JSON API Extras support and simpler code.

Could it be this simple? This appears to satisfy PHP on both JSON API 1.x and 2.x.

The remaining failures in EntityToJsonApiTest are legitimate. That test was introduced in [#29842455].

Ever since #2971040: PHP 7.1 Compatibility Warning, in the JSON API 2.x branch, the JsonApiDocumentTopLevel class was moved to be compatible with PHP 7.2. So we need to conditionally use the correct class. That's easy enough.

But on top of that, the constructor of the JsonApiDocumentTopLevel class evolved in the 2.x branch: it now requires more parameters.

After fixing that, turns out that the changes so far are somehow causing relationships to be normalized differently (incorrectly). It took some debugging, but then in hindsight it was obvious: increasing the service priorities in #25 cause \Drupal\jsonapi\Normalizer\FieldNormalizer to be picked instead of \Drupal\jsonapi\Normalizer\EntityReferenceFieldNormalizer for entity reference fields. I was able to fix that by also decorating serializer.normalizer.entity_reference_field.jsonapi. But then I had to also decorate serializer.normalizer.entity_reference_item.jsonapi. And more and more …

So, time to go back to @gabesullice's original proposal:

What we need to do, somehow, is make sure that instead of the standard serializer being set on the Extras normalizers, the JSON API serializer is set. I think we can do this by overriding the setSerializer() method. Hopefully, we can just turn it into a no-op method and set $this->serializer in the constructor. That might cause a loop in the DI container though, in which case we'll need to probably do something even uglier.

Now at least EntityToJsonApiTest is passing!

Now at least EntityToJsonApiTest is passing!

The test run confirms this.

Next up: a circular reference to be solved. Symfony's error message is (as usual) utterly useless: it says

Circular reference detected for service "serializer", path: "jsonapi.request_handler -> serializer".

But the circle is triggered like so: jsonapi.request_handler -> serializer -> serializer.normalizer.entity.jsonapi_extras -> jsonapi.serializer_do_not_use_removal_imminent -> serializer i.e. the serializer.normalizer.entity.jsonapi_extras is created by serializer but also has that as one of its dependencies.

So: time for something even uglier! Hopefully @gabesullice can do that while I sleep, otherwise I'll do it tomorrow.

On this again.

Fixing the CS violations is easy of course, but the circular dependency will require some more thought, tinkering and cursing.

✊

I managed to find a work-around :)

Now all tests should pass on jsonapi 1.x.

Fixed the failure in EntityToJsonApiTest.

And gahhhhhhhh 😭 JsonExtrasApiFunctionalTest::testWrite() is passing fine locally! Added debug output.

Fixed CS violations and refactored the decorator to be less sensitive to typos.

Looks like the refactoring of the decorator in #48 actually fixed it. That also explains why I couldn't reproduce it locally!

Removing debug output and fixing newly introduced CS violations.

🤞

D'oh.

FINALLY!

Now, let's see what happens when running this against JSON API 2.x. I think this is how I can get it to run against 2.x, let's see …

It's using 2.0-beta2, but we need it to install 54f560dea96c1cdd32165e23639cb19bc588134f, which includes #3006270: Add ResourceTypeRepository::createResourceType() for easier JSON API Extras support and simpler code.

AARGH JSON …

Rather than me being able to come back after lunch to a useful test run, now I need to wait again. 😡

Finally, a test run of JSON API Extras against JSON API 2.x!

1) Drupal\Tests\jsonapi_extras\Functional\JsonExtrasApiFunctionalTest::testRead
Exception: Notice: A non well formed numeric value encountered
Drupal\jsonapi_extras\Plugin\jsonapi\FieldEnhancer\DateTimeEnhancer->doUndoTransform()() (Line: 24)

Per https://www.drupal.org/node/2982678, we need to make the datetime enhancer a bit smarter.

Still same failure count, but failing later:

There was 1 error:

1) Drupal\Tests\jsonapi_extras\Functional\JsonExtrasApiFunctionalTest::testRead
Behat\Mink\Exception\ExpectationException: Current response status code is 200, but 404 expected.

/var/www/html/vendor/behat/mink/src/WebAssert.php:770
/var/www/html/vendor/behat/mink/src/WebAssert.php:130
/var/www/html/modules/contrib/jsonapi_extras/tests/src/Functional/JsonExtrasApiFunctionalTest.php:182

--

There was 1 failure:

1) Drupal\Tests\jsonapi_extras\Functional\JsonExtrasApiFunctionalTest::testWrite
Failed asserting that 'http://php-apache-jenkins-drupal8-contrib-patches-39924/subdirectory/api/articles/e54aeede-efee-49ca-aeb9-da8c7a6ade48' is identical to Array &0 (
    'href' => 'http://php-apache-jenkins-drupal8-contrib-patches-39924/subdirectory/api/articles/e54aeede-efee-49ca-aeb9-da8c7a6ade48'
).

That's happening here:

    // 13. Test a disabled related resource of single cardinality.
    $this->drupalGet('/api/taxonomy_term/tags/' . $this->tags[0]->uuid() . '/vid');
    $this->assertSession()->statusCodeEquals(404);
    $this->drupalGet('/api/taxonomy_term/tags/' . $this->tags[0]->uuid() . '/relationships/vid');
    $this->assertSession()->statusCodeEquals(404);

Which passes on 1.x but not on 2.x. Because #2953346: Define related/relationship routes per field, not dynamically (with route parameters that need validating) changed how this is handled: the related route is still not created in case all target resource types are internal, but the relationship route is created, meaning this now results in a 200 response with 'data' : null in the response document. The 2.x branch does this differently since #2953346, specifically since #2953346-26: Define related/relationship routes per field, not dynamically (with route parameters that need validating). This change was not explicitly discussed. If we want to reconsider this, we can open a jsonapi issue. For now, I'm just adding a 2.x-specific test expectation.

Let's look at the other failures, those in Drupal\Tests\jsonapi_extras\Kernel\Controller\EntityResourceTest.

They're easy to fix by adding 2.x and 1.x logic.

Looks like I made a c/p error in #61.

Last failure!

There was 1 error:

1) Drupal\Tests\jsonapi_extras\Functional\JsonExtrasApiFunctionalTest::testWrite
Undefined index: data

/var/www/html/modules/contrib/jsonapi_extras/tests/src/Functional/JsonExtrasApiFunctionalTest.php:299

ERRORS!
Tests: 2, Assertions: 55, Errors: 1.

If we look at the actual response, we get:

array(2) {
  ["errors"]=>
  array(1) {
    [0]=>
    array(4) {
      ["title"]=>
      string(11) "Bad Request"
      ["status"]=>
      int(400)
      ["detail"]=>
      string(88) "The provided type (taxonomy_term--tags) does not mach the destination resource types ()."
      ["links"]=>
      array(2) {
        ["via"]=>
        array(1) {
          ["href"]=>
          string(78) "http://d8/api/articles/071913b6-6d90-42be-845e-90d337bedc28/relationships/tags"
        }
        ["info"]=>
        array(1) {
          ["href"]=>
          string(64) "http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.1"
        }
      }
    }
  }
  ["jsonapi"]=>
  array(2) {
    ["version"]=>
    string(3) "1.0"
    ["meta"]=>
    array(1) {
      ["links"]=>
      array(1) {
        ["self"]=>
        array(1) {
          ["href"]=>
          string(30) "http://jsonapi.org/format/1.0/"
        }
      }
    }
  }
}

Note how there are zero destination resource types: ()!

Turns out this is because ever since JSON API 2.x gained a "field mapping" (for aliasing and disabling fields), we did not correctly update the relationship normalizer accordingly. AFAICT I introduced this in #2977669: Spec Compliance: some entity types have an "id", "type" or "uuid" field, this violates the spec. That issue resulted in far less probability of JSON API Extras breaking wrt disabling/aliasing of fields, because JSON API itself now does this. But in the case of JSON API, the fields we happen to alias automatically are all relationships to the corresponding bundle. And that just happens to be the one field that is in fact optional in a POST request. In other words: we only gained normalization test coverage in JSON API, not denormalization. The one case where it is now failing is in denormalization of relationships! i.e. when you POST or PATCH a RESOURCETYPE/relationships/RELATIONSHIPNAME route.
Since you cannot change a resource's bundle, and the bundle field is the only relationship that is automatically aliased by jsonapi, we'll need jsonapi to create a regression test for this.

Created #3007113: Follow-up for #2977669: denormalizing aliased relationships fails to fix #67.

#3007113: Follow-up for #2977669: denormalizing aliased relationships fails landed! 🤘

Reuploading #64's patch to retest.

#69 is green against jsonapi 2.x.
#51 was green against jsonapi 1.x, and so will this patch.

This is now in @gabesullice's hands for review, and especially in @e0ipso's!

1. ✔️ I agree with you in principle. But considering that that class was moved to a new namespace to be compatible with PHP 7.2, JSON API 2.x had no choice, and hence we don't have any choice here to support both. Once JSON API Extras stops supporting JSON API 2.x, all of this goes away :)
2. 👍 Good catch, fixed!
3. ❤️
4. ❤️
5. 👍 Good catch, fixed!
6. 🤔 You say "unnecessary", but I say "it PREVENTS silly typos and oversights". I introduced this in #48, and it immediately fixed failures in #47 that were the consequence of getting some details wrong. (The failure count reported here for those patches is inaccurate, look at the actual test output: #47 had one actual failure, #48 had zero actual failures but just printed debug output which somehow gets counted as 2 failures…)

   I don't feel strongly about this though — I'm happy to do what you describe. That's what I was originally doing in #47 too.

because then we don't comply with the interfaces without Reflection conjurations

Indeed.

have a preference for explicitness, but I see your point about typos.

I share that preference. But above anything else, I care about correctness and reliability. This is explicitly (without crazy hacks) relaying a call to another object. I think this is still equally explicit, at the cost of requiring to understand two relatively advanced PHP features: call_user_func_array() and func_get_args().

I'm OK with leaving it as is.

So then … what remains to be done for this to land? :)

I'm perfectly fine with you still preferring not using call_user_func_array() and func_get_args(). Your decision.

👍

🎉

Woot! Also great to see https://www.drupal.org/project/jsonapi_extras/releases/8.x-2.10 already released. Thank you, @e0ipso! :)

This issue was a blocker for the migration to the new version.

Exciting! :D

Note that you cannot use 2.0-beta2, you need the version after that (not yet released), or the latest dev snapshot. Because #3006270: Add ResourceTypeRepository::createResourceType() for easier JSON API Extras support and simpler code is needed.

the composer output says only JSON API 8.x-1/dev are compattable

Probably due to to "drupal/jsonapi": "^1.22", then.

@stevieb, could you open a new issue for this? Thanks!

Ah, right, good point! Updated the patch there: #2994885-12: JSONAPI Version Requirements prevent installation via Composer.