General
All the @package annotations are not something we do in Drupal core.
Annotation
  1. The docblock doesn't follow the standard. For examples see \Drupal\ckeditor\Annotation\CKEditorPlugin + \Drupal\quickedit\Annotation\InPlaceEditor + \Drupal\rest\Annotation\RestResource.
  2. The 'status' key. Ideally: remove (why does this exist?), otherwise: rename to 'status'.
  3. The 'data' key. This is a free-form storage. That's not what plugin definitions are designed to contain. So far, this only has 'prefix' and 'partialPath'. Why not make those explicit? What other use cases do you anticipate?
  4. The 'entityType' key. This limits JSON API to data stored in entities. Either that's intentional, and we can just remove JSON API plugins altogether (and generate routes directly etc), or we must remove this key.
  5. The 'bundle' key. Same.
  6. It seems additional plugin definition keys exist that are not listed: 'hasBundle', 'type', 'permission' and 'controller'.
The "Bundle" JSON API resource
Why not call it "entity bundle"? Just "bundle" makes little sense.
The plugin deriver
  1. The @see in ResourceDriver is wrong
  2. ResourceDriver should be called BundleDeriver
The plugin interface
  1. It still has the default comment: // Add get/set methods for your plugin type here..
  2. Worse, the sole implementation of this interface has no methods:
    class BundleJsonApiResource extends ContextAwarePluginBase implements JsonApiResourceInterface {}
    

    … so if your plugins don't implement any methods, what purpose do they serve? This touches on the problems I pointed out in the "General" section about the 'entityType' and 'bundle' keys.

The plugin namespace
This is currently jsonapi — it probably should be JsonApiResource.
The plugin manager
There's nothing to remark on JsonApiResourceManager… but
I first thought \Drupal\jsonapi\Configuration\ResourceManager(Interface) was the plugin manager, but apparently it is not. It's something completely independent. I'm not sure the name of that class can be improved, but it's very similar, and hence confusing.

Parts that are struck through have already been addressed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Issue summary: View changes
Status: Active » Needs review
FileSize
8.65 KB
Wim Leers’s picture

e0ipso’s picture

