This came up as a review point from #2784921-95: Add Workspaces experimental module.25:
+++ b/core/modules/workspace/src/WorkspaceCacheContext.php @@ -0,0 +1,53 @@ + /** + * {@inheritdoc} + */ + public function getContext() { + return $this->workspaceManager->getActiveWorkspace(); + } ... + public function getCacheableMetadata($type = NULL) { + return new CacheableMetadata(); + }Shouldn't the workspace negotiation inform the system how they determined this value? For example the session one depend on the current active session.
Discuss whether we need to do this and implement the solution.
TBD.
Nope.
Probably.
Nope.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | interdiff-9.txt | 1.47 KB | amateescu |
| #9 | 2934354-9.patch | 2.1 KB | amateescu |
| #4 | 2934354.patch | 649 bytes | amateescu |
Comments
Comment #2
fabianx commentedYes, it should.
The reason why there is a getCacheableMetadata() method on CacheContexts is that those are living in a kind of hierarchy, e.g. user permissions is a good thing. You can cache that per user / session, but if you remove it you need to add the cache tags of the user.roles so if those change the permissions hash is updated.
e.g. think of it like when the cache context needs to be cached by something else. (e.g. in Varnish per url or per user session).
Comment #3
amateescu commentedComment #4
amateescu commented@Fabianx, thanks for the detailed explanation. Given your last example and the fact that the active workspace will always end up in the user's session (e.g. workspaces do no make sense for anonymous users), I think this patch should be enough.
Comment #5
amateescu commentedComment #6
timmillwoodIn the future we may add more active workspace negotiators, and contrib (such as Relaxed module) will also add these. Therefore, would it make sense for the negotiators to be able to set the cache context? In other words, should we add a
getCacheContexts()method the negotiators?Comment #7
fabianx commented#6 It probably makes sense - yes.
The PR for now looks good to me however.
Comment #8
alexpottI can't see a reason why we would have test coverage for this?
Comment #9
amateescu commentedAdded a couple of assertions to check that the cache context used by
WorkspaceCacheContextis emitted properly.Comment #10
amateescu commentedComment #11
timmillwoodLooks good.
Comment #12
wim leers👌
Comment #13
plachSaving credit
Comment #14
plachCommitted 7a61525 and pushed to 8.6.x. Thanks!
Comment #17
amateescu commentedFix component following module rename.