Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#48957 closed defect (bug) (fixed)

Call to a member function format() on boolean in wp-includes/feed.php

Reported by: dd32's profile dd32 Owned by: rarst's profile Rarst
Milestone: 5.3.2 Priority: high
Severity: normal Version: 5.3.1
Component: Date/Time Keywords: has-patch has-unit-tests fixed-major
Focuses: Cc:

Description

[46774] / #48675 is causing a PHP fatal on buddypress.org, that may ultimately be caused by BuddyPress, but either way is a new core fatal condition as of 5.3.1.

Fatal error: Uncaught Error: Call to a member function format() on boolean in wp-includes/feed.php on line 692

The root cause of the issue is that the WP_Post object appears to have been emptied by BP similar to this: https://github.com/buddypress/BuddyPress/blob/fe47890c1ab7e2cb24c005f1c35fb9c6c8c8ab7c/src/bp-members/classes/class-bp-members-theme-compat.php#L119-L136

That ultimately means that $modified_times is [ 0 ] which causes date_create_immutable_from_format() to return false instead of the expected DateTime object.

cc @SergeyBiryukov @johnjamesjacoby

Attachments (2)

test-48957.diff (913 bytes) - added by tellyworth 4 years ago.
Unit test to reproduce the bug
48957-feed-build-date-fallback.patch (5.3 KB) - added by Rarst 4 years ago.

Download all attachments as: .zip

Change History (19)

#1 @dd32
4 years ago

This was triggered on the /members/feed/ buddypress feed, and while buddypress shouldn't be messing with the globals like that this much, core should still be defensive here

#2 @Rarst
4 years ago

Ugh. Updating from fuzzy inputs / fuzzy outputs hurts.

Will patch up a check there.

#3 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.3.2

#4 @azaozz
4 years ago

  • Priority changed from normal to high

What would be the best fix here? I can see this happening in other cases/for CPTs perhaps. Would it be enough to check the returned value from date_create_immutable_from_format() or perhaps have a wrapper for it that can handle ...other exceptions/errors?

Looking at the PHP docs (for date_create_from_format which is like date_create_immutable_from_format), specifically at https://www.php.net/manual/en/datetime.createfromformat.php#refsect1-datetime.createfromformat-parameters, it implies some possible errors when the same strings like for date() are used.

Setting this to high priority as a fatal is a fatal, needs fixing asap :)

#5 @dd32
4 years ago

Would it be enough to check the returned value from date_create_immutable_from_format()

I think checking the return value is in general probably enough, with the expectation that it'll take appropriate action to try to make sure it's valid.

In this case, max( $modified_times ) is probably enough to attempt to get a valid date, avoiding the 0 and 0000-00-00 00:00:00 dates if there's valid data available.

The question just remains - what kind of output should this function have in the event of this error condition?
It looks like the fallback higher up in the function for get_lastpostmodified() is probably a good option, so:

if ( ! $datetime ) {
    // Fallback to last time any post was modified or published.
    return get_lastpostmodified( 'GMT' );
}

@tellyworth
4 years ago

Unit test to reproduce the bug

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


4 years ago

#7 @Rarst
4 years ago

  • Owner set to Rarst
  • Status changed from new to assigned

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


4 years ago

#9 @Rarst
4 years ago

Working on it, but turns out existing fallback is incorrect because it doesn't respect the $format argument at all.

Writing out the test a bit for that and looking to adjust the fallback implementation.

#10 @Rarst
4 years ago

  • Keywords has-patch has-unit-tests added

Ok, take on a patch. This ensures:

  1. we do not error
  2. we fall back in all necessary cases
  3. we respect format when falling back
  4. we pass all cases through the filter
  5. inline documentation is clear on what it does and possibility of false on failure (which was already possible, but not documented)
Last edited 4 years ago by Rarst (previous) (diff)

#11 @Rarst
4 years ago

I am not 100% confident on passing false case through the filter though. I hate it when return filter randomly doesn't apply to some of the branches, but in modern PHP I would not add a different type to something established. Previously fallback case could return false but w/o passing through the filter.

Intuitively I think passing everything and updating documentation is the way this time, but open to feedback.

#12 @SergeyBiryukov
4 years ago

In 46973:

Docs: Clarify that get_lastpostdate() and get_lastpostmodified() can return false on failure.

The both use _get_last_post_time() internally.

Props Rarst.
See #48957.

#13 @SergeyBiryukov
4 years ago

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

In 46974:

Date/Time: Ensure that get_feed_build_date() correctly handles a modified post object with invalid date.

  • Clarify in the documentation that the function returns false on failure.
  • Consistently pass the return value through the get_feed_build_date filter.

Props Rarst, dd32, azaozz, tellyworth.
Fixes #48957.

#14 @SergeyBiryukov
4 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 5.3.2.

#15 @SergeyBiryukov
4 years ago

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

In 46977:

Date/Time: Ensure that get_feed_build_date() correctly handles a modified post object with invalid date.

  • Clarify in the documentation that the function returns false on failure.
  • Consistently pass the return value through the get_feed_build_date filter.

Props Rarst, dd32, azaozz, tellyworth.
Merges [46974] and [46973] to the 5.3 branch.
Fixes #48957.

#16 @SergeyBiryukov
4 years ago

In 46981:

Tests: Use delta comparison in test_should_fall_back_to_last_post_modified() to avoid race conditions.

See #48957.

#17 @SergeyBiryukov
4 years ago

In 46982:

Tests: Use delta comparison in test_should_fall_back_to_last_post_modified() to avoid race conditions.

Merges [46981] to the 5.3 branch.
See #48957.

Note: See TracTickets for help on using tickets.