Cache route by Uri and not just Query+Path

+1

Based on the test failure error message, I think it's likely there's a test that's hard-coded to the old logic that we will need to update.

Otherwise, I am +1 on this change.

The one possible downside is that, if domain access is installed, it will result in a considerable increase in the number of cache entries for the routing cache. However, that is the goal here because we DO need the cache to be different in at least some of those cases. For non-DA sites I don't expect it to have any measurable impact.

I'm good here. Thanks, Ken!

This will also affect sites with mixed http/https, http://www and http://, and domain language negotiation won't it?

Looks like a good case for #2551419: Abstract RenderCache into a separate service that is capable of cache redirects in a non-render array-specific way to me - then we'd be able to modify the cid only on sites that actually need it. Not saying we necessarily need to postpone on that, but I'd like to see a @todo and confirm whether the other cases I mentioned would also be affected at least.

Yes, I guess this will result in more cache variants on some sites, but not redirecting www/non-www is a bad practice anyway and I guess we'll see more and more https only sites that redirect http://, with letsencrypt and so on.

I guess this is OK.

However, note that I think it should be relatively easy to solve in domain.module itself. It just needs to make sure that the response has the url.site context, which would result in exactly the same behavior. I guess we could argue that since Drupal by default doesn't care about the domain, it would be the responsibiity of a module that changes that to ensure the right cache contexs?

+++ b/core/modules/system/src/Tests/Routing/RouteProviderTest.php
@@ -454,31 +454,31 @@ public function testRouteCaching() {
-    $path = '/path/add/one';
+    $path = 'http://example.com/path/add/one';
     $request = Request::create($path, 'GET');

This is a bit confusing. Is this really how a request would look like. Would the the path in Request::create() be actually absolute?

@berdir:

I am not sure I can follow how to do that, in \Drupal\Core\Routing\RouteProvider\RouteProvider::getRouteCollectionForRequest() the cache ID is hard-coded to

$cid = 'route:' . $request->getPathInfo() . ':' . $request->getQueryString();

so I don't see how we can inject the url.site context here? If there would be an API for appending an optional context (like the url.site), the problem should be pretty straightforward to fix in the Domain module, but currently I don't see a way for other modules to add the context to the route cache.

Yeah, sorry, I thought this was about dynamic page cache somehow... misread. Which is the next thing you might run into once this is fixed. But *that* should be fixed by making sure that cache context is added.

Re #12, that seems OK to me. Request::create() uses parse_url() and if there's a host, then it sets the server variables accordingly.

Based on that, setting back to RTBC.

The one possible downside is that, if domain access is installed, it will result in a considerable increase in the number of cache entries for the routing cache.

99% of sites are not accessible over multiple domains, so the actual cost (or rather, reduction in efficiency) will remain lower.

Looks like a good case for #2551419: Abstract RenderCache into a separate service that is capable of cache redirects in a non-render array-specific way

+1

And just generally a good reason for this to use cache contexts instead, which are Drupal's standardized mechanism for this. https://www.drupal.org/developing/api/8/cache/contexts

so I don't see how we can inject the url.site context here? If there would be an API for appending an optional context (like the url.site), the problem should be pretty straightforward to fix in the Domain module, but currently I don't see a way for other modules to add the context to the route cache.

Exactly! The key problem here is that it's hardcoded and cannot be manipulated, even though routing itself can be manipulated. That is the bug.

So given #10 + #13: I think the ability to change the cache contexts by which routes are cached would be the better solution. Executing an alter hook on every request is probably too expensive. But what about a container parameter? Just like we have:

parameters:
  renderer.config:
    required_cache_contexts: ['languages:language_interface', 'theme', 'user.permissions']

we could also have:

parameters:
  routing:
    cache_contexts: ['url.path', 'url.query_args']

And then the domain module could alter that container parameter when it is installed.

Thoughts?

Sounds like a flexible enough solution. Just to be clear, we should name it something which includes routing and inbound path processing,which is the actual thing, which is dynamic here, not the routing itself.

Could domain manipulate that list via code, rather than manually? Right now it documents adding a required render cache context manually. I'd hate to make Domain need to have even more manual configuration. (If it can add both automatically, so much the better.)

Could domain manipulate that list via code, rather than manually?

Of course, using \Drupal\Core\DependencyInjection\ServiceModifierInterface. That's why I said And then the domain module could alter that container parameter when it is installed..

#19 :) :) :)

OT: the README in that branch/repo says: Edit the required_cache_contexts to add domain-awareness, as indicated by the url.site parameter. — that should not even be necessary. See #18. Look at \Drupal\language\LanguageServiceProvider::alter() for an example.

#15

But what about a container parameter? [...] And then the domain module could alter that container parameter when it is installed.

That sounds sweet to me :-)