Great insights!

  1. +++ b/src/Annotation/JsonApiResource.php
    @@ -5,9 +5,17 @@ namespace Drupal\jsonapi\Annotation;
    + * Defines a CKEditorPlugin annotation object.
    

    This is probably a copy & paste leftover.

  2. +++ b/src/Annotation/JsonApiResource.php
    @@ -33,6 +41,8 @@ class JsonApiResource extends Plugin {
    +   * @todo This limits JSON API to data stored in entities. Either that's intentional, and we can just remove JSON API plugins altogether (and generate routes directly etc), or we must remove this key.
    

    It is intentional. But we cannot remove plugins since resources can be toggled off.

    This is used to detect the entity type of the current resource, without relying on a fixed URL structure.

  3. +++ b/src/Annotation/JsonApiResource.php
    @@ -47,6 +59,8 @@ class JsonApiResource extends Plugin {
    +   * @todo This is a free-form storage. That's not what plugin definitions are designed to contain. So far, this only has 'prefix' and 'partialPath'. Why not make those explicit? What other use cases do you anticipate?
    

    My initial thought was to allow 3rd party modules to store unanticipated metadata about the resource.

    Things like "I want to add a computed property to this resource, here is my callback". ATM this is just a placeholder.

    Do you want me to get rid of it?

  4. +++ b/src/Annotation/JsonApiResource.php
    @@ -54,6 +68,8 @@ class JsonApiResource extends Plugin {
    +   * @todo Ideally: remove (why does this exist?), otherwise: rename to 'status'.
    

    As I said we are allowing implementors to disable resources for certain entity types.

    https://www.drupal.org/node/2781459#comment-11749367 contains an example on how to do so.

  5. +++ b/src/Plugin/jsonapi/BundleJsonApiResource.php
    @@ -9,16 +9,16 @@ use Drupal\jsonapi\Plugin\JsonApiResourceInterface;
    + * @todo rename to EnityBundleJsonApiResource? If so, the deriver should also be renamed.
    

    I'm good with that. I went for a shorter name for readability reasons, if you don't think it's a concern we can change it.

  6. +++ b/tests/src/Unit/Plugin/Deriver/BundleDeriverTest.php
    @@ -64,13 +60,17 @@ class ResourceDeriverTest extends UnitTestCase {
    +      // @todo Not listed in \Drupal\jsonapi\Annotation\JsonApiResource.
    

    This should be added to the annotation.

Wim Leers’s picture

  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.
Wim Leers’s picture

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.

Wim Leers’s picture

Priority: Normal » Critical

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

Wim Leers’s picture

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

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.
Wim Leers’s picture

Assigned: Unassigned » Wim Leers

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.

Wim Leers’s picture

FileSize
28.48 KB

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.

Wim Leers’s picture

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.

Wim Leers’s picture

FileSize
38.61 KB
51.64 KB

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.

Wim Leers’s picture

FileSize
52.19 KB
1.3 KB

I forgot to stage the changes for the annotation.

Wim Leers’s picture

FileSize
39.24 KB
85.78 KB

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.

Wim Leers’s picture

FileSize
35.21 KB
101.06 KB

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.

Status: Needs review » Needs work

The last submitted patch, 15: 2829398-15.patch, failed testing.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
35.73 KB
135.81 KB

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

Status: Needs review » Needs work

The last submitted patch, 17: 2829398-16.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
802 bytes
135.81 KB

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

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs review » Needs work
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++
Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
135.92 KB
7.76 KB

Done.

e0ipso’s picture

I'm starting a more in depth review of this. In general, there is nothing stated in this thread I feel strongly against.

I don't like structured arrays in a well-known form, I prefer typed objects for those situations. So I'm unsure about the change of $context['resource_config'] into a scalar (that can rapidly escalate to an array).

e0ipso’s picture

This is my partial review up to the RelationshipNormalizerInterface. I have to step out, but I'll continue the review later.

One thing that seems clear to me is that we want the ResourceConfig be an object with different properties instead of doing brittle string manipulations everywhere. What do you think?

Code review:

  1. +++ b/src/Annotation/JsonApiResourceType.php
    @@ -31,31 +36,36 @@ class JsonApiResource extends Plugin {
    -   * The entity type ID.
    +   * The JSON API resource type.
    ...
    -   * The bundle ID.
    

    I am ok about this, as long as we don't need to explode $type into $entity_type_id and $bundle_id, anywhere in the code.

  2. +++ b/src/Context/CurrentContext.php
    @@ -48,13 +49,11 @@ class CurrentContext implements CurrentContextInterface {
    +  public function __construct(JsonApiResourceTypeManager $jsonapi_resource_type_manager, RequestStack $request_stack) {
    

    We should type hint the interface for testing & polymorphic purposes.

  3. +++ b/src/Context/FieldResolver.php
    @@ -65,7 +66,7 @@ class FieldResolver implements FieldResolverInterface {
    +    $entity_type_id = EntityJsonApiResourceType::getEntityTypeIdForJsonApiResourceType($this->currentContext->getResourceConfig()['type']);
    

    Maybe change to something like:

    $entity_type_id = $this->currentContext->getResourceConfig()->getEntityTypeId();
    
  4. +++ b/src/EntityCollection.php
    @@ -3,9 +3,7 @@
    + * @internal
      */
     class EntityCollection implements EntityCollectionInterface {
    

    +1

  5. +++ b/src/LinkManager/LinkManager.php
    @@ -35,22 +37,24 @@ class LinkManager implements LinkManagerInterface {
    +//    assert("in_array($key, ['individual', 'collection', 'related', 'relationship'])");
    

    Do we want to uncomment this? It seems like a good idea to me.

  6. +++ b/src/LinkManager/LinkManager.php
    @@ -58,6 +62,7 @@ class LinkManager implements LinkManagerInterface {
    +    // @todo use the "current route match" service instead.
    

    +1

  7. +++ b/src/LinkManager/LinkManagerInterface.php
    @@ -7,29 +7,28 @@ use Drupal\jsonapi\Configuration\ResourceConfigInterface;
    +   *   Which JSON API route to generate for this resource type: 'collection',
    +   *   'individual', 'related' or 'relationship'.
    

    Maybe: Which JSON API resource enpoint …

  8. +++ b/src/Normalizer/DocumentRootNormalizer.php
    @@ -75,7 +75,8 @@ class DocumentRootNormalizer extends NormalizerBase implements DenormalizerInter
    -      $id_key = $this->currentContext->getResourceConfig()->getIdKey();
    +      // @todo fix in https://www.drupal.org/node/2831134
    +      $id_key = 'uuid';
    

    I'm more and more excited to drop support for entity IDs.

    :clap:

  9. +++ b/src/Normalizer/DocumentRootNormalizer.php
    @@ -84,8 +85,8 @@ class DocumentRootNormalizer extends NormalizerBase implements DenormalizerInter
    -        $entity_storage = $this->currentContext->getResourceManager()
    -          ->getEntityTypeManager()
    +        $entity_storage = \Drupal::getContainer()
    +          ->get('entity_type.manager')
    

    Let's inject this somehow.

  10. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
    @@ -170,12 +167,10 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
    -    $target_entity_id = $item_definition->getSetting('target_type');
    +    $target_entity_type_id = $item_definition->getSetting('target_type');
    

    Good catch.

  11. +++ b/src/Plugin/jsonapi/resource_type/EntityJsonApiResourceType.php
    @@ -69,41 +58,52 @@ class EntityResource implements EntityResourceInterface {
    +    list($entity_type_id, $bundle) = explode('--', $type);
    ...
    +    list($entity_type_id, $bundle) = explode('--', $type);
    

    This is what I wanted to avoid in my other comment about the annotation.

    I'd rather have the normalized components to generate the node--page when needed.

e0ipso’s picture

This is the rest of the review.

  1. +++ b/config/install/jsonapi.resource_info.yml
    @@ -1,2 +1 @@
     id_field: uuid
    

    Should we remove this as well?

  2. +++ b/src/Normalizer/RelationshipNormalizer.php
    @@ -85,15 +85,12 @@ class RelationshipNormalizer extends NormalizerBase {
    -      /* @var \Drupal\jsonapi\RelationshipItemInterface $relationship_item */
    -      if (!$relationship_item->resourceIsEnabled()) {
    -        continue;
    -      }
    

    I realize that you said:

    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.

    The biggest problem of doing so is here, with relationships. We need to be able to know if an entity_reference is a relationship to another resource or a UUID string (when there is no resource to point to).

  3. +++ b/src/Normalizer/RelationshipNormalizer.php
    @@ -85,15 +85,12 @@ class RelationshipNormalizer extends NormalizerBase {
    +      // @Todo
    

    @TODOne already?

  4. +++ b/src/Plugin/JsonApiResourceTypeManager.php
    @@ -36,10 +24,33 @@ class JsonApiResourceManager extends DefaultPluginManager {
    +    // For now, only allow the JSON API module to provide JSON API Resource Type
    +    // plugins. The API is not yet finalized, and it's not yet clear whether it
    +    // would make sense to allow additional types beyond entity types and entity
    +    // type bundles.
    

    Have only:

    […] allow additional types beyond entity type bundles.

  5. +++ b/src/Plugin/JsonApiResourceTypeManager.php
    @@ -36,10 +24,33 @@ class JsonApiResourceManager extends DefaultPluginManager {
    +    try {
    +      return parent::getDefinition($plugin_id, $exception_on_invalid);
    +    }
    +    catch (PluginNotFoundException $e) {
    +      // This work-around will work as long as EntityJsonApiResourceType is the
    +      // only plugin.
    +      return parent::getDefinition('entity:' . $plugin_id, $exception_on_invalid);
    +    }
    

    Does this mean that we're planning to get definitions with a plugin ID that is not really a plugin ID (ie: without the derivative prefix)?

    That seems a bit shady. It also gets in the way if we ever allow 3rd party JSON API resources.

  6. +++ b/src/Plugin/jsonapi/resource_type/EntityJsonApiResourceType.php
    @@ -27,18 +24,17 @@ use Symfony\Component\HttpFoundation\Request;
    + *   id = "entity",
    + *   label = @Translation("Entity"),
    + *   deriver = "Drupal\jsonapi\Plugin\Deriver\EntityDeriver",
    

    Can we have an ID and deriver class name that hints that resources are not at the Entity Type level, but at the Bundle level?

    I realize that naming may have not been ideal before either, but let's take the chance to improve on that.

  7. +++ b/src/Relationship.php
    @@ -68,15 +59,14 @@ class Relationship implements RelationshipInterface, AccessibleInterface {
    +        \Drupal::getContainer()->get('plugin.manager.jsonapi.resource_type'),
    

    I'd use an emoji to suggest dependency injection with a syringe, but d.o chokes with a 500 with emojis. I suspect on the MySQL table schema…

    In any case, we should inject the service.

  8. +++ b/src/RequestHandler.php
    @@ -31,10 +33,22 @@ class RequestHandler implements ContainerAwareInterface, ContainerInjectionInter
    -    return new static($container->get('entity_type.manager')->getStorage('rest_resource_config'));
    

    Woah! I almost don't remember when I wrote that… I agree, it needs to go away.

  9. +++ b/src/Routing/Routes.php
    @@ -50,13 +64,17 @@ class Routes implements ContainerInjectionInterface {
    +  public function __construct(JsonApiResourceTypeManager $json_api_resource_type_manager, EntityTypeManagerInterface $entity_type_manager, EntityTypeBundleInfoInterface $bundle_manager, AuthenticationCollectorInterface $auth_collector) {
    

    Are we missing JsonApiResourceTypeManager in the docblock? Maybe it's just an unfortunate diff.

  10. +++ b/src/Routing/Routes.php
    @@ -98,70 +111,57 @@ class Routes implements ContainerInjectionInterface {
    -      if ($bundle) {
    -        $route_collection->setRequirement('_bundle', $bundle);
    -      }
    ...
    -      $parameters = [$entity_type => ['type' => 'entity:' . $entity_type]];
    

    This is used by the custom route parameter upcaster to detect wrong entities. On a route for an article getting a page is not allowed and should trigger a meaningful error.

  11. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
    @@ -339,9 +338,6 @@ class JsonApiFunctionalTest extends BrowserTestBase {
    -    // 17. Test filtering on the same field.
    -    Json::decode($this->drupalGet('api/menu/menu'));
    -    $this->assertSession()->statusCodeEquals(404);
    

    Why are we losing this?

e0ipso’s picture

I like the direction this goes. The only two things that I'd like to change are:

  • Keep the resource config object. We may make it thinner if needed, but I think it's more robust than string based operations scattered across the code base.
  • Keep the ability to disable resources at the plugin level. The main purpose is to be able to turn ER fields into attributes when they are pointing to a disabled resource.

This is impressive work what you did here with so little time!! /me takes his hat off and bows.

How do you want to keep moving this forward?

Wim Leers’s picture

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.
Wim Leers’s picture

Assigned: Unassigned » Wim Leers
FileSize
135.66 KB

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

Status: Needs review » Needs work

The last submitted patch, 27: 2829398-27.patch, failed testing.

Wim Leers’s picture

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
139.08 KB
4.1 KB

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.

Wim Leers’s picture

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.

e0ipso’s picture

Slowly reviewing this. This caught my attention in the interdiff for #30.

+++ b/tests/src/Functional/JsonApiFunctionalTest.php
@@ -355,11 +354,8 @@ class JsonApiFunctionalTest extends BrowserTestBase {
-    // 17. Test filtering on the same field.
-    Json::decode($this->drupalGet('api/menu/menu'));
-    $this->assertSession()->statusCodeEquals(404);
-    // 18. Single user (check fields lacking 'view' access).
-    $user_url = Url::fromRoute('api.dynamic.user--user.individual', [
+    // 17. Single user (check fields lacking 'view' access).
+    $user_url = Url::fromRoute('jsonapi.user--user.individual', [

It seems we are losing a test in here.

Wim Leers’s picture

#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.

e0ipso’s picture

LOL. I repeat myself.

Sorry, I just need to get a good chunk of time to review this and help to move it forward.

e0ipso’s picture

I think this is the time to make an important decision. Let's lock the resources to bundles. One resource is tied one (and only one) entity type + bundle. Also, one entity type + bundle can only have one resource. Since there is no single ID that can help identify entity type + bundle, I suggest using "$entity_type_id--$bundle" to match the type of the resource. We can call this the resource ID from now on.

Given a resource ID we should be able to build a plugin ID, maybe use the resource ID directly (?). A plugin for a resource should be aware of the entity type id and bundle id that the resource is for. This will effectively merge the responsibilities of ResourceConfig into the resource itself and reduce / eliminate the usage of the ResourceManager (which should only remain for relationships & related stuff).

I believe that this proposal will address #31.1, #31.2, #31.5 and make the code simpler.

Wim Leers’s picture

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.

Wim Leers’s picture

Wim Leers’s picture

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

Wim Leers’s picture

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.

Wim Leers’s picture

#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.

Wim Leers’s picture

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.

Wim Leers’s picture

Wim Leers’s picture

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

Wim Leers’s picture

Category: Task » Plan
Wim Leers’s picture

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.

Wim Leers’s picture

Status: Needs review » Active
Issue tags: +blocker

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

e0ipso’s picture

Status: Active » Fixed

Everything was marked as fixed. Fixing this plan as well.

Wim Leers’s picture

Indeed! So glad to have this closed :)

Status: Fixed » Closed (fixed)

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