WordPress.org

Make WordPress Core

Opened 12 years ago

Closed 4 months ago

Last modified 4 months ago

#4575 closed enhancement (fixed)

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

Reported by: delusions Owned by: adamsilverstein
Milestone: 5.2 Priority: normal
Severity: normal Version: 2.2.1
Component: Feeds Keywords: dev-feedback needs-dev-note 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 (24)

4575.diff (2.1 KB) - added by stevenkword 4 years ago.
4575_2.diff (2.8 KB) - added by stevenkword 4 years ago.
4575_3.diff (2.8 KB) - added by stevenkword 4 years ago.
4575_4.diff (4.6 KB) - added by stevenkword 4 years ago.
4575_5.diff (4.6 KB) - added by stevenkword 4 years ago.
4575_6.diff (4.8 KB) - added by stevenkword 4 years ago.
4575_7.diff (4.5 KB) - added by stevenkword 3 years ago.
Refresh @ 4.5.1
4575_8.diff (3.7 KB) - added by stevenkword 2 years ago.
Refresh @ 4.7.2
4575.9.diff (3.6 KB) - added by stevenkword 2 years ago.
Now without a new query!
4575_tests.diff (2.1 KB) - added by mauteri 2 years ago.
Unit Test for get_last_build_date
4575.10.diff (5.7 KB) - added by mauteri 2 years 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 years ago.
4575.11.diff (5.9 KB) - added by ryanshoover 2 years ago.
Adds filter & cleans up logic
4575.12.diff (5.3 KB) - added by stevenkword 21 months ago.
Refreshed at 4.9.1
4575.3.diff (7.9 KB) - added by spacedmonkey 7 months ago.
4575.4.diff (7.4 KB) - added by adamsilverstein 5 months ago.
4575.5.diff (6.5 KB) - added by adamsilverstein 4 months ago.
4575.6.diff (6.5 KB) - added by adamsilverstein 4 months ago.
4575.7.diff (6.3 KB) - added by adamsilverstein 4 months ago.
4575.8.diff (6.3 KB) - added by adamsilverstein 4 months ago.
4575.13.diff (7.5 KB) - added by adamsilverstein 4 months ago.
4575.14.diff (7.6 KB) - added by adamsilverstein 4 months ago.
4575.15.diff (7.6 KB) - added by adamsilverstein 4 months ago.
4575.16.diff (7.6 KB) - added by adamsilverstein 4 months ago.

Download all attachments as: .zip

Change History (128)

#1 @foolswisdom
12 years ago

  • Milestone set to 2.4 (future)

#2 @ffemtcj
11 years ago

  • Milestone changed from 2.5 to 2.7

#3 @matt
11 years ago

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

#4 @Denis-de-Bernardy
10 years ago

  • Severity changed from normal to minor

#5 @Denis-de-Bernardy
10 years ago

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

#6 @peaceablewhale
10 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
10 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
10 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
10 years ago

  • Type changed from task (blessed) to enhancement

#11 @nacin
6 years ago

  • Component changed from Template to Taxonomy
  • Focuses template added

#12 @ericlewis
5 years ago

  • Keywords good-first-bug added

#13 follow-up: @theMikeD
5 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
5 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
4 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
4 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
4 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
4 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 4 years ago by stevenkword (previous) (diff)

#19 @stevenkword
4 years ago

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

@stevenkword
4 years ago

#20 follow-up: @MikeNGarrett
4 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
4 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 4 years ago by stevenkword (previous) (diff)

#22 @MikeNGarrett
4 years ago

Looks good!

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

@stevenkword
4 years ago

#23 @stevenkword
4 years ago

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

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

@stevenkword
4 years ago

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


4 years ago

#25 @stevenkword
4 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
4 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
4 years ago

  • Keywords good-first-bug removed

#28 @jorbin
4 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
4 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
4 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 4 years ago by stevenkword (previous) (diff)

#31 @stevenkword
4 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 4 years ago by stevenkword (previous) (diff)

#32 in reply to: ↑ 30 @stevenkword
4 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 4 years ago by stevenkword (previous) (diff)

@stevenkword
4 years ago

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


4 years ago

@stevenkword
4 years ago

#34 @stevenkword
4 years ago

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

#35 @obenland
4 years ago

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

#36 @wonderboymusic
4 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
4 years ago

#37 @stevenkword
4 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.


4 years ago

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


4 years ago

#40 @obenland
4 years ago

  • Type changed from enhancement to task (blessed)

#41 @obenland
4 years ago

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

#42 follow-up: @obenland
4 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
4 years ago

Do we need to revert [32765]?

#44 @obenland
4 years ago

We do.

