WordPress.org

Make WordPress Core

Opened 10 years ago

Last modified 2 weeks ago

#4575 assigned enhancement

Add functions to return the last-modified timestamp of a category/tag

Reported by: delusions Owned by: stevenkword
Milestone: 5.0 Priority: normal
Severity: normal Version: 2.2.1
Component: Feeds Keywords: has-patch has-unit-tests commit
Focuses: template Cc:

Description

Hi,

All categories Last-Modified dates are the same, and thats a one big rss file i guess.
Is there a chance to send individual Last-Modified dates to these feed ?

Thanks

Attachments (13)

4575.diff (2.1 KB) - added by stevenkword 3 years ago.
4575_2.diff (2.8 KB) - added by stevenkword 3 years ago.
4575_3.diff (2.8 KB) - added by stevenkword 3 years ago.
4575_4.diff (4.6 KB) - added by stevenkword 2 years ago.
4575_5.diff (4.6 KB) - added by stevenkword 2 years ago.
4575_6.diff (4.8 KB) - added by stevenkword 2 years ago.
4575_7.diff (4.5 KB) - added by stevenkword 19 months ago.
Refresh @ 4.5.1
4575_8.diff (3.7 KB) - added by stevenkword 9 months ago.
Refresh @ 4.7.2
4575.9.diff (3.6 KB) - added by stevenkword 5 months ago.
Now without a new query!
4575_tests.diff (2.1 KB) - added by mauteri 3 months ago.
Unit Test for get_last_build_date
4575.10.diff (5.7 KB) - added by mauteri 3 months ago.
Putting everything in one diff so it's easier for the committer to review
4575.2.diff (5.7 KB) - added by spacedmonkey 2 months ago.
4575.11.diff (5.9 KB) - added by ryanshoover 2 months ago.
Adds filter & cleans up logic

Download all attachments as: .zip

Change History (80)

#1 @foolswisdom
10 years ago

  • Milestone set to 2.4 (future)

#2 @ffemtcj
10 years ago

  • Milestone changed from 2.5 to 2.7

#3 @matt
9 years ago

  • Milestone changed from 2.7 to 2.9
  • Priority changed from normal to low

#4 @Denis-de-Bernardy
9 years ago

  • Severity changed from normal to minor

#5 @Denis-de-Bernardy
8 years ago

  • Component changed from Optimization to Feeds
  • Keywords needs-patch added
  • Milestone changed from 2.9 to Future Release

#6 @peaceablewhale
8 years ago

  • Priority changed from low to normal
  • Severity changed from minor to normal

I suggest adding a function that returns the last-modified timestamp of the query in WP_Query.

Alternatively, since the first entry in the feed is the latest, using it as the feed's last-modified timestamp seems to be fine as well.

#7 @Denis-de-Bernardy
8 years ago

seems right, yes. if posts, use the latest (possibly non-sticky) post to work it out, else we fallback to the normal last modified date.

#8 @peaceablewhale
8 years ago

  • Component changed from Feeds to Template
  • Summary changed from Last-Modified for rss categories to Add functions to return the last-modified timestamp of a category/tag

Perhaps this should be moved from Feeds to Template because the category_last_modified and tag_last_modified functions have to be implemented in category-template.php first. The last_modified functions may also be used for generating the HTTP Last-Modified header when the category/tag is loaded.

#10 @ryan
8 years ago

  • Type changed from task (blessed) to enhancement

#11 @nacin
4 years ago

  • Component changed from Template to Taxonomy
  • Focuses template added

#12 @ericlewis
3 years ago

  • Keywords good-first-bug added

#13 follow-up: @theMikeD
3 years ago

If someone can tell me how to duplicate/reveal this issue so I can see it, I'll take a crack at it.

#14 in reply to: ↑ 13 @Denis-de-Bernardy
3 years ago

Replying to theMikeD:

If someone can tell me how to duplicate/reveal this issue so I can see it, I'll take a crack at it.

As I understand the ticket, it's about the last modified date advertised by the feeds. Currently (or when this was posted anyway), WP sets it to the last post's date or to the last post's modified date (or to whatever else... I'm not sure which.)

OP suggests it should be set to the the same in the appropriate category or tag instead.

In the event the issue is still around, an alternative fix (which would likely be better and for all feeds if this hasn't been done already) would be to use the latest last modified date from any post that appears in that feed (i.e. loop through the main loop's post, and extract the maximum modified date). This for any type of feed, ideally -- including the main ones and comment feeds.

#15 @MikeNGarrett
3 years ago

  • Component changed from Taxonomy to Feeds

I'm assuming this references lastBuildDate on category feeds. I noticed these were all the same for all categories regardless of the feed content.

