Clean up JsonApiResource: the annotation, plugin type, plugin manager, plugin implementations, the dynamic routes generator and the request handler

1. Hah, oops, yes!
2. I don't understand why we can't remove plugins because resources can be turned off. The turning off happens at \Drupal\jsonapi\Routing\Routes::routes() and it's at \Drupal\jsonapi\Configuration\ResourceManager::all() that you invoke the alter hook to allow specific entity JSON API resources to be turned off. None of that is in any way dependent on plugins.
3. I think it's safer to get rid of it, and then add it later when the need arises :) Only grow in complexity when you truly understand the needed complexity.
4. Right, but then it should be called status, for consistency with the rest of core.

Oh and apparently it's \Drupal\jsonapi\Resource\EntityResource(Interface) that contain the actual logic. If you want to keep plugins, I think it's this what should become the sole default plugin and the plugin interface. That could actually work. But again, I'd defer that to the future, when a real need arises to have JSON API for content other than entities.

This is by far the most important architectural issue that needs to be addressed.

To add to #6: \Drupal\jsonapi\Resource\EntityResource(Interface) is what contains the actual logic, but then there is logic in \Drupal\jsonapi\RequestHandler to actually call the appropriate method on that EntityResource class.

So, the separation/coupling between:

1. the RequestHandler, which calls
2. the EntityResource class, which contains the actual logic of
3. the BundleJsonApiResource plugin, which contains zero logic, and for which per-entity-type-and-bundle derivatives are generated by
4. the BundleDeriver class, for which routes are generated by
5. the \Drupal\jsonapi\Routing\Routes class, which makes the two aforementioned classes pretty much pointless…
6. … indicates that this is in need of some serious refactoring.

Taking this on.

In the mean time, already did #2831127: \Drupal\jsonapi\Plugin\FileDownloadUrl is not a plugin, links to wrong core issue, because that'll get in the way while working on this.

This is essentially the direction. Unfortunately, lots of services use the jsonapi.resource.manager service, so I've got lots more refactoring ahead of me to actually make this work.

What makes all of this at least ten times harder/more work than I thought, is the fact that code just about everywhere in the JSON API module is using \Drupal\jsonapi\Configuration\ResourceConfig objects. From the normalizers, to the link manager, to the JSON API context. It all retrieves metadata from ResourceConfig objects. It's actually arguably one of the most important objects in the JSON API module. You just would never know by looking at the name.

Still looking into how to deal with this. But it seems pretty clear that I can't get rid of it all. I'm still certain it can be made a whole lot clearer/simpler though.

So, rather than getting rid of the plugin type/manager/annotation and Resource(Config|Manager) and EntityResource, I just fixed it:

1. Renamed the JsonApiResource annotation to JsonApiResourceType, because for every resource type in JSON API, you can do CRUD on individual entities, but you can also access the collection of all resources of that type, and look at its relationships.
2. So, renamed BundleJsonApiResource to EntityJsonApiResourcetype, and moved all logic in EntityResource into it. Now BundleJsonApiResource is no longer an empty shell.
3. Because it's still not yet clear whether it makes sense to expose additional resource types beyond those for entities, I made the JsonApiResourceTypeManager (a plugin manager) only look for plugins in the JSON API module, which effectively means contrib can't add more JSON API Resource Type plugins.
4. The end result is that it's now actually possible to add new resource types in the future, that are not backed by entities. We'd still need to figure out how to make that work with relationships and so on, but as long as they don't have relationships with entities (in which case they arguably should be entities), this should be sufficient.

This is succesfully generating derived plugins and routes. Actual responses do not yet work.

I forgot to stage the changes for the annotation.

And this then is fully operational. There's definitely ugliness, but that's just because all of the existing normalizers either call out to "the current context", or call out to the resource manager (which is supposed to be entity-agnostic but now is no longer), and so on.

I took two shortcuts here:

