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

[delusions]

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

[peaceablewhale]

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.

[Denis-de-Bernardy]

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.

[peaceablewhale]

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.

[Denis-de-Bernardy]

see also #5028

[theMikeD]

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

[Denis-de-Bernardy]

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.

[MikeNGarrett]

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?

[stevenkword]

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.

[MikeNGarrett]

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.

[stevenkword]

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

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

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?

[stevenkword]

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.

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

[MikeNGarrett]

Looks good!

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

[stevenkword]

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

[stevenkword]

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

[jorbin]

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

[jorbin]

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.

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

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

[stevenkword]

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.

[stevenkword]

@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

[stevenkword]

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.

[stevenkword]

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

[obenland]

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

[wonderboymusic]

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]

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

[obenland]

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

[ocean90]

Do we need to revert [32765]?

[obenland]

We do.

[stevenkword]

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.

[obenland]

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

[stevenkword]

Refresh @ 4.5.1

[stevenkword]

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)

[stevenkword]

Refresh @ 4.7.2

[jbpaul17]

Punting to 4.8.1 per today's bug scrub.

[stevenkword]

Now without a new query!

[stevenkword]

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

[spacedmonkey]

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.

[jbpaul17]

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

[mauteri]

Unit Test for get_last_build_date

[mauteri]

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

[stevenkword]

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

[spacedmonkey]

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.

[ryanshoover]

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]

Adds filter & cleans up logic

[stevenkword]

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.

[melchoyce]

Punting per ​https://wordpress.slack.com/archives/C02RQBWTW/p1507173338000139

[stevenkword]

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

[stevenkword]

Refreshed at 4.9.1

[jbpaul17]

This ones been punted out of a couple major releases, so adding an early to this one in hopes it can land in 5.0 and not continue with regular patch refreshes past 2018.

[pento]

Well, this didn't land early in 5.1. It seems reasonable for 5.2, though.

[stevenkword]

Replying to pento:

Well, this didn't land early in 5.1. It seems reasonable for 5.2, though.

I mean this in the least bitter way possible -- What do I have to do to get a committer to review this? I'm the maintainer of this component, and this was some low-hanging fruit that was identified and has been refreshed for the past 4 years. I've attending core meetings and been promised some attention on this specific ticket on more than one occasion. It's cycled through multiple contributor days and hope is almost lost. I'm asking from a desire to be a more effective maintainer, not anger. Either I would humbly ask to be trusted to commit against this component, or I need a reliable ally to act on my behalf. Otherwise, this component is dead in the water. Help!

[pento]

That's a fair question, @stevenkword.

With WordPress 5.1 Beta 1 due for release this week, and there still being several hundred tickets in the milestone, I'm erring on the side of re-milestoning anything that I can't immediately look at and be confident it doesn't have any unexpected side effects.

I moved it to the 5.2 milestone because, with a brief review it seems good, but I'd like to give it further investigation. In particular, changes that touch the $wp_query global need to be carefully considered.

I'm also being fairly conservative in what I move to the 5.2 milestone, as opposed to Future Release: only tickets that I think can realistically be reviewed and merged in 5.2 are there. So, barring unforeseen circumstances, I think it's reasonable that this enhancement can be committed in WordPress 5.2.

[stevenkword]

Replying to pento:

That's a fair question, @stevenkword.

With WordPress 5.1 Beta 1 due for release this week, and there still being several hundred tickets in the milestone, I'm erring on the side of re-milestoning anything that I can't immediately look at and be confident it doesn't have any unexpected side effects.

I moved it to the 5.2 milestone because, with a brief review it seems good, but I'd like to give it further investigation. In particular, changes that touch the $wp_query global need to be carefully considered.

I'm also being fairly conservative in what I move to the 5.2 milestone, as opposed to Future Release: only tickets that I think can realistically be reviewed and merged in 5.2 are there. So, barring unforeseen circumstances, I think it's reasonable that this enhancement can be committed in WordPress 5.2.

Thank you for the prompt response @pento. That seems reasonable and I do understand the hesitation to muck with WP_Query. I'll ensure this ticket stays in a committable state through the 5.2 cycle. Can we please just keep an eye on it when the time comes? There are more tickets that I'd also like to shepherd through the process, but this ticket illustrates some of the challenges the contributors to this component have been facing. I don't want Feeds to be a 2nd-class citizen. I feel the component used a lot more than people realize and I would like to see the project address some long-standing issues in 2019.

[spacedmonkey]

In 4575.3.diff I have refreshed the patch so it applies to trunk at time of writing and improved the tests.

This ticket has been ready for 18 months and was signed by two compound maintainers (myself and @stevenkword ). I am not sure I under @pento point about touching wp_query. The functions access the variable but doesn't modify it in anyway, just reads values. Tonnes of other functions in core do this, so I am not sure I understand the issue here.

