Make CachePluginBase::generateResultsKey() to pass valid data to CacheContextsManager::convertTokensToKeys

The only thing I can imagine that its NULL sometimes ...

      $this->resultsKey = $this->view->storage->id() . ':' . $this->displayHandler->display['id'] . ':results:' . hash('sha256', serialize($key_data));

at least this piece of code will always return in a string

This makes QueryArgsCacheContext::getContext() always return a string. This would also fix a problem with query strings like '?value[0]=a&value[1]=b', see #2501905-7: \Drupal\views\Plugin\views\argument_default\QueryParameter should specify a more specific cache context.

Thanks @Aki Tendo and @olli!

The contents of this patch seem radically out of scope for the title of the issue, so let's create a patch that just fixes the bug. A simple unit test or such would be preferable for test coverage.

Also, the issue needs a beta evaluation.

Here's a test.

1. +++ b/core/lib/Drupal/Core/Cache/Context/QueryArgsCacheContext.php
   @@ -28,11 +28,12 @@ public static function getLabel() {
   +      return $query_string === NULL ? '' : $query_string;
   ...
   +    return $value === NULL ? '' : http_build_query([$query_arg => $value], '', '&');
   

   What about just casting that into a string.

2. +++ b/core/tests/Drupal/Tests/Core/Cache/Context/QueryArgsCacheContextTest.php
   @@ -0,0 +1,64 @@
   +    $container_prophecy = $this->prophesize('Symfony\Component\DependencyInjection\ContainerInterface');
   +    $container_prophecy->get('cache_context.url.query_args')->willReturn(new QueryArgsCacheContext($request_stack));
   

   <3

Thanks for the review @dawehner. Casting seems to work too.

1. +++ b/core/lib/Drupal/Core/Cache/Context/QueryArgsCacheContext.php
   @@ -30,11 +30,12 @@ public static function getLabel() {
   +      $query_string = $this->requestStack->getCurrentRequest()->getQueryString();
   +      return (string) $query_string;
   

   removed $query_string variable.

2. +++ b/core/lib/Drupal/Core/Cache/Context/QueryArgsCacheContext.php
   @@ -30,11 +30,12 @@ public static function getLabel() {
   +    return (string) http_build_query([$query_arg => $value], '', '&');
   

   No need to cast here.

Nice test coverage!

+++ b/core/tests/Drupal/Tests/Core/Cache/Context/QueryArgsCacheContextTest.php
@@ -0,0 +1,64 @@
+    $container_prophecy = $this->prophesize('Symfony\Component\DependencyInjection\ContainerInterface');

Just for future reference, you can now use ::class

+++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
@@ -107,6 +107,8 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
@@ -124,7 +126,6 @@ public function convertTokensToKeys(array $context_tokens) {

@@ -124,7 +126,6 @@ public function convertTokensToKeys(array $context_tokens) {
       list($context_id, $parameter) = $context;
       $keys[] = $this->getService($context_id)->getContext($parameter);
     }
-
     // Create the returned object and merge in the cacheability metadata.
     $context_cache_keys = new ContextCacheKeys($keys);
     return $context_cache_keys->merge($cacheable_metadata);

@Aki Tendo: Did you mean to add an assert for $keys here? I think that's what failed in #1.

Fixed the test and reverted .htaccess changes.

Just for future reference, you can now use ::class

That is nice, thank you.

Reroll.

Found this via #2729439: QueryArgsCacheContext should return a special value for ?arg (without value).

1. +++ b/core/lib/Drupal/Core/Cache/CacheableMetadata.php
   @@ -128,6 +128,8 @@ public function merge(CacheableMetadata $other) {
   +    assert('$result instanceof \\Drupal\\Core\\Cache\\CacheableMetadata');
   

   I think this assertion is quite pointless, because $result = clone $this;, so that's always true.

2. +++ b/core/lib/Drupal/Core/Cache/Context/CacheContextsManager.php
   @@ -107,6 +107,8 @@ public function getLabels($include_calculated_cache_contexts = FALSE) {
   +    assert('\\Drupal\\Component\\Assertion\\Inspector::assertAllStrings($context_tokens)');
   
   +++ b/core/lib/Drupal/Core/Cache/Context/ContextCacheKeys.php
   @@ -28,6 +28,7 @@ class ContextCacheKeys extends CacheableMetadata {
   +    assert('Drupal\\Component\\Assertion\\Inspector::assertAllStrings($keys)');
   

   These assertions OTOH are very valuable!

3. +++ b/core/lib/Drupal/Core/Cache/Context/QueryArgsCacheContext.php
   @@ -30,11 +30,11 @@ public static function getLabel() {
   +      return (string) $this->requestStack->getCurrentRequest()->getQueryString();
   

   This is the bugfix the issue summary/title state.

4. +++ b/core/lib/Drupal/Core/Cache/Context/QueryArgsCacheContext.php
   @@ -30,11 +30,11 @@ public static function getLabel() {
   +    return http_build_query([$query_arg => $value], '', '&');
   

   I don't think this is a good idea. \Drupal\Core\Cache\Context\CacheContextsManager::convertTokensToKeys() is already ensuring a clear key-value pair.

   Let's not change this.

Thanks for your review!

4. We need that (in addition to 3) to always get a string from QueryArgsCacheContext::getContext(). Without it, both '?value[min]=1' and '?value[min]=2' for 'url.query_args:value' evaluate to 'Array' with a notice 'Array to string conversion' from CacheContextsManager::convertTokensToKeys().

Oh, interesting. Fine, let's do some conversion from array to string, but let's not repeat $query_arg.

Also, #2729439: QueryArgsCacheContext should return a special value for ?arg (without value) just landed, which already added QueryArgsCacheContextTest. That means this patch's test will need to be refactored.

The related issues fixed now it needs work and reroll

This was a bugsmash daily target. I ran the test that is in the patch and it fails, so I assumed this still needed to be done. larowlan confirmed that in slack.

This also needs an issue summary update, see Write an issue summary for an existing issue for guidance.

Trying to improve the title

Adding Rerole for d10, As previous patch failed to apply on d10 and also most of the files already had the changes so there is minor change in the patch.

Regarding #46

also most of the files already had the changes

How so? I'm checking D10.1 and I don't see the changes for a few of those files. Also this lost test coverage so adding that tag back

Was previously tagged for issue summary which still needs to happen as well.

I've altered the existing test to support the new format brought by $cache_context->getContext()

Issue summary section for solution is missing.

Tagging for submaintainer to make sure they agree on the approach

Those asserts are not doing anything, see MR comment.

I feel like #27.1 was never addressed. My assumption is also that if CacheableMetadata::merge() has $result = clone $this;, then it seems kind of moot to assert($result instanceof CacheableMetadata);.

Comment #28 also touched on the ampersand but not sure if we really need that.

From a cache submaintainer point of view, I see nothing wrong here. Assertions are harmless, casting casche context return values to strings is too because worst case scenario that leads to a few cache entries no longer being reachable, at which point they get recalculated and stored again. Only been subsystem maintainer for a few months now, so will ask @catch before removing tag.

This looks fine conceptually, still NW per my latest comment, though.