#45 @stevenkword
4 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
4 years ago

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

@stevenkword
3 years ago

Refresh @ 4.5.1

#47 in reply to: ↑ 42 @stevenkword
3 years 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
3 years ago

  • Milestone changed from Future Release to 4.6

#49 @stevenkword
3 years ago

  • Milestone changed from 4.6 to Future Release

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


3 years ago

@stevenkword
2 years ago

Refresh @ 4.7.2

#51 @stevenkword
2 years ago

  • Milestone changed from Future Release to 4.8

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


2 years ago

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


2 years ago

#54 @jbpaul17
2 years ago

  • Milestone changed from 4.8 to 4.8.1

Punting to 4.8.1 per today's bug scrub.

@stevenkword
2 years ago

Now without a new query!

#55 @stevenkword
2 years 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
2 years 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.


2 years ago

#58 @jbpaul17
2 years 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
2 years ago

Unit Test for get_last_build_date

@mauteri
2 years ago

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

#59 @stevenkword
2 years ago

  • Owner changed from nacin to stevenkword

#60 @stevenkword
2 years ago

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

@spacedmonkey
2 years ago

#61 @spacedmonkey
2 years 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 years 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 years ago

Adds filter & cleans up logic

#63 @stevenkword
2 years 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.


23 months ago

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


23 months ago

#66 @melchoyce
23 months ago

  • Milestone changed from 4.9 to Future Release

#67 @stevenkword
22 months 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.

@stevenkword
21 months ago

Refreshed at 4.9.1

#68 @jbpaul17
17 months ago

  • Keywords early added

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.

#69 @pento
10 months ago

  • Milestone changed from 5.0 to 5.1

#70 follow-up: @pento
7 months ago

  • Milestone changed from 5.1 to 5.2

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

#71 in reply to: ↑ 70 @stevenkword
7 months ago

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!

Last edited 7 months ago by stevenkword (previous) (diff)

#72 follow-up: @pento
7 months ago

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.

#73 in reply to: ↑ 72 @stevenkword
7 months ago

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 is used a lot more than people realize and I would like to see the project address some long-standing issues in 2019.

Last edited 7 months ago by stevenkword (previous) (diff)

@spacedmonkey
7 months ago

#74 follow-up: @spacedmonkey
7 months ago

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.

#75 in reply to: ↑ 74 @stevenkword
7 months ago

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.

Last edited 7 months ago by stevenkword (previous) (diff)

#76 @ryanshoover
7 months ago

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

#77 follow-up: @adamsilverstein
6 months ago

  • Owner changed from stevenkword to 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 :)

#78 in reply to: ↑ 77 @stevenkword
6 months ago

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!

#79 @adamsilverstein
6 months ago

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

#80 @adamsilverstein
6 months ago

cc: @spacedmonkey @ryanshoover

#81 @spacedmonkey
6 months ago

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

#82 @adamsilverstein
6 months ago

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!

#83 follow-up: @JeffPaul
5 months ago

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

#84 in reply to: ↑ 83 @spacedmonkey
5 months ago

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.

#85 follow-up: @adamsilverstein
5 months ago

4575.4.diff update @since to 5.2.0

#86 in reply to: ↑ 85 @stevenkword
5 months ago

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.

#87 @adamsilverstein
5 months ago

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

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.

#88 @MikeNGarrett
5 months ago

🎉 YAY!

#89 @pento
4 months ago

  • Keywords dev-feedback needs-dev-note added; has-patch has-unit-tests commit early removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#90 @adamsilverstein
4 months ago

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

#91 @adamsilverstein
4 months ago

4575.6.diff includes phpbf auto-fixes.

#92 @adamsilverstein
4 months ago

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

#93 @adamsilverstein
4 months ago

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

#94 @spacedmonkey
4 months ago

The use keyword requires php 5.3 or higher.

#95 @adamsilverstein
4 months ago

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.

Last edited 4 months ago by adamsilverstein (previous) (diff)

#96 @spacedmonkey
4 months ago

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

#97 @adamsilverstein
4 months ago

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.

#98 @adamsilverstein
4 months ago

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.

Last edited 4 months ago by adamsilverstein (previous) (diff)

#99 @pento
4 months ago

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?

#100 @adamsilverstein
4 months ago

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.

#101 @adamsilverstein
4 months ago

  • Keywords commit added

4575.16.diff fixes the tabs from 4575.14.diff

#102 @adamsilverstein
4 months ago

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

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.

#103 @stevenkword
4 months ago

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!

Last edited 4 months ago by stevenkword (previous) (diff)

This ticket was mentioned in Slack in #core-committers by antpb. View the logs.


4 months ago

Note: See TracTickets for help on using tickets.