This should be merge first thing in 5.2 as it has been bumped from every release since 4.6.

[stevenkword]

Replying to spacedmonkey:

In 4575.3.diff I have refreshed the patch so it applies to trunk at time of writing and improved the tests.

<3. Thank you, sir!

I am not sure I under @pento point about touching wp_query. The functions access the variable but doesn't modify it in anyway, just reads values. Tonnes of other functions in core do this, so I am not sure I understand the issue here.

That's right, I was conflating this with another ticket. This one doesn't modify WP_Query at all. It only reads it. Doesn't feel very risky.

[ryanshoover]

@stevenkword @spacedmonkey Are we good to queue this patch up for 5.2?

The only concern seems to be its modification of $wp_query - that seems to be misplaced. While the new get_last_build_date() function reads from $wp_query, it doesn't modify it in any way.

[adamsilverstein]

Thanks for the continued, repeated efforts to get this solved by all those involved on this ticket!

It is truly impressive to see a 12 year old ticket that is both still viable, and still getting contributor effort.

Given the unit tests and careful review by the component maintainers, I'm already confident this is a solid solution. I'll take some time to review, test, fine tune and commit this soon - before we hit 5.2 beta.

@stevenkword i'm going to own this ticket because that is how i keep track of my commitments :)

[stevenkword]

Replying to adamsilverstein:

@stevenkword i'm going to own this ticket because that is how i keep track of my commitments :)

Sounds great, thanks Adam!

[adamsilverstein]

@stevenkword - I reviewed the latest patch... this looks great and is very close to ready.

I have a question about the use of max to find the latest date. This is comparing the values in post_modified_gmt (or comment_date_gmt) which are date string, eg 2019-02-27 19:49:41 - I think this works, but i worry about potential edge cases where this string might not sort correctly (for example a one digit month '2' could sort wrong against '11' - although i think these datestrings will have '0's padding them). string sorting can sometime have unexpected results.

Would it be worth converting these strings to numbers (timestamps) before the call to max to ensure the sorting always works as expected? eg add $modified_time = array_map( 'strtotime', $modified_times ); then convert back to a string before returning.

Also: I would love to see the tests expanded a bit if possible, ideally creating posts in two categories so its clear category feeds lastModified are updated independently (eg the test would fail before the new helper)

[adamsilverstein]

cc: @spacedmonkey @ryanshoover

[spacedmonkey]

@adamsilverstein Just did some testing and two things. Firstly, the datetime does pad 0 in front of months, for this date as an example 2019-01-22 23:40:59. So I don't think it is possible to max to fail. But I can change the tests, so that test posts are from different months. So I don't think that strtotime array map is required.

I will change up the tests a bit, but I am not personally sure it is required...

[adamsilverstein]

I will change up the tests a bit, but I am not personally sure it is required...

@spacedmonkey thank you for checking, I wasn't sure either and was mostly asking because of past experience sorting strings. i agree, based on how we store the _gmt fields this should work as expected. my main point was that converting these strings to timestamps would increase my confidence that the sorting will always work correctly. additional tests would as well! if the tests could demonstrate the original point of this ticket: that all category feeds don't have the same date, that would be even better!

[JeffPaul]

@spacedmonkey any updates on getting those tests updated to help with @adamsilverstein's review on this as we try and get this into 5.2?

[spacedmonkey]

Replying to JeffPaul:

@spacedmonkey any updates on getting those tests updated to help with @adamsilverstein's review on this as we try and get this into 5.2?

Not had the time to update the tests. Don't personally agree that the test need upgrading. Maybe @stevenkword could look at them.

[adamsilverstein]

4575.4.diff​ update @since to 5.2.0

[stevenkword]

Replying to adamsilverstein:

4575.4.diff​ update @since to 5.2.0

Does this mean what I think it means!?!

I haven't had a chance to take a 2nd pass at the test yet, but if your eyes are on it I can prioritize this for tomorrow.

[adamsilverstein]

In 44948:

Feeds: ensure build/update date matches current query.

Displaying the correct build date in feeds is as important today as it was twelve years ago when this ticket was opened.

Fix an issue where all feeds in WordPress showed the same date for their last build date (the datapoint is lastBuildDate, updated or dc:date depending on the feed type).

Introduce a new get_last_build_date filter to adjust the date used for lastBuildDate. Developers who previously filtered get_lastcommentmodified to alter feed dates should use this filter instead.

• get_last_build_date extracts the latest post (or comment) in the current WP_Query object.
• In all feed templates, use get_last_build_date vs get_lastpostmodified( 'GMT' );.

Props stevenkword, spacedmonkey, ryanshoover, mauteri, nacin, jorbin, MikeNGarrett, Denis-de-Bernardy, peaceablewhale.
Fixes #4575.

