-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Feeds: Guard against empty array passed to max() in get_feed_build_date() #11387
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
chubes4
wants to merge
8
commits into
WordPress:trunk
Choose a base branch
from
chubes4:fix/59956-feed-build-date-empty-array
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+118
−5
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
beb5070
Feeds: Guard against empty array passed to max() in get_feed_build_da…
chubes4 eeb7bd5
Fix PHPCS: align equals signs in test assignments.
chubes4 5b79996
Tests: Fix test to match actual production trigger for empty modified…
chubes4 cdfa9f5
Merge branch 'trunk' into fix/59956-feed-build-date-empty-array
chubes4 c0fe548
Merge branch 'trunk' into fix/59956-feed-build-date-empty-array
westonruter 6b4c4c3
Feeds: Address review feedback on get_feed_build_date() empty array fix.
chubes4 7962e11
Tests: Use multi-line block comment style in get_feed_build_date test.
chubes4 ec8dfad
Feeds: Resolve scalar post IDs in get_feed_build_date() to WP_Post ob…
chubes4 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test currently uses
query_posts( [ 'fields' => 'ids' ] )to forcewp_list_pluck()to return an empty array, which requiressetExpectedIncorrectUsage( '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 settingglobal $wp_queryto a freshWP_Querywithpost_count = 1(sohave_posts()is true) but an emptypostsarray (sowp_list_pluck()returns[]). This keeps the test focused on guardingmax()against empty input.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chubes4 Would this replicate the original scenario that you've encountered? Curious what your
WP_Queryhas in it which caused this issue in the first place.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
artisttaxonomy, and a customdata_machine_eventspost type.When there are existing
data_machine_eventsposts using a givenartistterm, but 0 regular posts, and we query a feed URL withfields => 'ids'set,WP_Queryreturns an array of scalar ids instead ofWP_Postobjects.wp_list_pluck()requires each element to be an object or array, so it hits_doing_it_wrongand returns an empty array.max([])then fatals.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this masking maybe a lower level issue? Shouldn't this also ensure that
get_feed_build_date()works whenpostsis a list of post IDs and notWP_Postobjects? 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.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 usingarray_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.