It appears to correlate to the date and time of the latest updated post using get_lastpostmodified.

The only other place this function is referenced is in send_headers() to set Last-Modified and ETag globally, so changing get_lastpostmodified() needs to be done carefully.

In this case, could we rely on the globalized post object being accurate for the last modified post? or would this require hooking into wp_query to get a more accurate result?

#16 @stevenkword
3 years ago

I don't believe modifying get_lastpostmodified() is the appropriate solution. In addition to the methods use outside of RSS feeds, plugin authors may be using this method to bust caches, etc.

I think we first need to determine the desired behavior and perhaps create a new function specifically for feed syndication. I am in agreement that using the same lastBuildDate site-wide for all feeds is not ideal. In the current state, feeds will issue new timestamps to aggregators even when the content is otherwise unchanged. For example. If you add a new post to an 'Entertainment' category, the lastBuildDate for a 'News' category feed would also change, even though there were no changes to the 'News' category feed's content.

It would be more appropriate to only issue a change to the lastBuildDate value of individual feeds if the query itself has changed. Adding something like a stored hash of the WP_Query object or perhaps an array of post modified timestamps to term/object meta would still allow aggregators to detect all changes to the feed, but only when content actually changes.

#17 @MikeNGarrett
3 years ago

I see what you're saying.

As for desired behavior, feeds are one of the few places we can reliably depend on a particular format for the content of that view. For that reason, the lastBuildDate should reflect the most recently published content according to the parameters of the content being displayed in the feed rather than site-wide.

To achieve this, I would be in favor of allowing get_lastpostmodified to accept a parameter that can be passed to _get_last_post_time to modify the WHERE part of the sql statement.

I think hashing the result of the query and saving that would be a messy solution since we're already using wpdb to make a query in _get_last_post_time to find the correct value.

#18 @stevenkword
3 years ago

@MikeNGarrett -- After having put some more thought towards it, I agree that creating a new hash would be messy so I've revised my approach.

4575.diff adds a new methods specifically for returning the most recently modified post or comment from any given feed. It uses the default WP_Query object to obtain a list of post object IDs displayed in the feed and then leverages $wpdb->get_var() to retrieve the most recently modified post/comment.

Last edited 3 years ago by stevenkword (previous) (diff)

#19 @stevenkword
3 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

@stevenkword
3 years ago

#20 follow-up: @MikeNGarrett
3 years ago

This looks really good. We'll have to replicate this for the other feed types once we get a final version.

Here's another question: would you need to make another db query to get the last modified posts? You already have a list of posts, so instead of building an array of IDs you should be able to build an array of post_modified values and return the latest.

I would only have 2 concerns if we went that direction:

  1. What happens when feeds are not sorted by latest?
  2. Should we be concerned about pagination?

#21 in reply to: ↑ 20 @stevenkword
3 years ago

Replying to MikeNGarrett:

This looks really good. We'll have to replicate this for the other feed types once we get a final version.

Here's another question: would you need to make another db query to get the last modified posts? You already have a list of posts, so instead of building an array of IDs you should be able to build an array of post_modified values and return the latest.

Good catch. I could have sworn the WP_Query object excluded post_modified_gmt, but I double-checked and sure enough its there.

In 4575_2.diff I've been able to minimize the number of calls to the database by extracting the post IDs and last modified times from WP_Query. However, I still had to perform an extra lookup on comment feeds.

I would only have 2 concerns if we went that direction:

  1. What happens when feeds are not sorted by latest?

Since this is using PHP/MySQL max functions, this should sort itself out.

  1. Should we be concerned about pagination?

I don't think we need to worry about it since the query will always match displayed content in feeds.

Last edited 3 years ago by stevenkword (previous) (diff)

#22 @MikeNGarrett
3 years ago

Looks good!

For 2... I didn't even think about it, but feeds don't have pagination by default.

@stevenkword
3 years ago

#23 @stevenkword
3 years ago

4575_3.diff adds a MySQL clause for limiting the result to approved comments.

Last edited 3 years ago by stevenkword (previous) (diff)

@stevenkword
3 years ago

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


3 years ago

#25 @stevenkword
3 years ago

@jorbin -- per the conversation in Slack, could you please elaborate on a proposed solution? I'm not sure I'm following how to proceed without having logic inside the function.

#26 @jorbin
2 years ago

  • Keywords commit added
  • Milestone changed from Future Release to 4.3

This is looking good. I want to do a bit of testing, but I think it's ready.

#27 @obenland
2 years ago

  • Keywords good-first-bug removed

