WordPress.org

Make WordPress Core

Opened 22 months ago

Closed 20 months ago

Last modified 17 months ago

#38027 closed defect (bug) (fixed)

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

Reported by: dllh Owned by: rachelbaker
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Comments Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

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

Attachments (4)

35788.diff (5.1 KB) - added by swissspidy 22 months ago.
38027.diff (14.3 KB) - added by swissspidy 21 months ago.
38027.2.diff (15.6 KB) - added by swissspidy 20 months ago.
38027.4.diff (13.9 KB) - added by rachelbaker 20 months ago.
Return the cached value of get_lastcommentmodified() and rename callback function

Download all attachments as: .zip

Change History (21)

#1 @swissspidy
22 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.7
  • Summary changed from Atom comments feed returns invalid timestamp in http headers when there are no comments to Comments feed returns invalid timestamp in http headers when there are no comments

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

@swissspidy
22 months ago

#2 @swissspidy
22 months ago

  • Keywords needs-unit-tests added

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.

#3 follow-up: @leobaiano
21 months ago

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

#4 in reply to: ↑ 3 @swissspidy
21 months ago

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
21 months ago

#5 @swissspidy
21 months ago

  • Component changed from Feeds to Comments
  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

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.

#6 @swissspidy
21 months ago

  • Keywords dev-feedback added

#7 @swissspidy
21 months ago

@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.

#8 @rachelbaker
21 months ago

@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?

#9 @swissspidy
21 months ago

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

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


21 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


20 months ago

#12 @rachelbaker
20 months ago

  • Keywords dev-feedback removed
  • Owner set to rachelbaker
  • Status changed from new to assigned

#13 @rachelbaker
20 months ago

@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
Last edited 20 months ago by rachelbaker (previous) (diff)

@swissspidy
20 months ago

#14 @swissspidy
20 months ago

@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
20 months ago

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

#15 @rachelbaker
20 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#16 @swissspidy
17 months ago

#36126 was marked as a duplicate.

#17 @swissspidy
17 months ago

#25358 was marked as a duplicate.

Note: See TracTickets for help on using tickets.