REST Resource config entities do not respect the status (enabled/disabled)

Should this be looked at from the greater config entity perspective? Should all config entities be disable-able?

Confirmed. Great catch!

Should all config entities be disable-able?

That's already the case: \Drupal\Core\Config\Entity\ConfigEntityInterface::enable().

But of course the REST module long predates config entities. This was overlooked in #2308745: Remove rest.settings.yml, use rest_resource config entities.

It already had a comment // Iterate over all enabled REST resource configs.… but that didn't reflect reality sadly. It also still had // Iterate over all enabled resource plugins., which should have been removed in #2308745: Remove rest.settings.yml, use rest_resource config entities.

Once #2815845: Importing (deploying) REST resource config entities should automatically do the necessary route rebuilding we'll be able to easily test this: we will be able to change EntityResourceTestBase::deprovisionResource() to no longer delete the REST Resource config entity, but instead disable it. And in provisionResource() we can then create it if it does not exist, but if it already exists, just call enable(). That's exactly what we need to test here!

I just tried to write some tests, no idea whether they fail or pass.

IMHO we need to both disable and delete.

This blocks #2851126: The UI says "disable", but it's really "delete".

#2815845: Importing (deploying) REST resource config entities should automatically do the necessary route rebuilding landed, this is now unblocked!

Rebased @dawehner's proposal of #10. (#10 no longer applies.)

1. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -131,17 +131,25 @@ public function setUp() {
   +    $resource_config->set('config', [
   

   Should be 'configuration'.

2. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -131,17 +131,25 @@ public function setUp() {
   +    $this->resourceConfigStorage->save($resource_config);
   

   Should be $resource_config->save().

Those explain the key failures.

Also, I think it doesn't make sense we allow modifying all those configuration details when we just need to be able to disable a resource. It'd be more logical to add a method to be able to retrieve the REST Resource Config entity then, so then you can call disable(), enable() and delete() on it. The complication is that you must always call refreshTestStateAfterRestConfigChange() after any of those, to keep the test environment in sync. But that's okay.

So, in this patch, I am:

1. restoring ResourceTestBase::provisionResource() and provisionEntityResource() to their original signatures
2. then removing the $resource_type parameter from ResourceTestBase::provisionResource(), and instead moving that to static $resourceConfigId
3. this then allows us to do $this->resourceConfigStorage->load(static::$resourceConfigId)->disable()->save(); to disable, $this->resourceConfigStorage->load(static::$resourceConfigId)->enable()->save(); to re-enable and $this->resourceConfigStorage->load(static::$resourceConfigId)->delete(); to save
4. hence I can now also delete this->deprovisionResource()

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -353,6 +347,22 @@ public function testGet() {
     $this->provisionEntityResource();
+    $this->resourceConfigStorage->load(static::$resourceConfigId)->disable()->save();
+    $this->refreshTestStateAfterRestConfigChange();
+
+
+    // The resource is still provisioned, but disabled.
+    $response = $this->request('GET', $url, $request_options);
+    if ($has_canonical_url) {
+      $this->assertResourceErrorResponse(403, '', $response);
+    }
+    else {
+      $this->assertResourceErrorResponse(404, 'No route found for "GET ' . str_replace($this->baseUrl, '', $this->getUrl()->setAbsolute()->toString()) . '"', $response);
+    }
+
+
+    $this->resourceConfigStorage->load(static::$resourceConfigId)->enable()->save();
+    $this->refreshTestStateAfterRestConfigChange();
     // Simulate the developer again forgetting the ?_format query string.
     $url->setOption('query', []);

This is what @dawehner proposed in #10. It's well-intended, but it's not testing what we want to test.

We want to first verify that the provisioned resource has a 200 response. THEN we want to disable it and verify that we get a 404. Then if we re-enable again, we want a 200. And then if we delete it, we again want a 404.

It's important that we verify those 200 responses in between, because only then we're actually testing that routes are working (200 response) vs not working (404 response) at the expected times!

So, keeping this exact same test code, but moving it after our first successfully verified 200 response!

We now have:

1. 200
2. disable
3. 404
4. enable
5. delete
6. 404

We still need to add an assertion of a 200 response between "enable" and "delete". This does that. Test coverage is now complete.

Now the test coverage (and its failing tests) make it clear that the bug reported in the IS is not yet fixed.

So, time to fix that. This interdiff is identical to my patch in #6.

Finally, there were some small flaws in the assertions that check the behavior after disabling. We should just copy/paste the assertions we already had for a deleted resource.

Also make the comments consistent.

This should be green :) And hopefully RTBC'able! :)

Clearer.

1. +++ b/core/modules/rest/src/Routing/ResourceRoutes.php
   @@ -59,13 +59,14 @@ public function __construct(ResourcePluginManager $manager, EntityTypeManagerInt
   -      $resource_routes = $this->getRoutesForResourceConfig($resource_config);
   -      $collection->addCollection($resource_routes);
   +      if ($resource_config->status()) {
   +        $resource_routes = $this->getRoutesForResourceConfig($resource_config);
   +        $collection->addCollection($resource_routes);
   +      }
   

   Very clear that we aren't adding routes for disabled resources!

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -137,22 +137,13 @@
   -  protected function deprovisionEntityResource() {
   -    $this->resourceConfigStorage->load('entity.' . static::$entityTypeId)
   -      ->delete();
   -    $this->refreshTestStateAfterRestConfigChange();
   

   Removal of deprovisionEntityResource makes things simpler!

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -481,10 +475,34 @@ public function testGet() {
   +    // DX: upon disabling a resource, immediate 404 if no route, 406 otherwise.
   +    $response = $this->request('GET', $url, $request_options);
   +    if (!$has_canonical_url) {
   +      $this->assertSame(404, $response->getStatusCode());
   +    }
   +    else {
   +      $this->assert406Response($response);
   +    }
   

   We have this code block now twice.
   Could we refactor to
   protected function assertRouteNotAvailable($url, $request_options)
   No need to pass $has_canonical_url has that can be figured out.

Other than the refactor suggestion I think this could be RTBC.

Tests just passed yay!

Yay! :) Done.

Didn't go with assertRouteNotAvailable but with assertResourceNotAvailable, because it's about the resource, not the route (a single resource has many routes).

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -1154,4 +1142,24 @@ protected function assert406Response(ResponseInterface $response) {
+  }
+
+
 }

Extra line that can be removed on commit.

Great! RTBC!

Forgot a bit.

And fixing the nit in #28.

@Wim Leers sorry I missed the missing arguments in #27. I guess that is why we have tests ;)

Checked the test failures and were caused by this. Tests passed in #29 after fix. and #30

Still RTBC!

+++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
@@ -56,6 +56,15 @@
   /**
+   * The REST Resource Config entity ID under test (i.e. a resource type).
+   *
+   * The REST Resource plugin ID can be calculated from this.
+   *
+   * @var string
+   */
+  protected static $resourceConfigId = NULL;

@@ -123,24 +132,23 @@ public function setUp() {
-  protected function provisionResource($resource_type, $formats = [], $authentication = []) {
+  protected function provisionResource($formats = [], $authentication = []) {

I've stared at this for a bit and thought is this change really necessary. Cause at the moment this fix is eligible for 8.3.1 cause 8.3.x is frozen till the release of 8.3.0. This means there will be a time when the cascade of classes involved in entity resource REST testing are out of sync between 8.3.x and 8.4.x if I commit this now. Which might prove costly for writing patches during this time.

I'm not sure what to do here. If @Wim Leers or @tedbow feel that they want to get this in to 8.4.x now and are happy to live with the out-of-syncness I'm happy to go along with that. Since they are working on most of the issues that touch this area.

That shouldn't hurt us much. We're rarely touching the "provision resource" code. This is likely the last time.

If this can still land in 8.3.1, that means that this problem is also very temporary. If this could never land in 8.3, this wouldn't even be that big of a problem.

Committed b739f56 and pushed to 8.4.x. Thanks!

Thanks @Wim Leers for addressing #33.

Porting to 8.3.

Well, actually a simple git cherry-pick 6d2c634ea1baf081c2092b757ae54ccf908c2a54 seems to work fine :)

I just realized alexpott probably only set it to "Patch (to be ported)" so that he can cherry-pick it into 8.3.x once 8.3.0 is out. Oops :)

@catch, @cilefen and @xjm and I just decided that patch rules will apply to the RC up until RC2 so backporting this to 8.3.x

Committed 8b622f8 and pushed to 8.3.x. Thanks!

Even better, thanks!