#28 @jorbin
2 years ago

  • Owner set to jorbin
  • Resolution set to fixed
  • Status changed from new to closed

In 32765:

Improve lastBuildDate timestamp in rss feeds

RSS feed timestamps should reflect the actual timestamps for those RSS feeds rather than the generic timestamp for all posts and all comments.

Props stevenkword.
Fixes #4575.

#29 follow-up: @nacin
2 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Something feels a bit off about this. I can't put my finger on it.

  • The query for a comments feed looks kind of nasty. Is it actually necessary for /comments/feed/? I don't think so. I think it's only necessary for a comment feed for an individual post, which should be a lot easier to query and wouldn't require the IN or MAX. Can a comment feed other than /comments/feed/ have comments from more than one post? Should we even try to support that with an insane query that could be a slow query against a huge DB table? I'm not sure.
  • This doesn't patch atom feeds.
  • What's the purpose of get_lastcommentmodified()? Assuming the atom feed is also updated, it won't be used anymore. Seems like we might need to do some tweaks.

#30 in reply to: ↑ 29 ; follow-up: @stevenkword
2 years ago

Replying to nacin:

Something feels a bit off about this. I can't put my finger on it.

  • The query for a comments feed looks kind of nasty. Is it actually necessary for /comments/feed/? I don't think so. I think it's only necessary for a comment feed for an individual post, which should be a lot easier to query and wouldn't require the IN or MAX. Can a comment feed other than /comments/feed/ have comments from more than one post? Should we even try to support that with an insane query that could be a slow query against a huge DB table? I'm not sure.

I see what you are saying regarding the query and I'm taking another look at it. I think we can make this perform a lot better given those truths.

  • This doesn't patch atom feeds.

Agreed, this should patch atom as well. I will include this in the upcoming patch.

  • What's the purpose of get_lastcommentmodified()? Assuming the atom feed is also updated, it won't be used anymore. Seems like we might need to do some tweaks.

I did a little digging through the plugin repo and found that 6 plugins are using get_lastcommentmodified and roughly 100 plugins are making calls to get_lastpostmodified. Looking at some of those plugin names, I am not sure we should deprecate the functions since the expected behavior might be slightly different.

The existing core functions both return a cached value that is relevant to all content, system wide. This might be useful for things like timestamps for sitemaps. On the other hand, the proposed function get_last_build_date_feed was designed to return the timestamp related to the most recently modified unit of content on any given request. This provides a level of granularity that previously did not exist for things like category feeds, independently of each other.

I am thinking now that the function should be more generalized to something like get_last_build_date (dropping _feed) since we could also use it for things like Last-Modified HTTP headers.

Last edited 2 years ago by stevenkword (previous) (diff)

#31 @stevenkword
2 years ago

@nacin 4575_4.diff makes another pass at this, but unfortunately doesn't really avoid the "IN" MySQL statement in comments query. I'm open to suggestions on how to improve this piece.

This patch also:

  • Adds Atom support
  • Replaces the comment query with a call to WP_Comment_Query
Last edited 2 years ago by stevenkword (previous) (diff)

#32 in reply to: ↑ 30 @stevenkword
2 years ago

Replying to stevenkword:

I am thinking now that the function should be more generalized to something like get_last_build_date (dropping _feed) since we could also use it for things like Last-Modified HTTP headers.

Scratch what I said about HTTP headers. Those are set before the default query is run. I'm not sure if it's going to be possible to setup HTTP headers based off displayed content.

Last edited 2 years ago by stevenkword (previous) (diff)

@stevenkword
2 years ago

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


2 years ago

@stevenkword
2 years ago

#34 @stevenkword
2 years ago

4575_5.diff refreshes 4575_4.diff (which was dirty). Ready for another look @jorbin.

#35 @obenland
2 years ago

@jorbin, could you take another look and resolve the ticket?

#36 @wonderboymusic
2 years ago

  • Keywords needs-refresh added; commit removed

Patch doesn't fully apply. Also, you should make the patches from root (src should be the first part of the path for each changed file). Doing so lets us know that you are capable of running unit tests with the patch applied.

@stevenkword
2 years ago

#37 @stevenkword
2 years ago

  • Keywords needs-refresh removed

4575_6.diff is refreshed and relative to the /src/ directory.

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


2 years ago

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


2 years ago

#40 @obenland
2 years ago

  • Type changed from enhancement to task (blessed)

#41 @obenland
2 years ago

  • Owner changed from jorbin to nacin
  • Status changed from reopened to assigned

#42 follow-up: @obenland
2 years ago

  • Milestone changed from 4.3 to Future Release
  • Type changed from task (blessed) to enhancement

No movement since we entered beta, let's try again in a future release.

