Move deserialization from RequestHandler to JsonApiParamEnhancer

I'm less convinced by this change actually. Can you explain the rationale?

   if ($request->isMethodSafe(FALSE)) {
     return NULL;
   }

means that ::deserialize() returns early for GET requests.

we'll be able to move this back out of EntityResource into a route enhancer

Ah, that would make it worthwhile indeed!

IOW: this is a step on the path towards moving the mapping of a request body (through deserialization) to a controller parameter. Correct?

1. +++ b/jsonapi.services.yml
   @@ -149,11 +149,9 @@ services:
   -    public: false
   
   +++ b/tests/src/Kernel/Controller/EntityResourceTest.php
   @@ -163,14 +164,7 @@ class EntityResourceTest extends JsonapiKernelTestBase {
   +    $this->entityResource = $this->container->get('jsonapi.entity_resource');
   

   Do we really need this to be public?

   I think this test is the only reason we make it public? If so, let's just define a test-only service in the test's setUp() that is a public alias of this private service. That will achieve the same!

2. +++ b/tests/src/Kernel/Controller/EntityResourceTest.php
   @@ -551,8 +538,9 @@ class EntityResourceTest extends JsonapiKernelTestBase {
              'type' => 'article',
   -          'title' => 'PATCHED',
   -          'field_relationships' => [['target_id' => 1]],
   +          'attributes' => [
   +            'title' => 'PATCHED',
   +          ],
   

   Nice bugfix!

3. +++ b/tests/src/Kernel/Controller/EntityResourceTest.php
   @@ -666,52 +663,35 @@ class EntityResourceTest extends JsonapiKernelTestBase {
   -  public function testDeleteIndividualConfig() {
   

   Shouldn't this be moved to jsonapi_extras?

4. +++ b/tests/src/Kernel/Controller/EntityResourceTest.php
   @@ -725,14 +705,18 @@ class EntityResourceTest extends JsonapiKernelTestBase {
   -   * @dataProvider patchRelationshipProvider
       */
   -  public function testPatchRelationship($relationships) {
   +  public function testPatchRelationship() {
   
   @@ -749,70 +739,46 @@ class EntityResourceTest extends JsonapiKernelTestBase {
   -  public function patchRelationshipProvider() {
   -    return [
   -      // Replace relationships.
   -      [[['target_id' => 2], ['target_id' => 1]]],
   -      // Remove relationships.
   -      [[]],
   -    ];
   -  }
   

   Why this change? (It seems to be removing valuable test coverage?)

5. +++ b/tests/src/Kernel/Controller/EntityResourceTest.php
   @@ -749,70 +739,46 @@ class EntityResourceTest extends JsonapiKernelTestBase {
   -   * @dataProvider deleteRelationshipProvider
       */
   -  public function testDeleteRelationship($deleted_rels, $kept_rels) {
   +  public function testDeleteRelationship() {
   ...
   -  public function deleteRelationshipProvider() {
   -    return [
   -      // Remove one relationship.
   -      [[['target_id' => 1]], [['target_id' => 2]]],
   -      // Remove all relationships.
   -      [[['target_id' => 2], ['target_id' => 1]], []],
   -      // Remove no relationship.
   -      [[], [['target_id' => 1], ['target_id' => 2]]],
   -    ];
   -  }
   

   Why this change? (It seems to be removing valuable test coverage?)

1. +++ b/src/Controller/EntityResource.php
   @@ -963,4 +962,69 @@ class EntityResource {
   +  protected function deserialize(Request $request, ResourceType $resource_type) {
   
   +++ b/src/Controller/RequestHandler.php
   @@ -72,72 +57,10 @@ class RequestHandler {
   -  protected function deserialize(Request $request, ResourceType $resource_type) {
   

   👍 Confirmed these are identical before & after!

2. +++ b/tests/src/Kernel/Controller/EntityResourceTest.php
   @@ -719,94 +737,156 @@ class EntityResourceTest extends JsonapiKernelTestBase {
   -  public function testPatchRelationship($relationships) {
   +  public function testPatchRelationship() {
   ...
   -  public function patchRelationshipProvider() {
   ...
   +  public function testDeleteRelationship() {
   ...
   -  public function deleteRelationshipProvider() {
   

   You restored this test coverage, but it looks *very* different now. Is that truly necessary? It makes this patch much harder to review, and the interdiff twice as large.

I've just applied this patch plus #2987609-21: Rename the entity parameter from the entity type ID to 'entity' for all routes plus #2987610-10: Remove RequestHandler class and service and add EntityResource methods to each route definition, to see the end result.

1. Select tests seem to be passing 👍
2. 20 files changed, 431 insertions(+), 448 deletions(-) is not as exciting a diffstat as I'd hoped … 😞
3. And I've yet to see this, and am having some doubts:
   

   we'll be able to move this back out of EntityResource into a route enhancer

   Ah, that would make it worthwhile indeed!

   How can a route enhancer deserialize a request body? AFAIK this is not possible?

#15: Ahhhh, of course! Makes sense.
#16: Woah! 😮 Now that is an interdiff :D It does look like some other patch got entangled in there though.
#19: I first wanted to bring back the test coverage. But then I realized that the existing test coverage was literally just verifying that a specific normalizer would be called, and the logic in HEAD for JsonApiParamEnhancer was in fact already hardcoding a specific normalizer for a specific purpose. So the test coverage was not really testing anything. I agree that this test coverage has served its purpose (dating back to JSON API's early days, added in May 2016, commit 2a2b593!)

1. This needs to be rebased now that #2977659: Spec Compliance: POST|PATCH|DELETE on relationships should respect arity rules is in.

2. +++ b/jsonapi.services.yml
   @@ -104,12 +103,12 @@ services:
   -    arguments: ['@serializer.normalizer.filter.jsonapi', '@serializer.normalizer.sort.jsonapi', '@serializer.normalizer.offset_page.jsonapi']
   +    arguments: ['@jsonapi.serializer_do_not_use_removal_imminent']
   

   I wonder if this has a performance impact.

   In HEAD, we're instantiating 3 normalizer services during routing.

   With this, we're instantiating the entire JSON API serializer and hence all JSON API normalizers … but also core's serializer and all of its normalizers.

   From 3 services in the critical routing path to many dozens.

   That means this approach definitely needs profiling.

3. +++ b/jsonapi.services.yml
   @@ -104,12 +103,12 @@ services:
   -    arguments: ['@router.no_access_checks', '@url_generator']
   +    arguments: [null, '@url_generator']
   

   🙃😵

Rather than let this get blocked on that case and profiling, let's just lazy load the serializer under all circumstances.

Pragmatism FTW! 👍

1. +++ b/jsonapi.services.yml
   @@ -4,7 +4,6 @@ parameters:
   -    public: false
   

   I'd like to see this change removed from the patch. Is that still not possible?

2. +++ b/jsonapi.services.yml
   @@ -104,7 +103,8 @@ services:
   -    arguments: ['@serializer.normalizer.filter.jsonapi', '@serializer.normalizer.sort.jsonapi', '@serializer.normalizer.offset_page.jsonapi']
   

   What is again the problem with keeping these as-is?

   Or, put differently, why are these even normalizer services at all?

3. +++ b/src/Routing/JsonApiParamEnhancer.php
   @@ -3,47 +3,45 @@
   +      $this->serializer = $this->container->get('jsonapi.serializer_do_not_use_removal_imminent');
   

   Ah, this means it's not possible.

It was NW for #29.1, and kinda for #29.2.

#30.2: yes, you could theoretically argue that way that this is denormalization for the reasons you cite. But … only barely so. According to the same line of reasoning, \Drupal\Core\ParamConverter\ParamConverterInterface::convert() should be removed in favor of a denormalizer too. Because really, this is mapping information in a URL to a value object. The opposite direction (normalization) never occurs. I think it's pretty clear this is not using the Symfony serialization system fore the intended purpose.
Another reason you cite is to not load anything unless they're actually needed, but … that's only because we're using the serialization system at all. Usually, I'd argue that this means that new things can be added in the future (in this case: new parameters). But that's not even true, since \Drupal\jsonapi\Routing\JsonApiParamEnhancer::enhance requires a mapping from URL query arg name to denormalization class to be hardcoded anyway!

If we'd remove these normalizer services, we'd have less weirdness, and compared to #28, we'd be able to not inject normalizer services nor pass the entire container. Passing the entire container is always frowned upon.

Additionally, all this logic only makes sense (only runs) for JSON API collection resources anyway. Just grep for _route_params and _json_api_params. So rather than implementing \Drupal\Core\Routing\EnhancerInterface, which is executed at runtime (because the signature is public function enhance(array $defaults, Request $request);), it'd be much more logical to run this in \Drupal\jsonapi\Controller\EntityResource::getCollection() instead. That's the only place where it's being used. And hence it's the only place that should be slowed down by this.

…

But doing all that would be a much bigger refactoring. It would remove the bulk of JsonApiParamEnhancer::enhance(). It'd remove what that was originally designed for (i.e. "the JSON API params", meaning filter, sort and page), but the deserialization of the request body would still be left to be done. So the serialzier would still need to be injected.

So, rather than insisting things be done even better, I think this patch is already better than what's in HEAD. There's a net negative diffstat. And it directly paves the path for #2987610: Remove RequestHandler class and service and add EntityResource methods to each route definition, which will result in the RequestHandler class being deleted. Core REST will be handling its request body deserialization in its RequestHandler class, JSON API will be handling it in a param enhancer. Neither is perfect.

My chief concern is that this may get in the way of #2958554: Allow creation of file entities from binary data via JSON API requests, which is IMHO far more important than this because it brings an important new capability to JSON API. In core REST, this required changes to RequestHandler. But #2958554 is doing stuff with an alternative RequestHandler, just like core REST. So I think that before this can be committed, we need to land #2958554: Allow creation of file entities from binary data via JSON API requests (ideal case) or we need to be very certain that removing JSON API's RequestHandler won't get in the way. Hence keeping at Needs review.

Per #28.

I wrote in #32:

My chief concern is that this may get in the way of #2958554: Allow creation of file entities from binary data via JSON API requests, which is IMHO far more important than this because it brings an important new capability to JSON API. In core REST, this required changes to RequestHandler. But #2958554 is doing stuff with an alternative RequestHandler, just like core REST. So I think that before this can be committed, we need to land #2958554: Allow creation of file entities from binary data via JSON API requests (ideal case) or we need to be very certain that removing JSON API's RequestHandler won't get in the way. Hence keeping at Needs review.

@gabesullice wrote in #33:

I'm very certain that it won't.

But because it's not trivial to see, I wanted to answer this by proving it with passing tests: #2958554-80: Allow creation of file entities from binary data via JSON API requests is #2958554 + this patch + #2987610: Remove RequestHandler class and service and add EntityResource methods to each route definition. It passes tests. Therefore there cannot be any doubt in anybody's mind that this is safe to do. Hence: RTBC, and committing :)

#2987610: Remove RequestHandler class and service and add EntityResource methods to each route definition unblocked!