Symfony\Component\HttpFoundation\ParameterBag supports nested query parameters, like ?foo[bar][baz].

For a query string like ?foo[bar]=baz calling ParameterBag->get('foo'); will return array('bar' => 'baz). This will cause a PHP notice when retrieving the cache context specific to 'foo' since QueryArgsCacheContext::getContext is expected to return a string and will instead return an array.

CommentFileSizeAuthor
#13 interdiff-2746761-11-13.txt1.79 KBgabesullice
#13 queryargscache_context-2746761-13.patch1.68 KBgabesullice
#11 interdiff-2746761-10-11.txt835 bytesgabesullice
#11 queryargscache_context-2746761-11.patch1.68 KBgabesullice
#10 interdiff-2746761-9-10.txt1.17 KBgabesullice
#10 queryargscache_context-2746761-10.patch1.87 KBgabesullice
#9 interdiff-2746761-3-9.txt2.39 KBgabesullice
#9 queryargscache_context-2746761-9.patch2.15 KBgabesullice
#3 interdiff-2746761-2-3.txt676 bytesgabesullice
#3 queryargscache_context-2746761-3.patch2.44 KBgabesullice
#2 queryargscache_context-2746761-2.patch2.34 KBgabesullice
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gabesullice created an issue. See original summary.

gabesullice’s picture

gabesullice
Primary language English
CreditAttribution: gabesullice at Aten Design Group commented
Issue summary: View changes
FileSize
queryargscache_context-2746761-2.patch2.34 KB

The attached patch eliminates this notice.

This patch also lets one specify 'foo[bar]' as the query arg to getContext. This would mean the following query string would be seen as the same as far as caching is concerned.

?foo[bar]=one&foo[baz]=two // context == 'one'
?foo[bar]=one&foo[baz]=three // context == 'one'

If the query arg were simply 'foo' then:

?foo[bar]=one&foo[baz]=two // context == md5(serialize(['bar' => 'one', 'baz' => 'two']))
?foo[bar]=one&foo[baz]=three // context == md5(serialize(['bar' => 'one', 'baz' => 'three']))
gabesullice’s picture

gabesullice
Primary language English
CreditAttribution: gabesullice at Aten Design Group commented
FileSize
queryargscache_context-2746761-3.patch2.44 KB
interdiff-2746761-2-3.txt676 bytes

Added another test.

gabesullice’s picture

gabesullice
Primary language English
CreditAttribution: gabesullice at Aten Design Group commented
Title: QueryArgsCache context does not support nested parameters » QueryArgsCacheContext does not support nested parameters
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Version: 8.2.x-dev » 8.1.x-dev
Priority: Normal » Major
Status: Needs review » Needs work
Issue tags: +D8 cacheability

This is a bug, and should be fixed in 8.1. Thanks for filing this and working on it!

+++ b/core/lib/Drupal/Core/Cache/Context/QueryArgsCacheContext.php
@@ -24,19 +24,25 @@ public static function getLabel() {
+        if (is_array($value)) {
+          return md5(serialize($value));
+        }

This is not ideal. #2738563: Headers cache context does not work, also needs test coverage has code to deal with nested parameters. This can use the exact same code. I think that'd be better.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2738563: Headers cache context does not work, also needs test coverage
gabesullice’s picture

gabesullice
Primary language English
CreditAttribution: gabesullice at Aten Design Group commented

@Wim Leers thanks for the review!

I took a deeper look at ParameterBag::get() and found that the 'deep' argument will be deprecated, so I've removed that from this patch.

I also took a look at the HeaderCacheContext code, but I'm not sure it applies as cleanly as could be hoped. A header can have multiple values, but as far as I can tell, it cannot have nested values. I.E. one can't have an array of arrays as is the case with query parameters. For example, if one were to have the querystring foo[bar][baz]=nested1&foo[bar][zap]=nested2 then get('foo') would return: ['bar' => ['baz' => 'nested1, 'zap' => 'nested2']].

I've updated the serialization to be 'human-readable' by using json_encode instead of serialize and md5. I didn't use the HeaderCacheContext code because I couldn't think of a good way to implement it recursively any more efficiently than json_encode can do for use 'out-of-the-box'.

gabesullice’s picture

gabesullice
Primary language English
CreditAttribution: gabesullice at Aten Design Group commented
Status: Needs work » Needs review
gabesullice’s picture

gabesullice
Primary language English
CreditAttribution: gabesullice at Aten Design Group commented
FileSize
queryargscache_context-2746761-9.patch2.15 KB
interdiff-2746761-3-9.txt2.39 KB
gabesullice’s picture

gabesullice
Primary language English
CreditAttribution: gabesullice at Aten Design Group commented
FileSize
queryargscache_context-2746761-10.patch1.87 KB
interdiff-2746761-9-10.txt1.17 KB

Since we're not doing deep parameters, I reverted most of the function back to its original code.

gabesullice’s picture

gabesullice
Primary language English
CreditAttribution: gabesullice at Aten Design Group commented
FileSize
queryargscache_context-2746761-11.patch1.68 KB
interdiff-2746761-10-11.txt835 bytes

Code standards

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented 
+++ b/core/lib/Drupal/Core/Cache/Context/QueryArgsCacheContext.php
@@ -29,7 +29,10 @@ public function getContext($query_arg = NULL) {
+        return json_encode($value);

Wouldn't http_build_query() be a better fit for this?

gabesullice’s picture

gabesullice
Primary language English
CreditAttribution: gabesullice at Aten Design Group commented
FileSize
queryargscache_context-2746761-13.patch1.68 KB
interdiff-2746761-11-13.txt1.79 KB

Derp.

Wim Leers’s picture

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

Perfect!

Nice to see this is able to incrementally improve upon #2729439: QueryArgsCacheContext should return a special value for ?arg (without value), which was where test coverage for this class was first introduced.

Also impressive how we first delegated to Symfony's logic, and now actually have to do a lot of gymnastics on top of it. I clearly failed to think the edge cases through originally.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 33cd4e3 and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed 4cff4e2 on 8.2.x 
    Issue #2746761 by gabesullice, Wim Leers: QueryArgsCacheContext does not...

  • alexpott committed 33cd4e3 on 8.1.x 
    Issue #2746761 by gabesullice, Wim Leers: QueryArgsCacheContext does not...

Status: Fixed » Closed (fixed)

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