1. I hardcoded UUID support, which means this will only work for entity types that support UUIDs.
2. I removed the "enabled" flag, which means you can no longer prevent certain entity types from being exposed via JSON API. Arguably, this can be done at the entity type access control handler level (which actually is already the case), or even the routing level.

So I think only the first is a real shortcut. And even then, \Drupal\aggregator\Entity\Item is the only entity type in core that doesn't support UUIDs (and it has an issue to fix that), so I think this is fine for now. UUIDs only really simplifies things, and is The Right Thing To Do for JSON API anyway.

This patch (unlike the on in #12/#13) does need a lot of clean-up still.

Oh, WTF.

Apparently JSON API in HEAD:

1. does not generate a /api/node route that returns all nodes, regardless of bundle (my patch above adds this)
2. if there's only a single bundle==no bundle (for example for view), it still generates a view--view type instead of just view, and worse, it generates a /api/view/view route, instead of just /api/view

IMHO those things do not make sense. It does make the code a bit easier. But they don't make sense.

This cleans up part of what #14 did. And it makes the functional test coverage be green again. Many other tests will fail though.

Skip all those failing tests. Failing tests fail because they rely on HEAD's ResourceConfig and ResourceManager. Hence many tests need significant refactoring.

Now this is blocked on feedback from mateu. Hopefully he agrees with the high-level direction and can push it to the finish line.

What's still left as a TODO, but which can be done in a follow-up IMO is:

1. removing the need for EntityJsonApiResourceType::(getJsonApiResourceTypeForEntity|getEntityTypeIdForJsonApiResourceType|getBundleForJsonApiResourceType)
2. changing $context['resource_config'] to no longer receive the entire plugin definition, but just a JSON API resource type (e.g. node--article)
3. renaming $context['resource_config'] to something more sensible

Ugh, IDK why, but Notice: Unexpected PHP error: Use of undefined constant individual - assumed 'individual' is fixed by this change.

16:52:08 <e0ipso> WimLeers the only think I have a somewhat strong opinion is on /<entityType>/<bundle>/<id>
16:52:26 <e0ipso> we had some intense back and forth
16:52:36 <e0ipso> and long discussions about it in the past
16:52:46 <e0ipso> I can pull up the videos
16:53:01 <e0ipso> The 2 main outcomes are:
16:53:01 <WimLeers> e0ipso: even when 'bundle' == 'entity type ID' ?
16:53:05 <WimLeers> e0ipso: i.e. node--node
16:53:08 <WimLeers> e0ipso: view--view
16:53:11 <WimLeers> e0ipso: tour--tour
16:53:15 <e0ipso> yeah
16:53:23 <WimLeers> ok curious about the reasoning for that
16:53:28 <WimLeers> but that's not a big deal to change anyway :)
16:53:43 <e0ipso> 1. No route should be exposed if we cannot generate a schema for the response
16:53:59 <e0ipso> like /api/node returning entities of all bundles
16:54:14 <WimLeers> e0ipso: ahhh +1
16:54:18 <WimLeers> e0ipso: I _knew_ there was a reason
16:54:33 <e0ipso> 2. We should not break BC if some module (custom or contrib) changes the entity definition
16:54:35 <WimLeers> e0ipso: and then 2. better to be consistent
16:54:43 <WimLeers> e0ipso: right, if somebody adds bundleability?
16:54:44 <e0ipso> at least at the routing level
16:54:49 <e0ipso> yeah
16:54:52 <WimLeers> e0ipso++

Done.

We discussed this during the weekly JSON API meeting.

1. We agreed to keep the ResourceConfig objects. They're objects holding information about entity type ID + bundle, and know how to generate the corresponding JSON API type. They also know how to do reverse lookups (type -> entity type ID + bundle), to remove the need for explode('--', $type).
2. We agreed to drop the ability to disable resources, because that's what the Entity Access API already is designed to do. As e0ipso put it: if you can read the data by scraping HTML, then it's also okay to be able to read the data via JSON API. Preventing access to it via either HTML or JSON API is the same: define the appropriate logic in the Entity Access API.
3. We agreed to work on a Drupal 8.3 core experimental module proposal for JSON API this week.

Continuing my work on this. First, rebasing to apply cleanly to HEAD again.

#26.3 was already done btw: #2836165: New experimental module: JSON API.

I resolved one conflict incorrectly.

And #2833850: IDs not converted to UUIDs for POST/PATCH to relationship endpoint introduced a bunch of code that required a fair amount of changes.

This should make the patch green again.

First: let's deal with "types" (e.g. node--article, user--user, etc) versus entity type IDs + bundles. This is the most important piece of feedback probably, and the hardest to address.

1. #23.1: Agreed in principle. But the real problem is that almost all of code pretends to be entity-agnostic, and entirely modeled on JSON API concepts, but it's not: almost all code contains entity type, bundle etc logic. So in order to comply with this, I'll need to fix all that.
2. #23.3: No, that would just move the problem around. The ResourceConfig value object (better name TBD, per #26) MUST NOT be aware of entity types. Otherwise you still can't add new (non-entity) JSON API resource types.
3. #23.9: See point 1.
4. #23.11: See point 1.
5. #24.5: Yes and no. It just means that I personally think that you should be able to do JsonApiResourceTypeManager::getDefinition('node--article'). Besides making code elsewhere simpler, there's a more important reason: prevent the same JSON API resource type to be defined by multiple plugins! Because right now it's totally possible to have entity:node--article and foobar:node--article plugins. Which means we're not guaranteeing that one and only one plugin can define a particular JSON API resource type. This is hugely problematic.
   This is why I wrote this in the accompanying docblock:

      * @todo \Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator::encodePluginId()
      * gets in the way; we specifically want all derived plugins to not be
      * prefixed by their base plugin ID, because that helps ensure that across all
      * JSON API Resource Type plugins, each type is specified only once.
   

Still trying to find a way to make this work. In the mean time, you can let me know whether you are convinced by these answers.

#32: you made the same remark at #24.11. That was explicitly testing whether a hook was able to set enabled = FALSE and whether that had the intended consequences. We agreed in #26.2 that that's no longer necessary/relevant.

I was working on moving forward on what I described in #31. But #35 means we're permanently tying the architecture of the JSON API module (at least for now) to entity type ID+bundles.

Which means that the entire problem of having the JSON API module deal in the concept of "resources" and "relationships" etc goes away, because "resources" become "entities" (of a specific entity type ID + bundles) and "relationships" become "entity references".

It also means we no longer have to refactor (now or later) close to 100% of the architecture/internals of the JSON API module, because concepts like "entity collection" for example really should have been "resource collection", which just might happen to contain entities, "field resolver" should have been "attribute resolver" which just might happen to contain fields, and so on.

But, probably the strongest argument of all to design JSON API to only ever support entities for the foreseeable future: relationships & collections. It would be difficult to support collection queries that involve relationships to non-entity data.

Therefore, I support this.

Even more so because the JSON API module has no PHP APIs, it only offers functionality: a RESTful API. Which means we're effectively free to change the implementation at any time, theoretically even when it becomes stable. This is also the direction that the BigPipe module chose: #2835604: BigPipe provides functionality, not an API: mark all classes & interfaces @internal. This deliberate design choice, combined with a complete lack of configuration (also just like the BigPipe) module would make the JSON API module perfectly able to iterate on in the future: its internal architecture can be changed completely, as long as it provides the same exact HTTP API: the same URLs, the same supported requests, the same responses.

So, I'm +1.

This does mean that we're throwing away pretty much everything in all the patches here. But that's not a bad thing! This would've been a momentously difficult patch to land anyway. We tried a different direction. We learned.

The good thing is we have agreement on many fronts:

1. most importantly: the scope is agreed upon: the JSON API module will only ever deal with entities
2. the plugin annotation/type/manager/implementation is messy, and needs to go away
3. same for the dynamic routes generator
4. the "enabled"-ness of entity types wrt JSON API needs to go away
5. the use of ResourceConfig value objects makes conceptual sense, but there are issues in the implementation as well as in the naming

So, moving forward on that common ground now, in separate child issues. That will make progress much easier.

To get started, here's a first child issue, with patch: #2838630: Remove the concept of "enabledness" from the JSON API module, rely on the Entity Access API instead. And in the process of working on that one, I already discovered another big bug that we definitely need to fix: #2838646: Relationships violate Entity Access API.

Added #2838580: Remove configurable 'prefix' + change the non-configurable prefix from '/api' to '/jsonapi' to remove the prefix configuration.

And now finished up #2831134: Remove configurable 'id_field': always use UUID. Together with #2838580, this will mean we'll have zero configuration left :)

Yay, #2838580: Remove configurable 'prefix' + change the non-configurable prefix from '/api' to '/jsonapi' and #2831134: Remove configurable 'id_field': always use UUID have already landed! This means we have zero configuration left, and \Drupal\jsonapi\Configuration\ResourceConfig became slightly simpler.

Next up is #2838630: Remove the concept of "enabledness" from the JSON API module, rely on the Entity Access API instead, which will make ResourceConfig again simpler.

#2831134: Remove configurable 'id_field': always use UUID landed.

And #2838630: Remove the concept of "enabledness" from the JSON API module, rely on the Entity Access API instead also landed just now! We're making good progress here.

I think next up is for me to work on simplification/unification of the plugin stuff and the ResourceConfig stuff. I'll be on vacation next week, so expect more patches first week of January.

In order to clean up ResourceConfig, we must first ensure that it's the single source of truth, rather than the truth being spread over ResourceConfig and a plugin. So, let's refactor the plugin away.

But in order to refactor the plugin away, we kind of need ResourceConfig objects to actually be value objects first, that'd make updating RoutesTest a lot easier.

So, let's start with making ResourceConfig actual value objects: let them be immutable, and let them not have any services injected:

1. #2841048: Remove ResourceConfig::getGlobalConfig(), and stop injecting the config factory service in ResourceConfig value objects is easy thanks to #2831134: Remove configurable 'id_field': always use UUID, which removed the last configuration
2. #2841050: Make ResourceConfig an actual value object: immutable, with no services injected is slightly bigger, but still not at all hard

Once those two land, we can do #2841056: Remove use of plugins.

The first two have landed. Yay!

#2841056: Remove use of plugins is currently awaiting review + commit.

Next step: #2841287: Remove ResourceManager::hasBundle(), ResourceManager::getEntityTypeManager(), ResourceConfig::getPath(), and rename ResourceConfig::getBundleId().

And another next step: #2841296: Rename ResourceConfig & ResourceManager and remove their interfaces. This one is still being developed.

So the queue is now:

1. Ready: #2841056: Remove use of plugins
2. Ready: #2841287: Remove ResourceManager::hasBundle(), ResourceManager::getEntityTypeManager(), ResourceConfig::getPath(), and rename ResourceConfig::getBundleId()
3. In progress: #2841296: Rename ResourceConfig & ResourceManager and remove their interfaces

#2841056 is in. #2841287 is waiting to be committed. #2841296 is blocked on that, and is also ready.

The next and final (!) step is to address EntityResource + RequestHandler (see #8) and the associated routes.

Created #2841591: Remove EntityResource, move its logic into RequestHandler, clean up routes for that, which is blocked on #2841296: Rename ResourceConfig & ResourceManager and remove their interfaces.

#2841056: Remove use of plugins and #2841287: Remove ResourceManager::hasBundle(), ResourceManager::getEntityTypeManager(), ResourceConfig::getPath(), and rename ResourceConfig::getBundleId() are in!

Now we're down to the last two patches:

1. #2841296: Rename ResourceConfig & ResourceManager and remove their interfaces
2. #2841591: Remove EntityResource, move its logic into RequestHandler, clean up routes

After those two patches are in, we can do #2842110: Meet Drupal coding standards.

Indeed! So glad to have this closed :)