[MikeNGarrett]

🎉 YAY!

[pento]

I have some feedback on [44948]. 🙂

It seems like all of the places where get_last_build_date() is called could be simplified if it accepted a formatting string parameter. For example, feed-rss2.php would go from

        <lastBuildDate>
        <?php
                $date = get_last_build_date();
                echo $date ? mysql2date( 'r', $date, false ) : date( 'r' );
        ?>
        </lastBuildDate>

to

        <lastBuildDate><?php echo get_last_build_date( 'r' ); ?></lastBuildDate>

This same pattern is repeated in all feed templates.

When get_last_build_date() returns the value of get_lastpostmodified( 'GMT' ), it doesn't go through the get_last_build_date filter.

Using max() for the string comparison is cute, but confusing, as @adamsilverstein ran into earlier in the thread. Relying on string sorting behaviour for sorting dates is also a recipe for subtle bugs. I would be inclined to explicitly sort by timestamp, instead:

        // Determine the maximum modified time.
        $max_modified_time = max(
                array_map(
                        function ( $time ) {
                                return mysql2date( 'G', $time, false );
                        },
                        $modified_times
                )
        );

The get_last_build_date() name is very generic, it doesn't indicate that this is a feed template function. Perhaps get_feed_build_date(), or something of that nature would work better.

This change also needs a dev-note, particularly as it includes back compat breaks.

[adamsilverstein]

@pento I addressed your feedback in 4575.5.diff​ - I haven't tested carefully yet - let me know if the patch looks good.

@stevenkword & @spacedmonkey can you please review this patch to make sure this looks good to you? Ideally test this and verify that everything still works as expected?

[adamsilverstein]

4575.6.diff​ includes phpbf auto-fixes.

[adamsilverstein]

Note: Seeing some unit test failures for feeds in travis after the latest patch: ​https://travis-ci.org/adamsilverstein/wordpress-develop-fork/jobs/520992998

[adamsilverstein]

4575.8.diff​ includes a missing use statement to pass the format into array_map. re-running tests: ​https://travis-ci.org/adamsilverstein/wordpress-develop-fork/builds/521300932

[spacedmonkey]

The use keyword requires php 5.3 or higher.

[adamsilverstein]

4575.13.diff​ fixes test naming

The use keyword requires php 5.3 or higher.

@spacedmonkey Good to know, I was looking for that information. Since we only support PHP 5.6+, this is fine.

[spacedmonkey]

@adamsilverstein why not just use a foreach loop here. It mean will mean we keep backwards compatibility and can save the breaking change for something more important.

[adamsilverstein]

tests are passing after 4575.14.diff​ ​https://travis-ci.org/adamsilverstein/wordpress-develop-fork/builds/521327099 - @spacedmonkey Sure, I'll try an array based version.

[adamsilverstein]

4575.15.diff​ is an array based version for comparison. The array_map version in 4575.14.diff​ feels more elegant to me though.

save the breaking change for something more important

@spacedmonkey I'm not really sure 4575.14.diff​ is a breaking change, it will only land in WP 5.2, which requires PHP 5.6 - how do you see this as a breaking change? To me it is more about using modern coding practices for new code.

Also appreciate any help you can provide testing that the dates still show up as you expect them to in the feeds.

[pento]

Thanks for handling this, @adamsilverstein!

Let's go with 4575.14.diff​. As of WP 5.2, index.php is the only thing that needs to parse-able in older PHP versions, anything outside of that can start adopting newer practices as we see fit.

One minor thing, it looks like you need to manually fix the indenting on the function passed to array_map(). WPCS isn't picking it up.

@jrf: is this a known issue?

[adamsilverstein]

Let's go with 4575.14.diff​.

Roger that, I'll get this committed.

One minor thing, it looks like you need to manually fix the indenting on the function passed to array_map(). WPCS isn't picking it up.

I noticed that, I think that indentation was the result of running grunt format:php, but definitely looks wrong. I'll fix and we can ignore the linter error for now.

[adamsilverstein]

4575.16.diff​ fixes the tabs from 4575.14.diff​

[adamsilverstein]

In 45247:

Feeds: improve structure and naming of feed build date helper function.

Simplify overall code structure by passing the required format to the helper function.
Clarify functionality by renaming get_last_build_date to get_feed_build_date.

Props pento, spacedmonkey.
Fixes #4575.

[stevenkword]

Absolutely! Thanks for all those involved in the rally here! I've asked a set of new eyes, @antpb, for good measure, but this looks good in my book!

Committed in https://core.trac.wordpress.org/changeset/45247.

[desrosj]

As far as I can tell, this one never received a dev note. Going to remove needs-dev-note as the ship has already sailed.