Headers cache context does not work, also needs test coverage

It's not a follow-up to #2729439: QueryArgsCacheContext should return a special value for ?arg (without value). It's just similar.

I think this is actually a good novice issue.

+++ b/core/tests/Drupal/Tests/Core/Cache/Context/HeadersCacheContextTest.php
@@ -0,0 +1,49 @@
+    $this->assertSame($cache_context->getContext($header_name), $context);
...
+      [NULL, NULL, NULL],
+      ['x-custom-header', NULL, NULL],
+      ['x-custom-header', '123', '123'],
+      ['x-custom-header', ['123'], '123'],

This is only testing the case of looking at one particular header, rather than many. It's also not testing the case of headers.foo, where the foo header does not exist.

Overall, I'd like to see this match the thinking in \Drupal\Tests\Core\Cache\Context\QueryArgsCacheContextTest more closely.

No, that can happen in a unit test just fine.

Much, much better! :)

Just a few nits and then this is ready:

1. +++ b/core/lib/Drupal/Core/Cache/Context/HeadersCacheContext.php
   @@ -25,11 +25,29 @@ public static function getLabel() {
   +      $result = NULL;
   +      foreach ($headers as $name => $value) {
   +        if ($result) {
   +          $result .= '&';
   +        }
   +        else {
   +          $result = '';
   +        }
   

   I'd change this to:

   $result = '';
   foreach (…) {
     if ($result) {
       $result .= '&';
     }
   

2. +++ b/core/lib/Drupal/Core/Cache/Context/HeadersCacheContext.php
   @@ -25,11 +25,29 @@ public static function getLabel() {
   +      }
   +      return '?valueless?';
   

   Let's use an else here, just like in QueryArgsCacheContext.

1) context should return NULL if no headers, not empty string

It must be a string per the interface:

  /**
   * Returns the string representation of the cache context.
   *
   * A cache context service's name is used as a token (placeholder) cache key,
   * and is then replaced with the string returned by this method.
   *
   * @return string
   *   The string representation of the cache context.
   */
  public function getContext();

Remember, it's going to be used to create a cache ID, which is by definition a string.

Looking great! Just one small thing:

+++ b/core/lib/Drupal/Core/Cache/Context/HeadersCacheContext.php
@@ -25,11 +25,26 @@ public static function getLabel() {
+    elseif ($this->requestStack->getCurrentRequest()->headers->has($header)) {
+      $value = $this->requestStack->getCurrentRequest()->headers->get($header);
+      if ($value !== '') {
+        return $value;
+      }
+      return '?valueless?';
     }
+    return '';

I think this is a bit confusing.

I think this would be clearer:

else {
  if (has header) {
    …
  }
  else {
    return '';
  }
}

Because that leaves you with an overall structure of:

if ($header === NULL) {
  …
}
else {
  …
}

Which clearly reflects there are two possible high-level cases: either you list all headers, or only the specific header that you care about.

That's less clear in the current code.

#20: Probably it makes sense to sort values within same name to minimize cache variations That'd be even better :) Please add test coverage for that then.

Thanks for helping to make this much better! :)

This is what I'm asking for in #21:

  public function getContext($header = NULL) {
    if ($header === NULL) {
      $headers = $this->requestStack->getCurrentRequest()->headers->all();
      // Order headers by name to have less cache variations.
      ksort($headers);
      $result = '';
      foreach ($headers as $name => $value) {
        if ($result) {
          $result .= '&';
        }
        $result .= $name . '=' . implode(',', $value);
      }
      return $result;
    }
    else {
      if ($this->requestStack->getCurrentRequest()->headers->has($header)) {
        $value = $this->requestStack->getCurrentRequest()->headers->get($header);
        if ($value !== '') {
          return $value;
        }
        return '?valueless?';
      }
      else {
        return '';
      }
    }
  }

Discussed with @andypost in IRC. He doesn't think this is clearer. I know it's a personal preference, so never mind that part. That just leaves the #20 remark to be addressed, then this is RTBC.

Sorry, this completely fell off my radar!

+++ b/core/tests/Drupal/Tests/Core/Cache/Context/HeadersCacheContextTest.php
@@ -0,0 +1,53 @@
+      // Values are sorted to minimize cache variations.
+      [['z' => ['1', '0'], 'a' => []], NULL, 'a=&z=0,1'],

I think you should provide this by adding another line that has 'z' => ['0', '1'], and show that it results in the same 'a=&z=0,1'. That'd make it conclusive :)

Perfect!

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Yay, a more reliable & more thoroughly tested cache system!

This is great to see this go in for 8.4+, so thanks for all your work: but can I confirm for the sake of 8.3 users that this is actually both increasing test coverage, *and* changing running code?

I've had people ask about headers cache context in 8.3 and wanted to be able to give them a coherent answer. The patch in #29 does seem to change the behaviour of HeadersCacheContext.php , not just increase test coverage.

If so it might be useful to change the title of this issue, so that it more clearly reflects what was done.

Thanks, @andypost, and thanks again for your work on this issue. I've changed the title now so it's clearer to people looking for this problem.