Follow-up to #2729439: QueryArgsCacheContext should return a special value for ?arg (without value)

Problem/Motivation

Return value could be NULL after #2729439: QueryArgsCacheContext should return a special value for ?arg (without value)

Proposed resolution

Check result of \Symfony\Component\HttpFoundation\Request::getQueryString() and convert it to empty string when NULL

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#12 2744421-12.patch2.78 KBandypost
#12 interdiff.txt391 bytesandypost
#9 2744421-9.patch2.82 KBandypost
#8 2744421-8.patch2.6 KBandypost
#8 interdiff.txt1.09 KBandypost
#6 2744421-6.patch2.6 KBandypost
#6 interdiff.txt1.08 KBandypost
#3 2744421-2.patch2.35 KBandypost
#2 2744421-2-testonly.patch1.34 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Active » Needs review
FileSize
2744421-2-testonly.patch1.34 KB
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
2744421-2.patch2.35 KB

And test+fix

The last submitted patch, 2: 2744421-2-testonly.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: QueryArgsCacheContext::getContext() should respect interface » QueryArgsCacheContext::getContext() may return NULL, violates the interface
Status: Needs review » Needs work

I'd RTBC this, but the first remark in #2738563-21: Headers cache context does not work, also needs test coverage also applies here. I know it's a pre-existing bit of lack of clarity here, but if we're touching this code, we might as well make the code more legible too.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
interdiff.txt1.08 KB
2744421-6.patch2.6 KB

I think this change makes code more readable

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
interdiff.txt1.09 KB
2744421-8.patch2.6 KB

Proper comments

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
2744421-9.patch2.82 KB

Re-roll after #2746761: QueryArgsCacheContext does not support nested parameters

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Cache/Context/QueryArgsCacheContext.php
@@ -35,13 +38,10 @@ public function getContext($query_arg = NULL) {
+    // No requested argument found.

This comment is wrong. It's also not helpful. Let's remove it.

Once that is fixed, this is RTBC.

The last submitted patch, 9: 2744421-9.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs work » Needs review
FileSize
interdiff.txt391 bytes
2744421-12.patch2.78 KB

Here's it, bot failures looks unrelated

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

Perfect, thanks!

  • catch committed 63b7ded on 8.2.x 
    Issue #2744421 by andypost, Wim Leers: QueryArgsCacheContext::getContext...

  • catch committed 0a27b6c on 8.1.x 
    Issue #2744421 by andypost, Wim Leers: QueryArgsCacheContext::getContext...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Status: Fixed » Closed (fixed)

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