Comments feed returns invalid timestamp in http headers when there are no comments

[dllh]

Repro:

1. Spin up a site with no comments.
2. Curl the atom feed for comments to get headers.

Expected: Last-Modified header will contain a valid date.

Actual: Last-Modified: GMT

Some sample data:

curl -I http://pleasetest.me/comments/feed/atom/
HTTP/1.1 200 OK
Server: nginx
Date: Mon, 12 Sep 2016 19:10:16 GMT
Content-Type: application/atom+xml; charset=UTF-8
Connection: keep-alive
Last-Modified: GMT
ETag: "99b889a9cd748e8b4eca0eb3758d138d"
Link: <http://pleasetest.me/wp-json/>; rel="https://api.w.org/"
Expires: Thu, 31 Dec 2037 23:55:55 GMT
Cache-Control: max-age=315360000

After I add a comment to the site, the headers look like so:

curl -I http://pleasetest.me/comments/feed/atom/
HTTP/1.1 200 OK
Server: nginx
Date: Mon, 12 Sep 2016 19:16:37 GMT
Content-Type: application/atom+xml; charset=UTF-8
Connection: keep-alive
Last-Modified: Mon, 12 Sep 2016 19:16:20 GMT
ETag: "f256537b10a04293f105f6ac0d1cf128"
Link: <http://pleasetest.me/wp-json/>; rel="https://api.w.org/"
Expires: Thu, 31 Dec 2037 23:55:55 GMT
Cache-Control: max-age=315360000

[swissspidy]

I can confirm it. Not limited to atom feeds though.

[swissspidy]

35788.diff​ fixes the header for the comments feed by returning the current date in this case. However, posts are likely also affected as soon as the posts list table is empty (rarely the case though).

Also, there's the lastBuildDate field in the RSS feeds. Ideally, this would be best solved in the get_lastcommentmodified() and get_lastpostmodified() functions directly.

[leobaiano]

@swissspidy you attached the wrong .diff believe that confused the ticket, this file is the https://core.trac.wordpress.org/ticket/35788 tiecket :D

[swissspidy]

Replying to leobaiano:

@swissspidy you attached the wrong .diff believe that confused the ticket, this file is the https://core.trac.wordpress.org/ticket/35788 tiecket :D

Whoops, indeed :-) That's the result of juggling too many balls at once I guess. Makes me want to work on a better patch though, so there's that.

[swissspidy]

Alright, 38027.diff​ is the patch I wish I had uploaded last time.

Since get_lastcommentmodified() uses a static variable for caching, it's hardly testable. I took the time to rewrite it to use wp_cache_get() / wp_cache_add() (just like get_lastpostmodified() / _get_last_post_time()) and add tests for it.

Everywhere get_lastcommentmodified() and get_lastpostmodified() are used, there are now checks to see if they in fact return a string. If not, the current date is used.

Moving to the comments milestone as the patch has the biggest impact there. Plus, I'm not sure about introducing _transition_comment_status() for the cache invalidation.

[swissspidy]

@rachelbaker Would you mind having a look at the latest patch? It's a bit heavier than the original one and introduces a new _transition_comment_status() function. That way, get_lastcommentmodified() can be properly cached.

[rachelbaker]

@swissspidy I am not very familiar with the comment feeds, but I have a few thoughts:

1. In _transition_comment_status() you appear to only handle when a comment becomes approved. What about when a comment changes from approved to another status?
2. _transition_comment_status() isn't a specific name for what the function does. As it is written in your latest patch the function flushes the cache for comments when their status transitions. Maybe there is a name that communicates this?

[swissspidy]

I named it _transition_comment_status() because there's _transition_post_status() already. Looking at the latter, the check in _transition_comment_status() should be more like 'approved' === $new_status || 'approved' === $old_status

[rachelbaker]

@swissspidy Where are you adding the microseconds to the date/time you are storing as the cache value? I don't see them in the Last-Modified Header output or anywhere in the class-wp.php logic.

However, when I running the attached unit tests I am getting 4 failures:

1) Tests_Comment_Last_Modified::test_default_timezone
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-2000-01-01 10:00:00.000000
+2000-01-01 10:00:00

/srv/www/wpapi/tests/phpunit/tests/comment/lastCommentModified.php:19

2) Tests_Comment_Last_Modified::test_server_timezone
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-2000-01-01 10:00:00.000000
+2000-01-01 10:00:00

/srv/www/wpapi/tests/phpunit/tests/comment/lastCommentModified.php:29

3) Tests_Comment_Last_Modified::test_data_is_cached
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-2000-01-01 10:00:00.000000
+2000-01-01 10:00:00

/srv/www/wpapi/tests/phpunit/tests/comment/lastCommentModified.php:70

4) Tests_Comment_Last_Modified::test_cache_is_cleared
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-2000-01-01 10:00:00.000000
+2000-01-01 10:00:00

[swissspidy]

@rachelbaker Good question. That's probably coming from my local setup. I changed the assertions to use strtotime() instead so it works regardless of the microseconds.

In 38027.2.diff​ I also adjusted _transition_comment_status() as per my last comment and added an additional test for that.

[rachelbaker]

Return the cached value of get_lastcommentmodified() and rename callback function

[rachelbaker]

In 38925:

Feeds: Always return a valid timestamp for the Last-Modified header of comment or post feeds.

Fixes bug where an invalid Last-Modified value would be returned in feed requests for sites that had 0 items to return. Comment or post feeds will now return the current timestamp as the Last-Modified header value. Example: a request for the comments feed for a site without any comments.

Replaced use of the local static variable $cache_lastcommentmodified to store the modified date in get_lastcommentmodified() with the Object Cache API. The get_lastcommentmodified() function returns early if there is a cached value and returns false if there where no comments found. Introduced _clear_modified_cache_on_transition_comment_status() to flush the lastcommentmodified cache key when a comment enters or leaves approval status. In get_lastpostmodified() return early if there is a cached value and return false if there are no posts found.

Props swissspidy, rachelbaker, dllh, leobaiano.
Fixes #38027.

[swissspidy]

#36126 was marked as a duplicate.

[swissspidy]

#25358 was marked as a duplicate.