Fatal error: Call to a member function getTimestamp()

We also see this error, now with Drupal Core 8.0.5:

FastCGI sent in stderr: "PHP message: PHP Fatal error: Call to a member function getTimestamp() on null in /var/www/current/web/core/modules/page_cache/src/StackMiddleware/PageCache.php on line 257" while reading response header from upstream, client: 127.0.0.1

Possible fix for this

@msgph: looks good, but it would be good to add 1 line of comment saying that getExpires returns Null when the expires header is not set.

Also, it needs tests...

Needs tests.

Yes, this is clearly broken my server too.

The patch need reroll for 8.2.x

I ran into the same error.
I was using "domain" module and got the error only on secondary domain, so this might be caused by that, but I don't have time to look into it.
Also the error only appeared for anonymous users.

Anyway, here is the 8.2.x reroll for the patch.

This still needs tests, I guess.

Two ternary operators in a row make code a bit hard to understand, so I have rearranged the condition a little bit. But now, after pocking in code and testing, I'm wondering if there is a logic error in this code?

The condition in

  if ($expire === Cache::PERMANENT || $expire > $request_time) {

will almost always be TRUE. The only case when it becomes FALSE and cache of the page is not set is when the response is a "client error" and the value of the cache_ttl_4xx setting is set to 0.

Intuitively, I would think that if the Expires header of a page is set to a past date, it means that the page is already expired and should not be cached. But this current behavior may have something to do with the internals of the cache system that I do not fully understand.

For context, in its original form this check was added in #2257709: Remove the interdependence between the internal page cache and management of the Cache-Control header for external caches.

Test-only patches failed as expected.

Code in 8.2.x and 8.3.x branches has diverged and requires two different patches (which I have not noticed at first).

The path that 20th provide is more elegant.

Using it. No errors at the moment.

Ran into this when returning a RedirectResponse that did not have the URL in the metadata and had page expiration set to a day, with `page_cache` enabled.

Nice to see the test. However I think we should have a least a kernel tests that trigger this error in a real request.

This bug is still relevant on Drupal 8.4.x

In SecureLogin module I have to work around by calling $response->setExpires(); on a cacheable redirect.

Re-roll of #25 for Drupal 8.3.6

Running tests for #37, there are tests and removing that tag.

EDIT: Also, we're experiencing this in 8.3 as well, still.

A test scenario that I'd add to the unit test:

new CacheableJsonResponse($stations, 200, ['Cache-Control' => 'max-age=300, public']);

this throws the an error when the $header is populated with Expires.

Re-roll for 8.3.7.

Patch in #40 appllies to 8.4.0 and fixes the problem for me, thanks!

This issue seems to happen in 8.4.0 with Redirect module, on paths that have been redirected. (It worked fine in 8.3.x), as mentioned in #2915430. Patch from 40 applies with an offset, and fixes the issue.

I also ran into this upgrading to 8.4 with the redirect module.

I agree with #33, the current tests reach into the internal implementation of Drupal\page_cache\StackMiddleware\PageCache which we shouldn't really do and it also doesn't prove what kind of real response can trigger this code path.

I've done some analysis and found that the offending code is in \Drupal\Core\EventSubscriber\FinishResponseSubscriber:

    $is_cacheable = ($this->requestPolicy->check($request) === RequestPolicyInterface::ALLOW) && ($this->responsePolicy->check($response, $request) !== ResponsePolicyInterface::DENY);

    // Add headers necessary to specify whether the response should be cached by
    // proxies and/or the browser.
    if ($is_cacheable && $this->config->get('cache.page.max_age') > 0) {
      if (!$this->isCacheControlCustomized($response)) {
        // Only add the default Cache-Control header if the controller did not
        // specify one on the response.
        $this->setResponseCacheable($response, $request);
      }
    }
    else {
      // If either the policy forbids caching or the sites configuration does
      // not allow to add a max-age directive, then enforce a Cache-Control
      // header declaring the response as not cacheable.
      $this->setResponseNotCacheable($response, $request);
    }

The two methods responsible for setting the "Expire" header are setResponseCacheable and setResponseNotCacheable. So the trick to generating a problematic response is:

• Make it cacheable.
• Make it isCacheControlCustomized.

Under those circumstances, both method calls are skipped and the response is sent with no Expire header.

The patch looks good and changes only the minimal amount of code to handle the problem. It only affects the behaviour in the error case and leaves other scenarios untouched.

It introduces no new coding standards issues. One could argue that getCacheableWithCustomCacheControl needs a return type but there are other one line methods in the same file that don't have this so I'm using the precedent there to determine it's unneeded.

With the above and the fact that the patch successfully fixes the issue on one of our production sites I'm marking this as RTBC.

I only have two super silly nits, which aren't even worth un-RTBC'ing this for.

1. +++ b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
   @@ -375,4 +375,11 @@ public function getTestHeader(Request $request) {
   +  public function getCacheableWithCustomCacheControl() {
   

   Nit: s/CacheableWith/CacheableResponseWith/

2. +++ b/core/modules/system/tests/modules/system_test/system_test.routing.yml
   @@ -196,3 +196,11 @@ system_test.echo_utf8:
   +    _title: 'Cacheable response custom cache control'
   

   Nit: s/response custom/response with custom/

Happy to fix the feedback! Thanks for the review.

👌

Creditting @Wim Leers and @jcnventura because their reviews affected the patch directly. Thanks to everyone else who reported that the patch successfully fixed issues in production sites.

Committed and pushed 505f3bf8d4 to 8.5.x and 8d5dd3334e to 8.4.x. Thanks!

As a bug fix with no BC implications backported to 8.4.x

We just discovered this happening on our site too after updated from 8.3.x to 8.4.0 for logged out users only.

Appears to be a new issue related to the 8.4.0 release so tagging as such.

The commit 8d5dd33 from comment #55 applied cleanly to the tagged release 8.4.0 and fixed the issue :)

This happened on our site after upgrading to 8.4.0 and using Paragraphs module where anonymous users didn't have permission to view the Paragraph.

After enabling the 'view' permission and clearing cache it worked.

After upgrading to 8.4.0 we started noticing that redirects were failing for anonymous users. Applied the patch from #49 and everything is back to normal!

Same here, fatal errors on redirect pages. Using 8d5dd33 fixed our troubles.

Updating version

#49 also fixed this problem for me!