system_page_attachments() varies by authenticated user role but does not add said cache context

Using patch because it's really easy to prove.

Oh and if it helps, this got introduced here: #2759859: Implement getDrupalSettings() on BrowserTestBase for checking JS settings →
And it doesn't occur when testing in the browser manually, so it could be theme related.

The last submitted patch, 2: drupal-3379220-2.patch, failed testing. View results →

Catch found the culprit (from system_page_attachments()):

  if (\Drupal::currentUser()->isAuthenticated()) {
    $page['#attached']['library'][] = 'core/drupal.active-link';
  }

So now we need to add user.roles:anonymous to the renderer.config.required_cache_contexts because that was clearly an oversight. As soon as anon/auth can get the same permission hash, as is the case with my new system, auth could see a cache result without any JS. If we add the cache context for anonymous, this would no longer be possible.

For the record: The correct behavior in stark is apparently that no JS shows up, but because other themes add more JS this does not hold true for them. So my test to prove the issue here got the ball rolling, but is now useless. We should instead write one test (perhaps add to BrowserTestBaseTest0 that first checks for the absence of drupal settings for anon and then the presence for auth.

Added a MR that fixes this by adding $page['#cache']['contexts'][] = 'user.roles:authenticated'; to system_page_attachments, which should have been there from the start.

Ugh, created from wrong branch. Moving version number.

All green now, should probably rename this issue.

Also updating the IS accordingly

Added a regression test that obviously goes green under the current circumstances (as core masked the bug), but should go red without this fix once we try to continue work on 📌 Implement the new access policy API Needs work . This ensures that we do not regress in the future as then said test will go red.

Fixed up issue summary a bit

Ran the test-only feature

1) Drupal\Tests\system\Functional\Routing\RouterTest::testFinishResponseSubscriber
Behat\Mink\Exception\ExpectationException: Current response header "X-Drupal-Cache-Contexts" is "languages:language_interface theme url.query_args:_wrapper_format user.permissions", but "languages:language_interface theme url.query_args:_wrapper_format user.permissions user.roles:authenticated" expected.
/builds/issue/drupal-3379220/vendor/behat/mink/src/WebAssert.php:794
/builds/issue/drupal-3379220/vendor/behat/mink/src/WebAssert.php:161
/builds/issue/drupal-3379220/core/modules/system/tests/src/Functional/Routing/RouterTest.php:58
/builds/issue/drupal-3379220/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!
Tests: 14, Assertions: 92, Errors: 1.

Going to lean on that and since the tests are green and the change is just adding the cache tag think this is good.

Two questions:

1. Why not do this?

So now we need to add user.roles:anonymous to the renderer.config.required_cache_contexts because that was clearly an oversight.

2. user.roles.anonymous and user.roles.authenticated feel like they are the same cache context, should we try to standardise on one, and if so which?

Opened 📌 Explore if tags user.roles.anonymous and user.roles.authenticated can be combined Active for discussion about the tags.

Re #13:

1. It turns out that only HtmlRenderer runs hook_page_attachments() and therefore it might not make sense that we add a cache context to renderer.config.required_cache_contexts as that would also affect things we render that might not see system attach this library and therefore cache context.
2. We should indeed discuss this in 📌 Explore if tags user.roles.anonymous and user.roles.authenticated can be combined Active

What a find! 🤯👏

I'm especially surprised due to \Drupal\Tests\system\Functional\Common\NoJavaScriptAnonymousTest::assertNoJavaScript() existing.

Are they the same though?

They are — they basically are each other's inverse. When one is TRUE, the other is FALSE, and vice versa.

I guess the ideas in 📌 Allow non-intrinsic (implementation-dependent) cache context services to specify their parents via DynamicParentContextInterface::getParents() Needs work could allow to optimize one of the two away. not sure if it needs a separate issue or what that would even do. they are both valid roles, if anything we'd need to explicitly deprecate/disallow both and introduce a new user.is_anonymous context, but is that really worth it?

Adjusted both comments to follow Wim's suggestions, setting back to RTBC as no functional code changed.

#18++ — I forgot about that! Thanks, @Berdir!

RTBC++

I'm triaging RTBC issues → . I read the IS, the comments and the MR. I didn't find any unanswered questions or other work to do.

Leaving at RTBC.

Commited/pushed to 11.x and cherry-picked to 10.2.x, thanks!

Awesome, thanks!

Automatically closed - issue fixed for 2 weeks with no activity.