Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
There's no test coverage for this cache context
Proposed resolution
Add a test like \Drupal\Tests\Core\Cache\Context\QueryArgsCacheContextTest
but for HeadersCacheContext
. That test was added in #2729439: QueryArgsCacheContext should return a special value for ?arg (without value).
Remaining tasks
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#29 | 2738563-29.patch | 3.67 KB | andypost |
#29 | interdiff.txt | 644 bytes | andypost |
#9 | 2738563-9-testonly.patch | 2.42 KB | andypost |
Comments
Comment #2
Wim LeersIt's not a follow-up to #2729439: QueryArgsCacheContext should return a special value for ?arg (without value). It's just similar.
Comment #3
Wim LeersComment #4
Wim LeersI think this is actually a good novice issue.
Comment #5
andypostSuppose something like that
Comment #6
Wim LeersThis 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 thefoo
header does not exist.Overall, I'd like to see this match the thinking in
\Drupal\Tests\Core\Cache\Context\QueryArgsCacheContextTest
more closely.Comment #7
andypost@Wim so that means we need another test (functional) cos that's what the class does
Comment #8
Wim LeersNo, that can happen in a unit test just fine.
Comment #9
andypostThat's really a bug because
headers->all()
returns array but context should be a stringAlso I'm sure we need to
ksort()
headers to minimize cache variationsComment #12
andypostRemoved debug code (
Comment #14
Wim LeersMuch, much better! :)
Just a few nits and then this is ready:
I'd change this to:
Let's use an
else
here, just like inQueryArgsCacheContext
.Comment #15
andypostI disagree
1) context should return
NULL
if no headers, not empty string2) no reason to wrap in unconditional else here
I'd better clean-up
QueryArgsCacheContext
cos this unconditionalelse {return ..}
just makes harder to read codeComment #16
Wim LeersIt must be a string per the interface:
Remember, it's going to be used to create a cache ID, which is by definition a string.
Comment #17
andypostI'll take another round today
We using this cache variation for rest endpoint to vary custom key (key in set vs wrong key
@Wim looks variation should be configurable, any other idea of usage?
Comment #18
andypostI mean there's 2 ways
1 presence of key
2 variation by key value
Is this different context like headers.x-custom
When value is headed:x-custom
Comment #19
andypostI mixed @return and @param so now fixed
Looks
QueryArgsCacheContext
and test should be updated accordinglyEDIT filed #2744421: QueryArgsCacheContext::getContext() may return NULL, violates the interface
Comment #20
andypostProbably it makes sense to sort values within same name to minimize cache variations
Comment #21
Wim LeersLooking great! Just one small thing:
I think this is a bit confusing.
I think this would be clearer:
Because that leaves you with an overall structure of:
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:
That'd be even better :) Please add test coverage for that then.Thanks for helping to make this much better! :)
Comment #22
Wim LeersThis is what I'm asking for in #21:
Comment #23
Wim LeersDiscussed 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.
Comment #24
andypostHere it is! Added value sorting
Comment #25
andypost@Wim please check the patch, it looks still applicable
Comment #26
andypostComment #27
Wim LeersSorry, this completely fell off my radar!
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 :)Comment #29
andypostHere it goes!
Comment #31
Wim LeersPerfect!
Comment #32
andypostBugfixes could go to betas
Comment #35
catchCommitted/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!
Comment #37
Wim LeersYay, a more reliable & more thoroughly tested cache system!
Comment #39
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commentedThis 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.
Comment #40
andypost@jp.stacey as 8.3 user I'm using the patch because without it the cache context is broken
Comment #41
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commentedThanks, @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.