Move the write functionality of config entities to a sub-module in preparation for removal

Posted this to #2952293-25: Branch next major: version 2, requiring Drupal core >=8.5.

I think we can skip the submodule part in 1.x and simply make a submodule that can be added to extras. Doing it in 1.x was simply a place for it to be developed while 1.x was the primary development branch of JSON API AFAICT.

leaving the users that use this feature stranded until this is replicated elsewhere

Let's definitely not do that :)

To avoid just removing it and leaving the users that use this feature stranded until this is replicated elsewhere

+1

(I wrote this yesterday but my battery died :P)

I pledge that we will do the necessary work to bring this to JSON API Extras. I just did the same for the EntityToJsonApi service/functionality: #2982455: Add `jsonapi.entity.to_jsonapi` service.

\Drupal\rest\Plugin\rest\resource\EntityResource does this:

  /**
   * {@inheritdoc}
   */
  public function availableMethods() {
    $methods = parent::availableMethods();
    if ($this->isConfigEntityResource()) {
      // Currently only GET is supported for Config Entities.
      // @todo Remove when supported https://www.drupal.org/node/2300677
      $unsupported_methods = ['POST', 'PUT', 'DELETE', 'PATCH'];
      $methods = array_diff($methods, $unsupported_methods);
    }
    return $methods;
  }

  /**
   * Checks if this resource is for a Config Entity.
   *
   * @return bool
   *   TRUE if the entity is a Config Entity, FALSE otherwise.
   */
  protected function isConfigEntityResource() {
    return $this->entityType instanceof ConfigEntityType;
  }

So let's transplant that logic to JSON API. The logical place in JSON API is ResourceTypeRepository, which creates immutable ResourceType value objects. That repository service already has a isLocatableResourceType() method, the result of which is stored in the ResourceType value object, its result accessible via ResourceType::isLocatable().
So the logical solution is to add ResourceTypeRepository::isValidatableResourceType() and ResourceType::isValidatable(). This can then trivially be overridden by JSON API Extras!

This patch adds that infrastructure. But doesn't actually use it yet.

The first way we can use the infrastructure that #9 adds is to keep routes the same, but check it in the controller.

The second way we can use the infrastructure that #9 adds is to modify the routes, which results in the routing system giving us 405 responses automatically. I personally prefer this, because there's fewer moving parts, we rely on the routing system to do what it's meant to do, and it keeps routes declarative.

I personally prefer #11, what do others prefer?

Both approaches make it super easy for JSON API Extras to override this:

diff --git a/src/ResourceType/ConfigurableResourceTypeRepository.php b/src/ResourceType/ConfigurableResourceTypeRepository.php
index 135b42f..5e110bb 100644
--- a/src/ResourceType/ConfigurableResourceTypeRepository.php
+++ b/src/ResourceType/ConfigurableResourceTypeRepository.php
@@ -3,6 +3,7 @@
 namespace Drupal\jsonapi_extras\ResourceType;
 
 use Drupal\Core\Entity\EntityTypeBundleInfoInterface;
+use Drupal\Core\Entity\EntityTypeInterface;
 use Drupal\Core\Entity\EntityTypeManagerInterface;
 use Drupal\Core\Entity\EntityRepositoryInterface;
 use Drupal\jsonapi\ResourceType\ResourceTypeRepository;
@@ -84,6 +85,13 @@ class ConfigurableResourceTypeRepository extends ResourceTypeRepository {
   /**
    * {@inheritdoc}
    */
+  protected static function isValidatableResourceType(EntityTypeInterface $entity_type) {
+    return TRUE;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
   public function all() {
     if (!$this->all) {
       foreach ($this->getEntityTypeBundleTuples() as $tuple) {

The second way we can use the infrastructure that #9 adds is to modify the routes, which results in the routing system giving us 405 responses automatically.

#11 is absolutely my preference also.

+++ b/src/ResourceType/ResourceType.php
@@ -196,6 +203,19 @@ class ResourceType {
+  public function isValidatable() {
...
+

I feel like this isn't the right method name as it leaks a rather Drupal-specific implementation detail. We shouldn't really need to ask this question because all entities have a validate() method, we just happen to know that it doesn't really work for some entity types.

I'd rather see something more generic, like isLocatable() and isMutable(). Validation should just be an internal precondition to being "mutable".

+++ b/src/Routing/Routes.php
@@ -222,6 +222,11 @@ class Routes implements ContainerInjectionInterface {
+    if (!$resource_type->isValidatable()) {
+      $routes->setMethods(['GET']);
+    }

I'd rather not see this override the existing HTTP methods and instead just set the correct methods at the get-go. This makes things easier to reason about.

I think you forgot to exclude POST on the collection route.

#11 is absolutely my preference also.

🤘

I feel like this isn't the right method name as it leaks a rather Drupal-specific implementation detail.

1. ResourceType isn't a public API, so it shouldn't matter anyway.
2. ResourceType::isLocatable() is then similarly problematic
3. ResourceType::getEntityTypeId() is then even worse

I'd rather see something more generic, like isLocatable() and isMutable().

isMutable() works for me, I even considered that actually :D Will rename.

I'd rather not see this override the existing HTTP methods and instead just set the correct methods at the get-go. This makes things easier to reason about.

WFM!

I think you forgot to exclude POST on the collection route.

😅

That was sloppy, sorry. 😳

Updated patch has zero mentions of validatable. 👍

1. ResourceType isn't a public API, so it shouldn't matter anyway.
2. ResourceType::isLocatable() is then similarly problematic
3. ResourceType::getEntityTypeId() is then even worse

1. Resource type mirrors concepts to the specification, we should do the best we can I think :)
2. I thought locatable was actually quite nice. It's very spec-ish. URI vs. URL
3. I agree, let's fix that too then (2.x is our chance). Not here ofc.

+++ b/src/Routing/Routes.php
@@ -140,7 +140,11 @@ class Routes implements ContainerInjectionInterface {
+    $methods = $resource_type->isLocatable() ? ['GET', 'POST'] : ['POST'];
+    if (!$resource_type->isMutable()) {
+      $methods = array_intersect($methods, ['GET']);
+    }
+    $collection_route->setMethods($methods);

Nit: this seems rather roundabout. First you add it then you remove it if something is not TRUE. What about:

$methods = $resource_type->isLocatable() ? ['GET'] : [];
$methods[] += $resource_type->isMutable() ? ['POST'] : [];

+++ b/src/Routing/Routes.php
@@ -197,7 +201,7 @@ class Routes implements ContainerInjectionInterface {
-    $individual_route->setMethods(['GET', 'PATCH', 'DELETE']);
+    $individual_route->setMethods($resource_type->isMutable() ? ['GET', 'PATCH', 'DELETE'] : ['GET']);

👌

1. You're right.
2. Right again!
3. 👍 (Not sure we can really change that, but def out of scope.)

Nit: this seems rather roundabout. First you add it then you remove it if something is not TRUE. What about:

😳 OF COURSE 😳

Created JSON API Extras issue to restore support for mutating config entities, includes patch: #2982664: Enable mutating of config entities in JSON API 2.x.

CR created: https://www.drupal.org/node/2982668.

+++ b/src/Routing/Routes.php
@@ -140,10 +140,10 @@ class Routes implements ContainerInjectionInterface {
+    $methods = array_merge(
+      $resource_type->isLocatable() ? ['GET'] : [],
+      $resource_type->isMutable() ? ['POST'] : []
+    );

😘👌

Crediting everyone who participated in #2887313: JSON API indicates it supports POST/PATCH/DELETE of config entity types, but that's impossible.