Importing (deploying) REST resource config entities should automatically do the necessary route rebuilding

WFM

#4: You're doing a lot of tests with REST resources?

1. +++ b/core/modules/rest/src/Entity/RestResourceConfig.php
   @@ -263,4 +264,13 @@ protected function normalizeRestMethod($method) {
   +  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
   +    parent::postSave($storage, $update);
   +
   +    \Drupal::service('router.builder')->setRebuildNeeded();
   +  }
   

   Oh, interesting! I thought this would need a \Drupal\Core\Config\ConfigEvents::SAVE subscriber.

   Is this better? Why? Should we document that?

   (Asking for the patch, but also because I'd genuinely like to know.)

2. The patch is failing, for example in \Drupal\dblog\Tests\Rest\DbLogResourceTest.

   This is why:

   +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -411,15 +411,12 @@ protected function enableService($resource_type, $method = 'GET', $format = NULL
   -    $this->rebuildCache();
   ...
   -    // Rebuild routing cache, so that the REST API paths are available.
   -    $this->container->get('router.builder')->rebuild();
   

   I'm afraid we really need this router to be rebuilt too, because otherwise the test cannot access the router to generate URLs to access.

3. We still need a BrowserTestBase test that tests as a regular user: making config changes causes route changes to be available on the next request.

I think the rule might be like the following

Sounds good.

Let's see whether conditional rebuilds might help.

+1

Better, but not there yet.

Fails with Call to a member function getResourcePlugin() on a non-object, only for POST and PATCH (so GET and DELETE work fine). This means that routing to RequestHandler::handle() works, but this results in no entity being loaded:

    $resource_config_id = $route_match->getRouteObject()->getDefault('_rest_resource_config');
    /** @var \Drupal\rest\RestResourceConfigInterface $resource_config */
    $resource_config = $this->resourceStorage->load($resource_config_id);
    $resource = $resource_config->getResourcePlugin();

Interesting.

Rebased.

Since #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method, there's another place where we need to remove the forced rebuilding: ResourceTestBase.

This is what also needed to be removed per #14.

Note: We need to invalidate page_cache && dynamic_page_cache on a router rebuild.

REST resource responses have config:rest.resource.* cache tags, corresponding to the RestResourceConfig entities. But that doesn't help with cached 4xx responses, and that's what's going on here.

Wouldn't the more elegant solution therefore be to ensure that all 4xx responses for REST have the RestResourceConfig list cache tag?

I just remembered that emptying Page Cache & Dynamic Page Cache explicitly is not an acceptable solution, because the same cannot be done for reverse proxies.

So, whatever we do, it must only use cache tag-based invalidation.

Conceptually for me the route_match cache tag should be on every response. Technically route rebuilding could have arbitrary sideeffects.

That's a good point. So should we add a cache tag for that to all responses then?

#24: exactly.

We've used the rendered cache tag for that purpose, but it only works for HTML responses. This new cache tag would work for all responses.

Perhaps we want an even more generic cache tag? For example, drupal? There are other reasons to invalidate all responses, such as deploying new modules, in which case route_match is a poor name.

#26: sounds good!

#29: patch is looking good. Just two remarks for now:

1. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -135,11 +135,17 @@ public function onRespond(FilterResponseEvent $event) {
   +    // Always add the 'http_response' cache tag to be able to invalidate every
   +    // response.
   +    $response->getCacheableMetadata()->addCacheTags(['http_response']);
   

   Shouldn't this also be done for responses to subrequests?

2. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -135,11 +135,17 @@ public function onRespond(FilterResponseEvent $event) {
   -      $response->headers->set('X-Drupal-Cache-Tags', implode(' ', $response_cacheability->getCacheTags()));
   +      $tags = $response_cacheability->getCacheTags();
   +
   +      $response->headers->set('X-Drupal-Cache-Tags', implode(' ', $tags));
   

   This change looks like a debugging leftover?

So close!

This is looking excellent. I have only nitpicks basically.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -89,6 +89,21 @@ public function __construct(LanguageManagerInterface $language_manager, ConfigFa
   +   * Sets extra headers on any responses, also subprocess ones.
   

   on any response, also subrequest ones.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -89,6 +89,21 @@ public function __construct(LanguageManagerInterface $language_manager, ConfigFa
   +    // response.
   

   …, for example after rebuilding routes.

3. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -284,6 +299,8 @@ protected function setExpiresNoCache(Response $response) {
   +    // Execute before the onRespond method.
   +    $events[KernelEvents::RESPONSE][] = array('onAllResponds', 16);
   

   But why 16? Why not 7, or 77? Let's document the rationale for this number.

4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -196,9 +196,6 @@ public function setUp() {
   -    // @todo Remove this in https://www.drupal.org/node/2815845.
   -    drupal_flush_all_caches();
   

   <3

5. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php
   @@ -37,6 +38,14 @@
   +  public function setUp() {
   +    parent::setUp();
   +
   +    // We don't want to ship with a node resource by default for those tests.
   +    RestResourceConfig::load('entity.node')->delete();
   +  }
   

   This can be deleted, \Drupal\Tests\rest\Functional\ResourceTestBase::setUp() is already taking care of that :)

   It does this:

       // Ensure there's a clean slate: delete all REST resource config entities.
       $this->resourceConfigStorage->delete($this->resourceConfigStorage->loadMultiple());
   

6. +++ b/core/modules/system/src/Tests/Entity/EntityCacheTagsTestBase.php
   @@ -401,6 +401,7 @@ public function testReferencedEntity() {
   +    debug($referencing_entity_cache_tags);
   

   This is a debug leftover.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -299,7 +299,8 @@ protected function setExpiresNoCache(Response $response) {
   +    // Execute before the onRespond method. We went with prio 16 as we want to
   +    // run before static::onRespond().
   

   This "16" is still a mystery to me.

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -446,7 +446,7 @@ public function testGet() {
   -  public function testPost() {
   +  public function ptestPost() {
   
   @@ -640,7 +640,7 @@ public function testPost() {
   -  public function testPatch() {
   +  public function ptestPatch() {
   
   @@ -846,7 +846,7 @@ public function testPatch() {
   -  public function testDelete() {
   +  public function ptestDelete() {
   

   Oops :)

3. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -130,6 +130,9 @@ public function setUp() {
   +    // If we remove resources we also need to ensure they don't stick in the
   +    // router.
   +    \Drupal::service('router.builder')->rebuild();
   

   Why do we need this now? :( Because the router used by the test itself is otherwise outdated?

4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php
   @@ -4,6 +4,7 @@
   +use Drupal\rest\Entity\RestResourceConfig;
   

   This can now also be deleted :)

One solution would be to reset the internal states, which happens in core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:211 as well.

I think that's the only solution then.

I'd create a new issue and add a TODO to both places that use this hack that points to that issue. That new issue would say something like make \Drupal\Core\Cache\DatabaseCacheTagsChecksum work in all situations, not just a per-request situation. What do you think? I'd be happy to create that issue for you.

Actually, reading the explanation + interdiff in #49 again and also reading my concerns/criticism about that in #50 again, makes me disagree with myself.

I agree with @dawehner's assessment in #51:

well, to be honest our test is executing code both on a production system as well as on the test instance. This is the main issue we deal with.

There's no way around that.

But I do think we can make that a bit more clear:

1. +++ b/core/modules/rest/src/Entity/RestResourceConfig.php
   @@ -255,4 +256,22 @@ protected function normalizeRestMethod($method) {
   +  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
   +    parent::postSave($storage, $update);
   +
   +    \Drupal::service('router.builder')->setRebuildNeeded();
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function postDelete(EntityStorageInterface $storage, array $entities) {
   +    parent::postDelete($storage, $entities);
   +
   +    \Drupal::service('router.builder')->setRebuildNeeded();
   +  }
   

   These are the critical changes: these ensure that routes are rebuilt when necessary :)

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -196,9 +196,6 @@ public function setUp() {
   -    // @todo Remove this in https://www.drupal.org/node/2815845.
   -    drupal_flush_all_caches();
   

   Hurray!

3. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -130,6 +130,9 @@ public function setUp() {
   +    // If we remove resources we also need to ensure they don't stick in the
   +    // {router} table.
   +    \Drupal::service('router.builder')->rebuild();
   
   @@ -152,8 +155,10 @@ protected function provisionResource($resource_type, $formats = [], $authenticat
   -    // @todo Remove this in https://www.drupal.org/node/2815845.
   -    drupal_flush_all_caches();
   +    // Ensure that the cache tags invalidator has its internal values reset.
   +    // Otherwise the http_response cache tag invalidation won't work.
   +    $this->refreshVariables();
   +    \Drupal::service('router.builder')->rebuild();
   

   Let's rephrase this so that it's clear that we need to rebuild this solely for the purpose of the route building we do _inside_ the test (i.e. the tester, not the testee).

4. +++ b/core/modules/rest/src/Tests/RESTTestBase.php
   @@ -420,8 +420,7 @@ protected function enableService($resource_type, $method = 'GET', $format = NULL
   -    // Rebuild routing cache, so that the REST API paths are available.
   -    $this->container->get('router.builder')->rebuild();
   +    $this->container->get('router.builder')->rebuildIfNeeded();
   

   This one is oddly inconsistent with the changes in ResourceTestBase. This indicates we should also be able to update ResourceTestBase to use rebuildIfNeeded(). Doing so locally shows it works.

I addressed all of my own feedback. IMO it can be RTBC'd now. But it still needs a CR (working on that now).

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

Cool :)

RTBC then. I've been reviewing this patch, and dawehner just reviewed the sole interdiff I posted. (#13 was a straight rebase, #15 was just removing the @todo that pointed to this issue. @dawehner did all the work.)

Thanks for the extra diligence, @tedbow! I can't believe I missed those additional @todos back in #15.

Modifying rest.settings' bc_entity_resource_permissions causes routes to change. The changes in RestResourceConfig::(post|pre)save trigger a route rebuild when REST Resource Config entities are modified. But when rest.settings.bc_entity_permissions is modified, we also need to trigger a route rebuild!

All the remaining @todos are about rest.settings.bc_entity_permissions.

First, let's prove that what I wrote here is actually true. This should fail: this should result in 200 responses instead of 403 responses because routes are not being rebuilt and hence the BC entity permissions are not required, which is why it's resulting in 200 responses instead of 403.

This adds a ConfigEvents::SAVE subscriber that calls setRebuildNeeded() so that the router is rebuilt also when rest.settings.bc_entity_permissions is modified.

This should be green again.

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -395,8 +393,7 @@ public function testGet() {
        $this->config('rest.settings')->set('bc_entity_resource_permissions', TRUE)->save(TRUE);
   -    // @todo Remove this in https://www.drupal.org/node/2815845.
   -    drupal_flush_all_caches();
   +    $this->refreshTestStateAfterRestResourceConfigChange();
   

   The first line here is not modifying a REST Resource Config entity. It's modifying rest.settings, which is just Simple Configuration.

   Hence the name of the method called on the last line is not a great match.

2. +++ b/core/modules/rest/tests/src/Functional/ResourceTestBase.php
   @@ -141,8 +142,23 @@ protected function provisionResource($resource_type, $formats = [], $authenticat
   +   * Should be called after every RestResourceConfig entity change.
   +   */
   +  protected function refreshTestStateAfterRestResourceConfigChange() {
   ...
   +    // Tests using this base class may trigger route rebuilds due to changes to
   +    // RestResourceConfig entities. Ensure we generate routes using an
   +    // up-to-date router.
   

   IOW: these comments and the method name should be modified.

Addressed that.

@tedbow pointed out in chat that this is not yet addressed:

But for postDelet() what confirms the route is being rebuilt after deleting a REST resouce? Is this important?

At the end of each of the test*() methods in we could \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase delete the resource and confirm we get a 404.

Of course he is right.

\Drupal\Tests\rest\Functional\ResourceTestBase::setUp()does this:

    // Ensure there's a clean slate: delete all REST resource config entities.
    $this->resourceConfigStorage->delete($this->resourceConfigStorage->loadMultiple());

This deletes the default REST resource config entities: the one for node. So we’re already testing it, just not explicitly.

Let's do this right.

Done.

Actually, that 406 vs 403 behavior was encountered before, and there already is a helper method to deal with that. This behavior is being fixed in #2805279: Routing system + authentication system + format-specific routes (e.g. those in rest.module) = frustrating, unhelpful 403 responses instead of 406 responses.

This is now also blocking #2844046: REST Resource config entities do not respect the status (enabled/disabled).

This is also causing problems in the REST UI contrib module. It blocks the following fixes:

1. #2851127: REST UI module should not be rebuilding routes
2. #2851126: The UI says "disable", but it's really "delete"

Created this follow-up: #2851414: Many RouteBuilder::setRebuildNeeded() calls only occur when using the UI, not when using the configuration system. (It's not so much a follow-up to this issue as a realization that this issue triggered.)

I got #2844046: REST Resource config entities do not respect the status (enabled/disabled) and #2851127: REST UI module should not be rebuilding routes going again :)

And #2851126: The UI says "disable", but it's really "delete" now has one less blocker!