Problem/Motivation

Blocks #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!), causes 6 test failures there.

Proposed resolution

Specify proper cacheability for the session_test module's routes/controllers: Most of the routes should not be cached, and the session_test.id_from_cookie route needs to have the cookie ID as a context.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#1 2512734-1.patch3.47 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
3.47 KB
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine, just changes of tests, we have "test coverage" in #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!), but we can commit this separately to keep that patch there smaller.

dawehner’s picture

Specify proper cacheability for the session_test module's routes/controllers.

Well the patch doesn't look like that. We seem to opt out a lot here

Wim Leers’s picture

Well the patch doesn't look like that. We seem to opt out a lot here

Indeed. Because that's the proper course of action here, since these routes/controllers are all just printing session IDs.

dawehner’s picture

Well, should we not specify the session ID as cache context then? I mean its about test coverage and giving good examples for people.

xjm’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Discussed with @Wim Leers, @dawehner, and @Berdir. The summary confused me a little, but all we are doing here is specifying that most of the routes in the session test should not be cached, and that the session_test.id_from_cookie route needs to have the cookie ID as a context.

We discussed @dawehner's suggestion, but that would essentially just add test coverage that the cache metadata bubbling works, which is not the purpose of the test.

This issue only changes test code, so it is unfrozen during the beta per https://www.drupal.org/core/beta-changes, and it is a prioritized change as a bugfix and blocker for #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!). Committed and pushed to 8.0.x. Thanks!

  • xjm committed 782dfb3 on 8.0.x
    Issue #2512734 by Wim Leers, dawehner, Berdir: session_test routes/...
xjm’s picture

Issue tags: +D8 Accelerate London

Status: Fixed » Closed (fixed)

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