Feeds: Guard against empty array passed to max() in get_feed_build_date()#11387
Feeds: Guard against empty array passed to max() in get_feed_build_date()#11387chubes4 wants to merge 8 commits intoWordPress:trunkfrom
Conversation
…te(). When `wp_list_pluck()` returns an empty array from posts that lack the `post_modified_gmt` property, `max()` throws a `ValueError` on PHP 8+. This adds an emptiness check before calling `max()`, allowing `$datetime` to remain `false` and fall through to the existing `get_lastpostmodified()` fallback. Includes a unit test that simulates the condition. Fixes #59956.
|
Hi @chubes4! 👋 Thank you for your contribution to WordPress! 💖 It looks like this is your first pull request to No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making. More information about how GitHub pull requests can be used to contribute to WordPress can be found in the Core Handbook. Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook. If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook. The Developer Hub also documents the various coding standards that are followed:
Thank you, |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
Aligns variable assignments in the test to satisfy the Generic.Formatting.MultipleStatementAlignment sniff.
…_times. The production failure occurs when the posts array contains scalar values (e.g. post IDs) rather than post objects. wp_list_pluck() emits a _doing_it_wrong notice and returns an empty array, which then triggers the max() ValueError. Update the test to use an integer in the posts array and call setExpectedIncorrectUsage() to properly handle the expected notice. See #59956.
|
|
||
| // Determine the maximum modified time. | ||
| $datetime = date_create_immutable_from_format( 'Y-m-d H:i:s', max( $modified_times ), $utc ); | ||
| if ( ! empty( $modified_times ) ) { |
There was a problem hiding this comment.
While core uses empty() extensively, we should avoid it if possible. This can be more simply:
| if ( ! empty( $modified_times ) ) { | |
| if ( $modified_times ) { |
There was a problem hiding this comment.
Or more explicitly:
| if ( ! empty( $modified_times ) ) { | |
| if ( count( $modified_times ) > 0 ) { |
| global $wp_query; | ||
|
|
There was a problem hiding this comment.
| global $wp_query; |
| self::factory()->post->create( | ||
| array( | ||
| 'post_date' => $datetime->format( 'Y-m-d H:i:s' ), | ||
| ) | ||
| ); | ||
|
|
||
| // Simulate a query where have_posts() is true but wp_list_pluck() | ||
| // returns an empty array because the posts array contains scalar | ||
| // values (neither objects nor arrays). This triggers the _doing_it_wrong | ||
| // notice in WP_List_Util::pluck() and produces an empty result. | ||
| $wp_query = new WP_Query(); | ||
|
|
||
| $wp_query->posts = array( 1 ); | ||
| $wp_query->post_count = 1; |
There was a problem hiding this comment.
No need to simulate when we can do the real thing:
| self::factory()->post->create( | |
| array( | |
| 'post_date' => $datetime->format( 'Y-m-d H:i:s' ), | |
| ) | |
| ); | |
| // Simulate a query where have_posts() is true but wp_list_pluck() | |
| // returns an empty array because the posts array contains scalar | |
| // values (neither objects nor arrays). This triggers the _doing_it_wrong | |
| // notice in WP_List_Util::pluck() and produces an empty result. | |
| $wp_query = new WP_Query(); | |
| $wp_query->posts = array( 1 ); | |
| $wp_query->post_count = 1; | |
| $id = self::factory()->post->create( | |
| array( | |
| 'post_date' => $datetime->format( 'Y-m-d H:i:s' ), | |
| ) | |
| ); | |
| // Use a query where have_posts() is true but wp_list_pluck() | |
| // returns an empty array because the posts array contains scalar | |
| // values (neither objects nor arrays). This triggers the _doing_it_wrong | |
| // notice in WP_List_Util::pluck() and produces an empty result. | |
| query_posts( | |
| array( | |
| 'posts__in' => array( $id ), | |
| 'fields' => 'ids', | |
| ) | |
| ); |
- Replace `! empty( $modified_times )` with truthy check `if ( $modified_times )`, aligning with the ongoing effort to avoid empty() in new core code. - Rewrite the test setup to use the real query codepath: self::factory()->post->create() + query_posts( [ 'posts__in' => [ $id ], 'fields' => 'ids' ] ). 'fields' => 'ids' makes WP_Query->posts an array of integer IDs, which causes wp_list_pluck() to emit _doing_it_wrong and return an empty array naturally — no more manual $wp_query property mutation. - Add $this->assertIsString( $result ) to narrow the string|false return type before it flows into strtotime(), keeping PHPStan happy at higher analysis levels. Addresses review feedback from @westonruter. Props westonruter. See #59956.
|
Thanks so much for the thorough review @westonruter — really appreciate you showing up for this one. All five suggestions applied in 6b4c4c3:
Ready for another look whenever you have a minute. |
| // Use a query where have_posts() is true but wp_list_pluck() | ||
| // returns an empty array because the posts array contains scalar | ||
| // values (neither objects nor arrays). This triggers the _doing_it_wrong | ||
| // notice in WP_List_Util::pluck() and produces an empty result. |
There was a problem hiding this comment.
| // Use a query where have_posts() is true but wp_list_pluck() | |
| // returns an empty array because the posts array contains scalar | |
| // values (neither objects nor arrays). This triggers the _doing_it_wrong | |
| // notice in WP_List_Util::pluck() and produces an empty result. | |
| /* | |
| * Use a query where have_posts() is true but wp_list_pluck() | |
| * returns an empty array because the posts array contains scalar | |
| * values (neither objects nor arrays). This triggers the _doing_it_wrong | |
| * notice in WP_List_Util::pluck() and produces an empty result. | |
| */ |
There was a problem hiding this comment.
Pull request overview
This PR hardens feed date generation by preventing a PHP 8+ fatal error in get_feed_build_date() when the list of modified times is empty, ensuring RSS/Atom feeds still render successfully and fall back to get_lastpostmodified( 'GMT' ).
Changes:
- Add a guard to avoid calling
max()with an empty array inget_feed_build_date(). - Add a PHPUnit test that exercises the empty-
$modified_timespath and verifies the fallback behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/wp-includes/feed.php |
Guards the max( $modified_times ) call to avoid PHP 8+ ValueError on empty arrays and allow existing fallback logic to run. |
tests/phpunit/tests/date/getFeedBuildDate.php |
Adds coverage to ensure get_feed_build_date() does not error when plucked modified times are empty and instead falls back to last-modified time. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $datetime = new DateTimeImmutable( 'now', wp_timezone() ); | ||
| $datetime_utc = $datetime->setTimezone( new DateTimeZone( 'UTC' ) ); | ||
|
|
||
| $id = self::factory()->post->create( | ||
| array( | ||
| 'post_date' => $datetime->format( 'Y-m-d H:i:s' ), | ||
| ) | ||
| ); | ||
|
|
||
| // Use a query where have_posts() is true but wp_list_pluck() | ||
| // returns an empty array because the posts array contains scalar | ||
| // values (neither objects nor arrays). This triggers the _doing_it_wrong | ||
| // notice in WP_List_Util::pluck() and produces an empty result. | ||
| query_posts( | ||
| array( | ||
| 'posts__in' => array( $id ), | ||
| 'fields' => 'ids', | ||
| ) | ||
| ); | ||
|
|
||
| $this->setExpectedIncorrectUsage( 'WP_List_Util::pluck' ); | ||
|
|
There was a problem hiding this comment.
The test currently uses query_posts( [ 'fields' => 'ids' ] ) to force wp_list_pluck() to return an empty array, which requires setExpectedIncorrectUsage( 'WP_List_Util::pluck' ) and depends on a DB query plus an unrelated _doing_it_wrong() path. You can simulate the reported inconsistency more directly (and avoid the incorrect-usage expectation) by setting global $wp_query to a fresh WP_Query with post_count = 1 (so have_posts() is true) but an empty posts array (so wp_list_pluck() returns []). This keeps the test focused on guarding max() against empty input.
| $datetime = new DateTimeImmutable( 'now', wp_timezone() ); | |
| $datetime_utc = $datetime->setTimezone( new DateTimeZone( 'UTC' ) ); | |
| $id = self::factory()->post->create( | |
| array( | |
| 'post_date' => $datetime->format( 'Y-m-d H:i:s' ), | |
| ) | |
| ); | |
| // Use a query where have_posts() is true but wp_list_pluck() | |
| // returns an empty array because the posts array contains scalar | |
| // values (neither objects nor arrays). This triggers the _doing_it_wrong | |
| // notice in WP_List_Util::pluck() and produces an empty result. | |
| query_posts( | |
| array( | |
| 'posts__in' => array( $id ), | |
| 'fields' => 'ids', | |
| ) | |
| ); | |
| $this->setExpectedIncorrectUsage( 'WP_List_Util::pluck' ); | |
| global $wp_query; | |
| $datetime = new DateTimeImmutable( 'now', wp_timezone() ); | |
| $datetime_utc = $datetime->setTimezone( new DateTimeZone( 'UTC' ) ); | |
| self::factory()->post->create( | |
| array( | |
| 'post_date' => $datetime->format( 'Y-m-d H:i:s' ), | |
| ) | |
| ); | |
| // Simulate a query where have_posts() is true but wp_list_pluck() | |
| // receives an empty posts array, so modified_times is empty. | |
| $wp_query = new WP_Query(); | |
| $wp_query->post_count = 1; | |
| $wp_query->posts = array(); |
There was a problem hiding this comment.
@chubes4 Would this replicate the original scenario that you've encountered? Curious what your WP_Query has in it which caused this issue in the first place.
There was a problem hiding this comment.
No, that does not replicate the original scenario.
The specific query that triggers it can be observed here: https://github.com/Extra-Chill/data-machine-events/blob/main/inc/Core/Event_Post_Type.php#L153-L180
We use a custom artist taxonomy, and a custom data_machine_events post type.
When there are existing data_machine_events posts using a given artist term, but 0 regular posts, and we query a feed URL with fields => 'ids' set, WP_Query returns an array of scalar ids instead of WP_Post objects.
wp_list_pluck() requires each element to be an object or array, so it hits _doing_it_wrong and returns an empty array. max([]) then fatals.
There was a problem hiding this comment.
Is this masking maybe a lower level issue? Shouldn't this also ensure that get_feed_build_date() works when posts is a list of post IDs and not WP_Post objects? This would fix the underlying issue.
Now, it would sense still to include the check added in this PR to prevent passing an empty array to max(), but I see this as additional hardening on top of a more fundamental issue that can be fixed.
There was a problem hiding this comment.
I think this may be needed in both of these places:
wordpress-develop/src/wp-includes/feed.php
Line 724 in 59a9905
wordpress-develop/src/wp-includes/feed.php
Line 729 in 59a9905
Instead of using wp_list_pluck(), it should be using array_map() and just pass through the value if it is an integer. Otherwise, it can return the ID property of the respective object.
There was a problem hiding this comment.
You're right. The original fix only stopped the fatal, but it returned the global last modified time instead of the expected post. The bug was deeper down, and now it returns the timestamp for the correct post.
Follow WordPress inline documentation standards section 5.2 — multi-line comments use /* */ with leading asterisks, not repeated //. Addresses follow-up feedback from @westonruter. Props westonruter. See #59956.
…jects. Replace wp_list_pluck() with array_map() + array_filter() that resolves each entry via get_post() / get_comment(), supporting WP_Query results where $posts contains integer IDs (fields => 'ids') instead of WP_Post objects. Previously, a feed query using fields => 'ids' produced scalar IDs that wp_list_pluck() could not pluck post_modified_gmt from. The function would silently fall through to get_lastpostmodified() and return the site-wide latest modified time instead of the modified time of the posts actually in the feed. Adds a regression test asserting the build date matches the modified time of the post in the feed when fields => 'ids' is used. Follow-up to [60079].
Trac Ticket
See https://core.trac.wordpress.org/ticket/59956
Description
get_feed_build_date()passes the result ofwp_list_pluck()directly tomax()without checking for an empty array. When the query's post objects lack thepost_modified_gmtproperty,wp_list_pluck()returns an empty array andmax()throws aValueErroron PHP 8.0+.This happens in production when bots request RSS feed URLs (e.g.
/author/{name}/feed/,/taxonomy/{term}/feed/) for terms or authors with zero published posts. Thehave_posts()guard on line 722 passes (the query object exists and has a truthypost_count), but the underlying post objects are incomplete — sowp_list_pluck( $wp_query->posts, 'post_modified_gmt' )returns[].On PHP 7.x this was a silent warning. On PHP 8.0+ it is a fatal
ValueErrorthat returns HTTP 500 to the crawler.The Fix
Wrap the
max()call inif ( ! empty( $modified_times ) ). When the array is empty,$datetimeremainsfalseand falls through to the existingget_lastpostmodified( 'GMT' )fallback on line 741. No behavioral change for feeds that have valid post data.Testing
Automated: Added
test_should_not_error_when_modified_times_is_emptytotests/phpunit/tests/date/getFeedBuildDate.php. The test simulates a query wherehave_posts()is true but posts lackpost_modified_gmt, and verifies the function falls back toget_lastpostmodified()instead of throwing.Manual (production): Verified on a WordPress 6.9.4 multisite (PHP 8.4) with 1,175 artist taxonomy terms that had zero published posts on the main site. Before the fix, these feed URLs returned HTTP 500 with
ValueError: max(): Argument #1 ($value) must contain at least one element. After the fix, all return HTTP 200 with valid empty RSS feeds. Normal feeds with posts are unaffected.debug.log— fatal errors stopped immediately after applying the fix (were firing every 10–20 seconds prior)<item>elements and<lastBuildDate>AI Disclosure
AI assistance: Yes
Tool(s): Claude Code (Claude Opus 4.6 directly on a VPS via OpenCode)
Used for: Identifying the root cause from production debug logs, drafting the guard condition, and scaffolding the unit test. Final implementation, testing, and PR description were reviewed and edited by a human contributor.