Port parameter based resource config from REST module

1. Thus, both of these issues are essentially solved/outdated:

   By turning the resource type and target serialization classes into route parameters, we can completely removes every dependency on CurrentContext.

   These statements sound like music to my ears!

2. The same changes will make it possible to purge access to the route object and the current route match (except in the enhancers, where they still make some sense).

   This I don't understand.

3. The patch is not yet updating \Drupal\jsonapi\EntityToJsonApi.
4. 139 insertions, 480 deletions

   !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

   This is clear proof this is a vast simplification.

5. I think it'd be good if the proposed resolution listed every major code change: every class removal/addition and why. That allows reviewers to get a better overview of this big set of changes.
6. I don't immediately see anything that could be moved to a separate patch. Do you?

This will definitely need sign-off from @e0ipso.

Initial patch review:

1. +++ b/src/Controller/RequestHandler.php
   @@ -37,44 +35,43 @@ class RequestHandler implements ContainerAwareInterface, ContainerInjectionInter
   -  public function handle(RouteMatchInterface $route_match, Request $request) {
   +  public function handle(Request $request, ResourceType $resource_type) {
        $method = strtolower($request->getMethod());
   -    $route = $route_match->getRouteObject();
    
        // Deserialize incoming data if available.
   -    /* @var \Symfony\Component\Serializer\SerializerInterface $serializer */
   -    $serializer = $this->container->get('jsonapi.serializer_do_not_use_removal_imminent');
   -    /* @var \Drupal\jsonapi\Context\CurrentContext $current_context */
   -    $current_context = $this->container->get('jsonapi.current_context');
   -    $current_context->reset();
   -    $unserialized = $this->deserializeBody($request, $serializer, $route->getOption('serialization_class'), $current_context);
   +    $unserialized = $this->deserializeBody(
   +      $request,
   +      $this->container->get('jsonapi.serializer_do_not_use_removal_imminent'),
   +      $resource_type
   +    );
   
   @@ -102,25 +99,22 @@ class RequestHandler implements ContainerAwareInterface, ContainerInjectionInter
   -  public function deserializeBody(Request $request, SerializerInterface $serializer, $serialization_class, CurrentContext $current_context) {
   +  public function deserializeBody(Request $request, SerializerInterface $serializer, ResourceType $resource_type) {
        $received = $request->getContent();
        if (empty($received) || $request->isMethodCacheable()) {
          return NULL;
        }
   -    $resource_type = $current_context->getResourceType();
        $field_related = $resource_type->getInternalName($request->get('related'));
        try {
   -      return $serializer->deserialize($received, $serialization_class, 'api_json', [
   +      return $serializer->deserialize($received, $request->get('serialization_class'), 'api_json', [
            'related' => $field_related,
   -        'target_entity' => $request->get($current_context->getResourceType()->getEntityTypeId()),
   +        'target_entity' => $request->get($resource_type->getEntityTypeId()),
   

   😍

2. +++ b/src/ParamConverter/ResourceTypeConverter.php
   @@ -0,0 +1,47 @@
   +class ResourceTypeConverter implements ParamConverterInterface {
   

   I love how simple this is!

3. +++ b/src/Routing/Routes.php
   @@ -114,28 +115,38 @@ class Routes implements ContainerInjectionInterface {
   -        ->addDefaults($defaults)
   ...
   +      $route_individual = (new Route(sprintf('%s/{%s}', $route_base_path, $resource_type->getEntityTypeId()), $defaults))
   

   Nit: keeping addDefaults($defaults) here would make the change slightly simpler to understand.

4. +++ b/src/Routing/Routes.php
   @@ -154,13 +165,13 @@ class Routes implements ContainerInjectionInterface {
   +        ->addDefaults(['serialization_class' => EntityReferenceFieldItemList::class])
   ...
   -        ->setOption('serialization_class', EntityReferenceFieldItemList::class)
   

   Can we avoid this change? This feels out of scope.

As long as things are working as expected in all supported core versions I'm not opposed to code simplifications. Besides, this feels like a clean way to avoid CurrentContext (which was created under the expectation to have to manage complex application states and proved to not be the case). 👏🏽👏🏽

However I worry a bit about priority. Sorry to sound like a broken record, but this seems like it did not move us towards any of the issues in #2931785: The path for JSON API to core. I think our laser focus should be features we need to get in (like #2932035: ResourceTypes should be internal when EntityType::isInternal is TRUE, #2887313: JSON API indicates it supports POST/PATCH/DELETE of config entity types, but that's impossible, …), test coverage (like #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only) and bug fixes (like #2935947: Comply with code standards).

Once those categories are depleted I think our focus should be on getting a core comitter / fwk manager / whatev to start taking a look to the module. Then, in parallel, we can knock off these code refactors.

In summary, I think issues like this are very important but I'd like to get away from the core committer mind-reading so we can move steadily towards the goal to get this guy into core.

#7:

The patch is not yet updating \Drupal\jsonapi\EntityToJsonApi.

No. Why would it?

Because >50% of the code in that class is dealing with simulating "current context".

#8: 😍 — thank you, that helps a lot!

The fact that #7 is passing today means it's passing with #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only's tests also passing. I'd like to hold off on committing this until #2932031: Comprehensive JSON API integration test coverage phase 2: for every entity type, complex use-cases is in too, because that gives us even stronger guarantees this is not breaking anything. Those two issues combined mean we can be 100% certain at least individual requests still work as they did before. Collection requests, includes, sparse fieldsets etc do not yet have similarly comprehensive integration tests though, for that we have #2945093: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets. Should we wait for that to happen before committing this? It seems that's preferable, because more prudent. At least until some important bug is reported that would be fixed by committing this issue — when that happens, I definitely wouldn't want to wait for #2945093 to land first anymore.

Perhaps this was accurate in the past, but I don't think that's accurate now. AFAICT, all it does is prepare the serialization context, which is not the same as simulating "current context".

Oops, you're right! (Side note: we have too many contexts: Symfony/Drupal request context, Drupal context, Drupal cache context, Drupal render context, Symfony/Drupal serialization context and JSON API current context. It's great to see this issue removing one of them!)

I'm 80% certain that this will fix #2922121 […] My hope is that those tests will actually reveal the bugs that will be fixed by this. I'm sure those bugs are lurking there in the corner cases; we just need to hit them with a spotlight.

That sounds wonderful! Does that mean we want to officially mark this postponed on #2922121? (Also — please see #2945093-3: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets.)

I think it's probably best to postpone this on #2945093: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets, which is SUPER close to getting committed :)

Per #2883086-33: [PP-1] Port RequestHandler + ResourceResponseSubscriber improvements from REST module to JSON API, this is a blocker for that issue, and this should go in after #2935370: Mark the JSON API serializer, normalizer and encoder services as private.

That means this is now PP-2: blocked on both #2935370 and #2945093.

• #7: diffstat: 15 files changed, 147 insertions, 482 deletions.
• Per #14, this is rebased on top of #2935370. Now the diffstat is: 15 files changed, 145 insertions, 488 deletions.

Pretty much the same, as expected!

The "combined" patch includes #2935370-24: Mark the JSON API serializer, normalizer and encoder services as private, which is currently RTBC.

#2935370: Mark the JSON API serializer, normalizer and encoder services as private landed. This is just blocked on #2945093: Comprehensive JSON API integration test coverage phase 3: test JSON API-specific use cases: related/relationship routes, includes and sparse field sets now, which is also RTBC!

After a review of the code I cannot see the feature, the bug or the benefit of this change. I'm struggling to see the benefit of this patch. This is a nice refactor, but I don't feel is necessary at this point. I'm leaning towards taking the same route (pun intended) that we went with in #2932679: Remove unused "on_relationship" serialization context. Let's postpone for #2842148: The path for JSON API to "super stability".

I agree with @gabesullice, because:

1. 300 fewer lines of logic to maintain! That's not insignificant. There have been numerous issues caused by the "current context" service. It has served its purpose: it helped us get the JSON API module where it's usable and reliable. But Gabe found this simpler approach. Simpler means more reliable. Thanks to the test coverage, we know that we're not breaking anything. Why not go with the simpler solution then?
2. Problems related to the jsonapi.current_context service:
   1. #2930228: Refactor CurrentContext away
   2. #2925043: Server error when using the jsonapi.entity.to_jsonapi service
   3. #2922121: When fetching related resources, getResourceType() in CurrentContext gets the wrong resource type.

   The latter two are bug reports that have been open for 4 months! This patch solves them.

3. +++ b/jsonapi.services.yml
   @@ -143,7 +145,6 @@ services:
   -      - '@jsonapi.current_context'
   
   +++ b/src/Controller/RequestHandler.php
   @@ -87,12 +78,8 @@ class RequestHandler {
   -   * @param \Drupal\jsonapi\Context\CurrentContext $current_context
   -   *   The current JSON API context.
   
   @@ -102,9 +89,8 @@ class RequestHandler {
   -  public function __construct($serializer, CurrentContext $current_context, RendererInterface $renderer, ResourceTypeRepositoryInterface $resource_type_repository, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $field_manager, FieldTypePluginManagerInterface $field_type_manager, LinkManager $link_manager) {
   +  public function __construct($serializer, RendererInterface $renderer, ResourceTypeRepositoryInterface $resource_type_repository, EntityTypeManagerInterface $entity_type_manager, EntityFieldManagerInterface $field_manager, FieldTypePluginManagerInterface $field_type_manager, LinkManager $link_manager) {
   

   This alone is a good sign: our RequestHandler needs so much to be injected, removing a service dependency confirms this is an important simplification!

   Not to mention that there is a very real risk core committers will require that this class gets fewer services injected, because it is currently too complex. It has currently 8 dependencies injected, compared to only 2 in core's \Drupal\rest\RequestHandler!

4. +++ b/src/Controller/RequestHandler.php
   @@ -116,40 +102,39 @@ class RequestHandler {
   -    $this->currentContext->reset();
   

   And this. This was modifying what amounts to global state. The removal of this line proves global state handling is removed. That is a huge risk removed.

I hope you will reconsider, @e0ipso. I do agree with your request for relentless focus on #2931785: The path for JSON API to core and nothing else. But this too presents a significant risk.

That being said, upon re-reviewing, I found a few nits:

1. +++ b/src/Controller/RequestHandler.php
   @@ -87,12 +78,8 @@ class RequestHandler {
   -   * @param \Drupal\jsonapi\ResourceType\ResourceTypeRepositoryInterface $resource_type_repository
   -   *   The resource type repository.
   

   Nit: This should not be deleted.

2. +++ b/src/Controller/RequestHandler.php
   @@ -116,40 +102,39 @@ class RequestHandler {
   -   * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
   -   *   The route match.
   ...
   -    $route = $route_match->getRouteObject();
   ...
   +    if ($related = $request->get('related')) {
   
   @@ -206,26 +190,28 @@ class RequestHandler {
   -        elseif ($route_match->getParameter('related')) {
   +        elseif ($request->get('related')) {
   

   Nit: Is this change really necessary? It seems this could remain as-is? That'd reduce the amount of change here: it'd mean that 100% of the changes are related to "current context".

#2883086: [PP-1] Port RequestHandler + ResourceResponseSubscriber improvements from REST module to JSON API has been RTBC for a week, but is blocked on this. Let's get this done!

This is still blocking #2922121: When fetching related resources, getResourceType() in CurrentContext gets the wrong resource type. and #2925043: Server error when using the jsonapi.entity.to_jsonapi service. It'd solve both of those bugs. Hence bumping to Major.

I still think that we should not keep on changing things that work. I won't go into a debate on the different points in the comments above, because that's an awful use of the time of everyone here, given that everyone's position is clear :-P. I am going to merge this and tag a release with it.

However I'd like that you realize that refactors, that just change the way things are implemented into a way that you like better, have other implications. They require a free time that is scarce to me to review, they break things in related modules (JSON API Extras, Consumer Image Styles, Contenta CMS integrations, …) that then I need to fix in that scarce free time, they also pose a risk of unexpected regressions (don't underestimate the tested by production usage factor), etc. Since I don't have full time paid dedication on this, my options are either to ask for this, become a blocker here and in other projects, or step aside. I think that I can keep up if we postpone non-critical refactor for the future (I'm not saying they shouldn't happen). Also, please remember that arguments like complexity, usability, maintainability, reliability, etc. are most often subjective.

I hope you appreciate the honesty in my feelings here, and take it as a plea rather than criticism. This is my attempt to keep on being passionate about this and avoid burnout.

With that said, I think that this patch is an improvement over the existing code and I am merging it.

Thanks to this issue, we fixed two bugs (#2925043: Server error when using the jsonapi.entity.to_jsonapi service and #2922121: When fetching related resources, getResourceType() in CurrentContext gets the wrong resource type.) and have less code to maintain (#2930228: Refactor CurrentContext away). If we didn't fix bugs in the process, I'd agree with not making this change!