#43 @ocean90
2 years ago

Do we need to revert [32765]?

#44 @obenland
2 years ago

We do.

#45 @stevenkword
2 years ago

Agreed. The existing patch against trunk we deemed less than perfect. If we aren't able to take a look at the latest patch at this time, we should revert [32765] for the 4.3 release.

#46 @obenland
2 years ago

Reverted in [33281]. Apparently just mentioning a ticket doesn't crosslink to here.

@stevenkword
19 months ago

Refresh @ 4.5.1

#47 in reply to: ↑ 42 @stevenkword
19 months ago

Replying to obenland:

No movement since we entered beta, let's try again in a future release.

Can we take another look at this? Relates to (#36126)

#48 @stevenkword
18 months ago

  • Milestone changed from Future Release to 4.6

#49 @stevenkword
18 months ago

  • Milestone changed from 4.6 to Future Release

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


17 months ago

@stevenkword
9 months ago

Refresh @ 4.7.2

#51 @stevenkword
9 months ago

  • Milestone changed from Future Release to 4.8

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


7 months ago

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


6 months ago

#54 @jbpaul17
6 months ago

  • Milestone changed from 4.8 to 4.8.1

Punting to 4.8.1 per today's bug scrub.

@stevenkword
5 months ago

Now without a new query!

#55 @stevenkword
5 months ago

I found a new way to do this for free without creating a new query. @spacedmonkey Would you mind giving a 2nd opinion?

#56 @spacedmonkey
5 months ago

  • Keywords needs-unit-tests added

Can you provide some unit tests for this. If it is going to go into core, it will need tests. On the face of it, I am happy with it. I need to do some testing internally.

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


4 months ago

#58 @jbpaul17
4 months ago

  • Milestone changed from 4.8.1 to 4.9

Punting to 4.9 per today's bug scrub and to allow @stevenkword to work on unit tests.

@mauteri
3 months ago

Unit Test for get_last_build_date

@mauteri
3 months ago

Putting everything in one diff so it's easier for the committer to review

#59 @stevenkword
2 months ago

  • Owner changed from nacin to stevenkword

#60 @stevenkword
2 months ago

@spacedmonkey . Would you like to take one more pass at this before I throw the "commit" keyword on this?

@spacedmonkey
2 months ago

#61 @spacedmonkey
2 months ago

In 4575.2.diff it is tidying and refactoring. Key points

  • Remove while in tests and replaced with for loop as this is cleaner.
  • General WordPress coding style fixes.
  • Added ticket docs on test_get_last_build_date test
  • Return early on get_last_build_date if no posts.
  • Fixed docs and changed version number on get_last_build_date
  • Only have two return points in get_last_build_date

The only thing I noticed is that get_lastpostmodified has two filters in it pre_get_lastpostmodified and get_lastpostmodified will no longer fire. If developers using these filters to change the values in the feed, they stop working. This is small breaking change, but I personally don't have a problem with this. I would add a filter to the return at the of function, so developers can change the value if they want to.

#62 @ryanshoover
2 months ago

I've updated the patch to include a few enhancements / cleanups

  • Adds a filter at the end to allow modifying the last modified date
  • Simplifies the logic for determining the last modified comment
  • Renames the function's variables for clarity

Note that the new filter still leaves this as a breaking change from the current functionality. I don't see a clean way to maintain the other function's filters in this new function. get_lastpostmodified() is used by both feed-rss.php and class-wp.php. Any use of the original get_lastpostmodified() filters in this function risks overrunning modifications to class-wp.php.

Any plugin author that previously used the get_lastpostmodified filter needs to update to use the get_last_build_date filter.

@ryanshoover
2 months ago

Adds filter & cleans up logic

#63 @stevenkword
2 months ago

  • Keywords has-unit-tests commit added; needs-testing needs-unit-tests removed

Thanks for the patch @ryanshoover and congrats on your first core submission. I like what you've done with the function as far as making it leaner, and I also agree with your approach to the filter situation. Since the filters mentioned by @spacedmonkey are applied elsewhere with slightly different use cases, I think adding a new filter to this method is a good solution. While it would require plugin authors that are modifying this method to update their code, it would still provide them with a mechanism for doing so, which I think is the least of the evils.

IMO, this is ready to go.

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


8 weeks ago

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


7 weeks ago

#66 @melchoyce
7 weeks ago

  • Milestone changed from 4.9 to Future Release

#67 @stevenkword
2 weeks ago

  • Milestone changed from Future Release to 5.0

Patch still applies cleanly as of 4.9 RC2 at r42139. Unit tests still pass. Tagging for commit.

Note: See TracTickets for help on using tickets.