QueryArgsCacheContext should return a special value for ?arg (without value)

Instead of :empty:, I'd suggest using a value that you cannot actually send in a query argument in any sane way. Such as ?empty?, or ?valueless?.

So, this basically?

Now 90% less stupid.

This blocks #2688545: Remove ampNodeViewController in favor of amp_entity_view_alter().

And test coverage. Which showed that the 90% less stupid was accurate: it was not 100% less stupid. This is.

#2509436: Make CachePluginBase::generateResultsKey() to pass valid data to CacheContextsManager::convertTokensToKeys looks related, patch #25 would return 'arg=' for ?arg.

As written in IRC, this can't work as you still have the old return in place ;)

GAHHHHHHHHHH. My computer is acting seriously weird: https://twitter.com/wimleers/status/733340751665504256. I was testing to verify that tests were failing as expected with the old behavior. And now that ended up in the proper patch. Sigh.

Fixing.

Green now, so random fail indeed.

Berdir pointed out in IRC this can be even simpler.

I think this is now really ready for RTBC.

Worth noting that if somebody only needs the query argument with no value (?somearg), this patch will create a cache variation if ?somearg is present, versus it not being present. However it will also create cache variations if somebody adds a value to that arg, e.g. ?somearg=1, ?somearg=2 ... ?somearg=30000. If those arg values are meaningless, that's a lot of extra cache variations that aren't necessary.

One option to address that would be to create an additional cache context, something like ['query_empty_arg:argname'], with something like the following:

  public function getContext() {
    return (bool) $this->requestStack->getCurrentRequest()->query->has($argname);
  }

That would ensure that if somebody wants to explicitly set up an argument that will never have a value, that any values entered will be ignored.

I'd suggest doing that in addition to the above patch, not instead of, because the current behavior prior to this patch can be pretty confusing when an empty arg doesn't cause any cache variations.

If those arg values are meaningless, that's a lot of extra cache variations that aren't necessary.

I'd suggest doing that in addition to the above patch, not instead of,

I'm not convinced this is actually necessary, because you can always bypass caches (including even Page Cache and Varnish!) by adding random URL query arguments.

I personally don't want to make cache contexts more complex than necessary for little or no benefit. But, if a convincing argument can be made, then sure, but let's do that in a different issue then :)

My concern is not that adding values to query args (that are supposed to be empty) allows you to bypass cache. The issue is that somebody could spam a site with lots of requests with meaningless arg values and bloat up the cache in the db with unnecessary cache variations.

adding values to query args (that are supposed to be empty) allows you to bypass cache.

and

The issue is that somebody could spam a site with lots of requests with meaningless arg values and bloat up the cache in the db with unnecessary cache variations.

are the same thing!

I should've worded it better. You're bypassing the cache in that the existing cache entries are already exactly what you want. But because Drupal cannot ever know that additional query arguments won't cause different variations, Page Cache will always create a new cache entry. So even just with drupal.org?random=<random number between 1 and 1 billion>, you can easily DDoS any Drupal site. This is common knowledge. It's been around for more than a decade. And it's true for just about any CMS out there.

Fair point. Let's just get this in then.

+++ b/core/lib/Drupal/Core/Cache/Context/QueryArgsCacheContext.php
@@ -27,8 +27,8 @@ public function getContext($query_arg = NULL) {
-    else {
-      return $this->requestStack->getCurrentRequest()->query->get($query_arg);
+    elseif ($this->requestStack->getCurrentRequest()->query->has($query_arg)) {
+      return $this->requestStack->getCurrentRequest()->query->get($query_arg) ?: '?valueless?';
...
   }

This no longer always has an explicit return point. Relying on a default return of NULL seems to "work" here but isn't a good practice and can end up looking like a bug later. Not something we should be in the habit of doing.

#25: heh, this is exactly what @Berdir suggested :P Dammit!

Fixed that nit.

Just to say #23 is right, there's no way around this. #2697795: [meta] Cache storage in database can fill the disk, especially via 404s and related issues discuss potentially lowering the ttl for unbounded cache contexts in order to protect a bit against disk filling though - since it can affect the render cache generally as well as page cache.

+++ b/core/lib/Drupal/Core/Cache/Context/QueryArgsCacheContext.php
@@ -27,8 +27,11 @@ public function getContext($query_arg = NULL) {
+    elseif ($this->requestStack->getCurrentRequest()->query->has($query_arg)) {
+      return $this->requestStack->getCurrentRequest()->query->get($query_arg) ?: '?valueless?';

'?value=0' would return '?valueless?'?

You're right. Great catch.

Adds test coverage proving #28 is right, which means the prior patches still had an uncovered edge case.

PHP--
PHP--
PHP--

And here's the fix.

Random fail in migrate:

fail: [Other] Line 139 of core/modules/migrate_drupal_ui/src/Tests/MigrateUpgradeTestBase.php:

I feel bad asking this, but if I was looking through cache keys and saw ?valueless? I'd probably wonder where that came from. Would ?null? be more self-explanatory? If the answer is 'no' that's fine - leaving at RTBC.

I don't care. I found "empty" very confusing, so I went with "valueless". If you prefer "null", that works for me.

Meh I also don't care.

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

If this ever is a problem for someone, it's only the value of the context, so could be changed whenever.

Also you might infer from null that the query argument isn't there - i.e. that we're logging it's absence, not that it's there but without a value. So valueless works for that. It's an !isset() vs. !array_key_exists() problem.

it's only the value of the context, so could be changed whenever.

Yep.

Thanks for the commit!

Filed related issue for headers context #2738563: Headers cache context does not work, also needs test coverage

Also that needs follow-up for values that could be array, I'm using "," to split them

Here's another follow-up #2744421: QueryArgsCacheContext::getContext() may return NULL, violates the interface
The result of \Drupal\Core\Cache\Context\QueryArgsCacheContext::getContext() should not be *NULL*