When mapping cache contexts to cache keys, include the cache context ID for easier debugging

Hi I just found this issue. Please forgive the naive questions:

1. Does CacheContext order matter? Some implementations sort their contexts, some append a new context to the end of a list. There are a lot the send a static value.

2. Does #2428563-20: Introduce parameter-dependent cache contexts Primarily discuss a change to the CacheContext tests?

3. What are the next steps here?

AFAICT this is about this bit in #2428563-20: Introduce parameter-dependent cache contexts:

+++ b/core/tests/Drupal/Tests/Core/Cache/CacheContextsTest.php
@@ -17,20 +19,19 @@
+      'foo',
+      'baz:parameter',
+    ]);
-    $expected = ['bar'];
+    $expected = ['bar', 'baz.cnenzrgre'];
     $this->assertEquals($expected, $new_keys);

It confuses me that I put in baz then ':' and receive baz and '.'.

Should we not standardize that?

e.g. it might be needed (not sure where) to calculate from a CID back to cache contexts.

There's no need to standardize: what matters is that we get back a string, that somehow uniquely identifies this specific cache context. If we're going to also impose a strong structure on the value of the cache context, we're A) not gaining anything noteworthy, B) making things more complex.

What matters is that given a certain cache context, you get back a certain string of data. That's it. As long as that string makes sense within the context (heh…) of that cache context, that's fine.

That final bit, about calculating the cache contexts from a CID, I think a small desire to be able to do that, is what caused this confusion (and this very issue). But there is no way that that is possible. It's perfectly plausible that two different cache contexts return the exact same value. e.g.
#cache => ['keys' => ['foo', 'bar'], 'contexts' => ['color_of_the_day', 'user.favorite_color']] may very well result in a CID of foo:bar:blue:blue. How are you ever going to know which belongs to which?

#1:

1. No, cache contexts are always a set.
2. IDK, see above.
3. IDK, see above.

AFAICT, it's safe to close this. Feel free to reopen if I was wrong.

Yes, that hugely clarifies things.

Will post a patch tomorrow

Unit tests pass, a targeted selection of integration tests pass. But quite possibly there will still be some failures. Let's see what testbot has to say!

Lets ensure using a database inspector does show those [] cases and that we don't need additional escaping

That's indeed a boon for DX. Manually checked, and confirmed that this works without any nasty surprises.

FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: failed to clear checkout directory.

Testbot, you're really driving me insane.

FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: failed to clear checkout directory.

Re-testing again.

Ahh, finally sane failures :) Fixing!

Should be green & RTBC'able.

This issue is a task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

Beta evaluation added. Sorry for not having done it already!

Rebased. No conflicts.

Rebased. Trivial conflict with #2511472: Refactor all usages of drupal_render()/Renderer::render() that break #2450993.

The change makes a lot of sense but the beta evaluation needs work since DX is not a permitted reason for a change.

Well, it's not only about DX, right, but also site-builder understability / debuggability of what happens on the render cache ?
Also, disruption is null anyway ?

Anyway, big +1 on that change :-)

This is a straight reroll.

Well, it's not only about DX, right, but also site-builder understability / debuggability of what happens on the render cache ?

I'd argue that's DX? :) How many site builders are going to analyze the render cache? Aren't they developers at that point?

Also, disruption is null anyway ?

Yes, with the exception if very very minor disruption in tests (as the patch shows, and the beta evaluation indicates).

Anyway, big +1 on that change :-)

Yay :)

#2493033: Make 'user.permissions' a required cache context landed yesterday and caused the conflicts resulting in the test failure.

But! It actually fixed things in a generic way, so the patch here simply becomes smaller :)

Conflicted with #2351015: URL generation does not bubble cache contexts, which landed yesterday. Easy conflict resolution.

Committed ba1b14e and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation.

Updated https://www.drupal.org/developing/api/8/cache/contexts accordingly.

/**
   * {@inheritdoc}
   */
  public function getContext($role = NULL) {
    // User 1 does not actually have any special behavior for roles; this is
    // added as additional security and backwards compatibility protection for
    // SA-CORE-2015-002.
    // @todo Remove in Drupal 9.0.0.
    if ($this->user->id() == 1) {
      return 'is-super-user';
    }
    if ($role === NULL) {
      return implode(',', $this->user->getRoles());
    }
    else {
      return (in_array($role, $this->user->getRoles()) ? '0' : '1');
    }
  }

Is this function really suppose to return a 0 if $role is in the user roles? Because it produces stranges things like user.roles:authenticated=1 for anonymous pages.