I've implemented the \Drupal\Core\DependencyInjection\ServiceModifierInterface as suggested by Wim in #18.

If we run into conflicts with other modules that want to interact with the routing, we might need to sort out a different approach.

Have a look here:
https://github.com/agentrickard/domain/pull/209

Got a build failure applying the last patch to 8.1.x, so here's a re-roll

This blocks @agentrickard's domain module.

For core itself for now IMHO using the entire URI is quite good. Of course its not the ideal solution, but its fixing an existing bug out there.

I didn't document very well why I started applying this patch to a multilingual site, but if I recall correctly this issue also can cause problems for domain based language negotiation, where the same path alias exists in multiple languages.

+1

I think the core change to cache route by URI (patch in #26) is a more simple approach rather than letting Domain implement \Drupal\Core\DependencyInjection\ServiceModifierInterface - even though we can work it out in both ways.

Stumbled on this issue when trying to change the front page using the front page like this:

services:
  frontpage.overrider:
    class: \Drupal\qeado_dashboard\FrontpageOverrides
    tags:
      - {name: config.factory.override, priority: 5}

<?php
/**
 * @file
 * Contains \Drupal\qeado_dashboard\FrontpageOverrides.
 */

namespace Drupal\qeado_dashboard;

use Drupal\Core\Cache\CacheableMetadata;
use Drupal\Core\Config\ConfigFactoryOverrideInterface;
use Drupal\Core\Config\StorageInterface;

/**
 * Provides a override for the frontpage uri based on property count.
 */
class FrontpageOverrides implements ConfigFactoryOverrideInterface {
  public function loadOverrides($names) {
    $overrides = array();

    if (in_array('system.site', $names) && \Drupal::currentUser()->isAuthenticated()) {
      $query = \Drupal::entityQuery('property');
        $query->condition('uid', \Drupal::currentUser()->id());
        $properties = $query->execute();

      // Count properties.
      if ($properties) {
        if (count($properties) == 1) {
          $property_id = current($properties);
          $overrides['system.site'] = [
            'page' => ['front' => '/property/' . $property_id],
          ];
        }
        else {
          $overrides['system.site'] = [
            'page' => ['front' => '/user/' . \Drupal::currentUser()->id()],
          ];
        }
      }
    }

    return $overrides;
  }

  /**
   * {@inheritdoc}
   */
  public function getCacheSuffix() {
    return 'FrontpageOverrider-' . \Drupal::currentUser()->id();
  }

  /**
   * {@inheritdoc}
   */
  public function getCacheableMetadata($name) {
    $metadata = new CacheableMetadata();

    if ($name == 'system.site') {
      $metadata->setCacheContexts(['user']);
      $metadata->setCacheTags(['property_list']);
    }

    return $metadata;
  }

  /**
   * {@inheritdoc}
   */
  public function createConfigObject($name, $collection = StorageInterface::DEFAULT_COLLECTION) {
    return NULL;
  }

}

Also got stuck because of getRouteCollectionForRequest() for trying this method. I think the fix for me would be to create a KernelEvents::REQUEST subscriber which changes the $request->getPathInfo(). Or a dummy frontpage controller which sends back the proper frontpage.

Added some related issue: #2769421: Allow Inbound PathProcessor's to specify that they should not be cached

The original patch is a really simple solution to the problem. Maybe we should add a follow up with the suggestion of @Wim Leers in #2662196-15: Cache route by Uri and not just Query+Path, but make it easier for domain module.

Let's also correct the used tag. As @agentrickard said, this is not really blocking domain module from working, its just not nice :)

Re-roll for 8.2.x

Only change appears to be the path of the test class.

Created #2799543: Cache route by configured cache contexts as a followup feature request against 8.3.x

We have a separate issue to provide a more generic solution, for now this fixes a concrete problem in the wild.

Still not convinced that the quick fix here justifies potentially double the cache entries for mixed http/https sites.

Well, then let's do it properly and mark this issue as duplicate?

Fair enough, if someone feels really strongly we should do this, they can always re-open.