X-Drupal-Cache-Contexts and X-Drupal-Cache-Tags headers should not indicate emptiness by the empty string as header value

The IRC discussion we had:

11:57:58 alexpott: WimLeers: I don;t get the changes in FinishResponseSubscriber in https://www.drupal.org/node/2453341
11:58:00 Druplicon: https://www.drupal.org/node/2453341 => Contact forms don't have necessary cache contexts & tags; flood control should work on validate, not on view #2453341: Contact forms don't have necessary cache contexts & tags; flood control should work on validate, not on view => 35 comments, 2 IRC mentions
12:01:32 WimLeers: alexpott: in that patch, we encounter for the very first time a completely empty X-Drupal-Cache-Tags header. We missed that in https://www.drupal.org/node/2458349.
12:01:34 Druplicon: https://www.drupal.org/node/2458349 => Route's access result's cacheability not applied to the response's cacheability #2458349: Route's access result's cacheability not applied to the response's cacheability => 36 comments, 5 IRC mentions
12:02:11 WimLeers: alexpott: explode(' ', $empty_string) does not yield the expected result (an empty array), so we're then calling Cache::mergeTags() with something other than an array, causing a fatal
12:02:39 WimLeers: alexpott: so, we change the →has() to a →get(), so that we avoid doing that work if the header is the empty string
12:03:15 alexpott: WimLeers: so then we should do the assignment in the if to not call get twice
12:03:31 alexpott: WimLeers: but still this looks super odd
12:04:30 WimLeers: alexpott: yeah we could assign it to a variable first
12:04:55 berdir: not sure that's worth it, it's not an expensive call?
12:07:03 alexpott: tbh I still don't get why has is returning true then get is returning an empty string
12:07:14 alexpott: WimLeers: ^^
12:08:12 WimLeers: alexpott: because Symfony's bags
12:08:26 WimLeers: alexpott: basically, the has() checks if an array key exists
12:08:32 alexpott: WimLeers: like why are we setting an empty X-Drupal-Cache-Tags header?
12:08:56 alexpott: WimLeers: yes but why have we set it?
12:09:14 WimLeers: alexpott: so it doesn't happen for cache tags. We ALWAYS have cache tags
12:09:21 WimLeers: alexpott: it can only happen for cache contexts
12:09:32 WimLeers: alexpott: and it'd then be set as such in HtmlRenderer
12:09:45 WimLeers: alexpott: though I wonder if that's still true after https://www.drupal.org/node/2453059…
12:09:47 Druplicon: https://www.drupal.org/node/2453059 => Set default render cache contexts: 'theme' + 'languages:' . LanguageInterface::TYPE_INTERFACE #2453059: Set default render cache contexts: 'theme' + 'languages:' . LanguageInterface::TYPE_INTERFACE => 40 comments, 4 IRC mentions
12:10:45 WimLeers: alexpott: so I think it may not be necessary anymore in *practice*
12:10:50 WimLeers: alexpott: but it's still better to be safe than sorry
12:11:08 WimLeers: alexpott: it's totally valid for e.g. a controller that returns a JsonResponse to set X-Drupal-Cache-Tags
12:11:26 WimLeers: Because that'd allow Drupal to invalidate cached json responses on reverse proxies
12:11:40 WimLeers: And the logic there may still allow for an empty string to be set
12:11:46 WimLeers: And frankly, that *does* make sense
12:11:55 WimLeers: It communicates to the reverse proxy that there are NO cache contexts
12:11:58 WimLeers: (or no cache tags)
12:12:37 WimLeers: alexpott: ^^
12:12:40 WimLeers: berdir: ^^
12:12:43 alexpott: WimLeers: Well the other way of doing that is making that more explicit
12:12:55 WimLeers: Making *what* more explicit?
12:13:23 alexpott: WimLeers: a header of "Cache-control: " is meaningless
12:13:30 alexpott: WimLeers: a header of "Cache-control: none" is not
12:14:23 WimLeers: alexpott: oh, fair. But that'd mean we have to make 'none' a reserved keyword for both cache tags & contexts. That's fine, but that's out of scope.
12:14:58 WimLeers: alexpott: also, I think it's fine for each header to define its own semantics? (Unless HTTP dictates a header without a value — i.e. an empty string as value — can be omitted, of course)
12:22:18 alexpott: WimLeers: http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
12:22:31 alexpott: WimLeers: "Each header field consists of a name followed by a colon (":") and the field value."
12:23:37 alexpott: WimLeers: However... http://stackoverflow.com/questions/12130910/how-to-interpret-empty-http-accept-header
12:24:41 alexpott: WimLeers: But "IMHO, sending an empty header is completely pointless. It shouldn't be done, and parsers may not correctly parse these headers."
12:24:55 WimLeers: alexpott: hehe I was doing the same research :)
12:25:10 berdir: WimLeers: added the comment back
12:25:25 alexpott: WimLeers: my urge is to say that we should avoid this and by explicit
12:25:32 alexpott: WimLeers: no-cache-tags
12:25:39 alexpott: WimLeers: and no-cache-contexts
12:26:35 WimLeers: alexpott: fair. Let's do that in a follow-up though? We still need the has() → get() to make sure we don't break on Response()s generated by controllers.
12:30:57 alexpott: WimLeers: but couldn't we just change the logic in HtmlRender to only add the headers when necessary?
12:33:23 WimLeers: alexpott: that's the thing: "yes, we could", BUT "no, because since https://www.drupal.org/node/2453059, it's never empty" — i.e. these changes are strictly speaking not necessary anymore. But, again, even then any controller that returns a Response (e.g. a cacheable JSON response) may set those headers, and then they may still be empty.
12:33:26 Druplicon: https://www.drupal.org/node/2453059 => Set default render cache contexts: 'theme' + 'languages:' . LanguageInterface::TYPE_INTERFACE #2453059: Set default render cache contexts: 'theme' + 'languages:' . LanguageInterface::TYPE_INTERFACE => 40 comments, 5 IRC mentions
12:34:08 WimLeers: alexpott: until we have those 'no-cache-tags|contexts' values implemented, this is the most correct thing to do. Once we have those, we'll actually need to complicate FinishResponseSubscriber: it'll need to detect the presence of 'no-cache-tags|contexts', and *not* merge it, but remove it.
12:34:35 WimLeers: alexpott: that's the beauty of using the empty string as the 'nope, nothing' value: it's easy to merge additional tags/contexts into
12:35:17 alexpott: WimLeers: but it makes has() kinda wrong
12:36:41 WimLeers: alexpott: no, this is how has() is designed to work
12:36:52 WimLeers: alexpott: the same problem exists for →hasOption() on routes, hasRequirement(), etc
12:44:48 alexpott: WimLeers: we need a setIfValue() :)
12:45:32 WimLeers: alexpott: do we? we need to get & update the existing value too, so not sure that'd fly :)
12:47:12 alexpott: WimLeers: patch needs a reroll anyways :)
12:48:40 WimLeers: alexpott: yeah :P
12:48:50 WimLeers: alexpott: basically all of my patches are conflicting all the time lol
12:49:02 WimLeers: both a good and a bad sign I guess
12:49:21 WimLeers: (good: I'm indeed providing building blocks for next steps; bad: painful)

Ohhhh good point! Can you link to that issue and then close this one?