Per #2829398-8: Clean up JsonApiResource: the annotation, plugin type, plugin manager, plugin implementations, the dynamic routes generator and the request handler.

EntityResource contains the actual CRUD logic for entities (resources), as well as relationships/related. RequestHandler is tightly coupled to the methods that exist on EntityResource. So it'd be better to move EntityResource's logic into RequestHandler, i.e. to stop pretending they're independent, because they're tightly coupled already.

Blocked on #2841296: Rename ResourceConfig & ResourceManager and remove their interfaces.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Title: Remove EntityResource, move its logic into RequestHandler, clean up routes » [PP-2] Remove EntityResource, move its logic into RequestHandler, clean up routes
Status: Active » Postponed
FileSize
2.83 KB

Step 1: rename RequestHandler to JsonApiResourceController.

Wim Leers’s picture

FileSize
16.02 KB
13.26 KB

Step 2: remove EntityResourceInterface.

Wim Leers’s picture

FileSize
6.68 KB
22.27 KB

Step 3: move over the constructor logic from EntityResource to JsonApiResourceController, and make minor adjustments to JsonApiResourceController… to prepare for the next step.

Wim Leers’s picture

FileSize
56.82 KB
41.52 KB

Step 4: actually move the logic from EntityResource to JsonApiResourceController, and delete EntityResource.

Wim Leers’s picture

FileSize
7.03 KB
42.46 KB

Step 5: minimal clean-up of JsonApiResourceController, to remove the most confusing bits & pieces. $this->fieldManager was never used anywhere, so deleted it. $this->pluginManager should've been named $this->fieldTypeManager, so did that. Several places were A) retrieving a service from the container even though it was being injected, B) and it was then being passed around, equally pointlessly.

Wim Leers’s picture

FileSize
3.46 KB
44.74 KB

Step 6: update RequestHandlerTest.

Wim Leers’s picture

Step 7: update EntityResourceTest.

This is a nightmare. Skipping for now, until I'm certain e0ipso is +1 to all this.

Wim Leers’s picture

Issue summary: View changes

Step 8: split up JsonApiResourceController (the successor of RequestHandler) to the four separate controllers mentioned in the IS.

I failed miserably. There's still too much coupling & technical debt to be able to do that.

Problem: controller doing routing

Why do we even need to split it up? Because (RequestHandler|JsonApiResourceController)::action() is in fact just another router. We shouldn't have routers in our controllers. We should just have response logic, not routing logic. So, our routes should just point to concrete controllers, not to a generic entry point (RequestHandler::handle()) which then eventually results in additional routing.
Yes, the REST module in Drupal core does this too. But it has a sound excuse: it allows @RESTResource plugins to be created, i.e. it must automatically provide a controller for every REST resource plugin. The JSON API module doesn't have this need/requirement.

Problem: controller post-processing responses

This was also a problem in REST's request handler, but was fixed in #2807501: ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty. This will allow you to remove the presence of the renderer service in the controller, and (RequestHandler|JsonApiResourceController)::renderJsonApiResponse() will be able to go away.

Problem: all interdependent

EntityResource::getRelated() is calling EntityResource::getIndividual. But that should be using a subrequest. Introducing the use of subrequests for this would mean massive, massive refactoring of the logic itself. Which is out of scope here.


Therefore I think it makes sense to make this issue slightly less ambitious: the cleaning up of routes (and the associated splitting into multiple controllers) will have to wait for another issue. I figured it wouldn't be too invasive, but it is, which means this doesn't really make sense anymore to do at this time. It will make sense to do later.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
FileSize
18.96 KB
21.16 KB

#8 + #9 together mean that I don't know if it's actually remotely feasible to do everything this intended to do. :( :( Bye bye, several hours of work. But alas, that's part of untangling technical debt.

So, here's a much, much simpler alternative proposal. It does 3 things:

  1. Remove EntityResourceInterface (identical to #3's interdiff)
  2. Move \Drupal\jsonapi\RequestHandler to \Drupal\jsonapi\Controller\RequestHandler (i.e. into a Controller subnamespace)
  3. Move \Drupal\jsonapi\Resource\EntityResource to \Drupal\jsonapi\Controller\EntityResource (i.e. into that same Controller subnamespace, out of the Resource subnamespace)

This strongly signals that they are both necessary for "Controller" functionality. Which is sufficient clarification for now. Patch attached. The "commits" patch does 3 commits that do exactly those 3 steps.

Wim Leers’s picture

Title: [PP-2] Remove EntityResource, move its logic into RequestHandler, clean up routes » [PP-1] Remove EntityResource, move its logic into RequestHandler, clean up routes
e0ipso’s picture

Title: [PP-1] Remove EntityResource, move its logic into RequestHandler, clean up routes » Remove EntityResource, move its logic into RequestHandler, clean up routes
Status: Postponed » Needs review
e0ipso’s picture

Status: Needs review » Reviewed & tested by the community

There is only one very minor nit. I'm merging this along with the required change.

+++ b/src/Controller/RequestHandler.php
@@ -255,7 +254,7 @@ class RequestHandler implements ContainerAwareInterface, ContainerInjectionInter
-   * @return \Drupal\jsonapi\Resource\EntityResourceInterface
+   * @return \Drupal\jsonapi\Controller\EntityResourceInterface

\Drupal\jsonapi\Controller\EntityResourceInterface does not exist.

  • e0ipso committed 221663d on 8.x-1.x authored by Wim Leers
    fix(Maintainability) Remove EntityResourceInterface, consolidate...
e0ipso’s picture

Status: Reviewed & tested by the community » Fixed
Wim Leers’s picture

Hurray!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.