Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#10 | 2841591-10-alt-commits.patch | 21.16 KB | Wim Leers |
#10 | 2841591-10-alt.patch | 18.96 KB | Wim Leers |
Comments
Comment #2
Wim LeersStep 1: rename
RequestHandler
toJsonApiResourceController
.Comment #3
Wim LeersStep 2: remove
EntityResourceInterface
.Comment #4
Wim LeersStep 3: move over the constructor logic from
EntityResource
toJsonApiResourceController
, and make minor adjustments toJsonApiResourceController
… to prepare for the next step.Comment #5
Wim LeersStep 4: actually move the logic from
EntityResource
toJsonApiResourceController
, and deleteEntityResource
.Comment #6
Wim LeersStep 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.Comment #7
Wim LeersStep 6: update
RequestHandlerTest
.Comment #8
Wim LeersStep 7: update
EntityResourceTest
.This is a nightmare. Skipping for now, until I'm certain e0ipso is +1 to all this.
Comment #9
Wim LeersStep 8: split up
JsonApiResourceController
(the successor ofRequestHandler
) 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 callingEntityResource::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.
Comment #10
Wim Leers#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:
EntityResourceInterface
(identical to #3's interdiff)\Drupal\jsonapi\RequestHandler
to\Drupal\jsonapi\Controller\RequestHandler
(i.e. into aController
subnamespace)\Drupal\jsonapi\Resource\EntityResource
to\Drupal\jsonapi\Controller\EntityResource
(i.e. into that sameController
subnamespace, out of theResource
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.
Comment #11
Wim Leers#2841287: Remove ResourceManager::hasBundle(), ResourceManager::getEntityTypeManager(), ResourceConfig::getPath(), and rename ResourceConfig::getBundleId() is in, so this is now blocked only on #2841296: Rename ResourceConfig & ResourceManager and remove their interfaces.
Comment #12
e0ipsoComment #13
e0ipsoThere is only one very minor nit. I'm merging this along with the required change.
\Drupal\jsonapi\Controller\EntityResourceInterface does not exist.
Comment #15
e0ipsoComment #16
Wim LeersHurray!