Leverage symfony for HTTP revalidation

Regarding HTTP revalidation, this is from the HTTP/1.1 RFC:

   HTTP applications have historically allowed three different formats
   for the representation of date/time stamps:

      Sun, 06 Nov 1994 08:49:37 GMT  ; RFC 822, updated by RFC 1123
      Sunday, 06-Nov-94 08:49:37 GMT ; RFC 850, obsoleted by RFC 1036
      Sun Nov  6 08:49:37 1994       ; ANSI C's asctime() format

   The first format is preferred as an Internet standard and represents
   a fixed-length subset of that defined by RFC 1123 [8] (an update to
   RFC 822 [9]). The second format is in common use, but is based on the
   obsolete RFC 850 [12] date format and lacks a four-digit year.
   HTTP/1.1 clients and servers that parse the date value MUST accept
   all three formats (for compatibility with HTTP/1.0), though they MUST
   only generate the RFC 1123 format for representing HTTP-date values
   in header fields. See section 19.3 for further information.

      Note: Recipients of date values are encouraged to be robust in
      accepting date values that may have been sent by non-HTTP
      applications, as is sometimes the case when retrieving or posting
      messages via proxies/gateways to SMTP or NNTP.

   All HTTP date/time stamps MUST be represented in Greenwich Mean Time
   (GMT), without exception. For the purposes of HTTP, GMT is exactly
   equal to UTC (Coordinated Universal Time).

Let's see how other software implement this requirement:

• Symfony HeaderBag::getDate() only parses RFC 1123 (in fact they use the constant DATE_RFC2822 but its the same format)
• Varnish TIM_parse() iterates over all dates specified in the RFC and additionally parses ISO 8601.

Note that the If-Modified-Since header is not even parsed into a date upon HTTP revalidation in Symfony, but the actual string is compared to the Last-Modified header of the response.

The fact that the RFC encourages for robustness in HTTP applications may have lead to #327269: drupal_page_cache_header() should compare timestamp. Against this background (and also compared to Varnish), the Symfony-code looks quite brittle. However, it is very important to keep in mind that conditional requests are HTTP/1.1 only. This means that any client issuing conditional requests is in fact required to send RFC 1123 formatted dates in GMT.

Thus I think string comparison of If-Modified-Since and Last-Modified is sufficient and therefore we safely can substitute the custom HTTP revalidation code with Symfonys Response::isNotModified() (and fix our tests accordingly).

There is no reason to be concerned. The Symfony mechanism is perfect, supporting pre-HTTP/1.1 date formats in conditional requests is not necessary.

We have $as_time_ago date formatting and other gotchas.

Time ago formatting does not really play well with any long-living markup cache. Except when the interval is formatted on the client using JavaScript (basically what the contrib Timeago module does).

I do not see why a hash wouldn't work for us. The only requirement an ETag needs to fulfill (apart from it being syntactically correct) is that it has to change whenever the underlying resource changes.

Scroll down a little bit and there is:

Note: One example of a strong validator is an integer that is
incremented in stable storage every time an entity is changed.
An entity's modification time, if represented with one-second
resolution, could be a weak validator, since it is possible that
the resource might be modified twice during a single second.

you only need it to change when the content changes.

This is exactly what somehash($content); does with the additional benefit that we do not depend on (micro)time. Having a stable algorithm to generate the ETag also helps with testing.

What exactly is it that makes you think having time in the ETag is better than supplying a hash?

Thanks!

1. +++ b/core/modules/page_cache/tests/src/Functional/PageCacheTest.php
   @@ -227,21 +227,19 @@ public function testConditionalRequests() {
   -    // Verify the page is not printed twice when the cache is warm.
   -    $this->assertSession()->responseNotMatches('#<html.*<html#');
   

   The test design wasn't ideal in the beginning. Regrettably deleting those two lines will remove some test coverage. I propose to move those lines to near the top of the test method. So that the start reads something like this:

       [...]
       // Fill the cache.
       $this->drupalGet('');
       // Verify the page is not printed twice when the cache is cold.
       $this->assertSession()->responseNotMatches('#<html.*<html#');
   
       $this->drupalGet('');
       // Verify the page is not printed twice when the cache is warm.
       $this->assertSession()->responseNotMatches('#<html.*<html#');
       $this->assertSession()->responseHeaderEquals('X-Drupal-Cache', 'HIT');
       [...]
   

Otherwise this is RTBC.

CR Draft here: https://www.drupal.org/node/3328834

Tagging this for global contribution weekend 2023. I hope that somebody likes to pick this up. I'll stick around for questions / reviews.