Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#48653 closed defect (bug) (fixed)

PHP Notices when requesting a post with an unregistered status

Reported by: roytanck's profile roytanck Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.4 Priority: normal
Severity: normal Version: 3.0
Component: Posts, Post Types Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

Plugins can register new post statuses using register_post_status. When such a plugin is then disabled, it is possible that posts remain in the database with a post status that is no longer registered. In such cases, get_post_status_object will return null. This return value is not handled correctly in map_meta_cap and the WP_Query class, causing PHP notices.

Step to reproduce:

  • Set WP_DEBUG to true in wp-config.php.
  • Create posts with post_status that is not native to WordPress (I use "bogus"). The quickest way to fake this is to edit the database directly.
  • Request the post using the REST API, or by calling its guid/permalink in the browser.
  • PHP Notices will be shown, saying something like "Notice: Trying to get property 'public' of non-object".

This can be fixed by adding some defensive programming in the map_meta_cap function, and the WP_Query class. I'll attach a patch that suggests a way to do this.

Attachments (1)

48653.diff (2.9 KB) - added by roytanck 5 years ago.
Adds defensive programming to deal with get_post_status_object() returning null.

Download all attachments as: .zip

Change History (16)

@roytanck
5 years ago

Adds defensive programming to deal with get_post_status_object() returning null.

#1 @roytanck
5 years ago

  • Keywords has-patch added

#2 @SergeyBiryukov
5 years ago

  • Component changed from General to Posts, Post Types

Related: [34091], #16956.

#3 follow-up: @Rarst
5 years ago

I am not sure I agree with "doing_it_wrong" handling here, since there is nothing wrong with attempting the check. The problem is with invalid state of the system, not with developer making incorrect call. Sounds like a straight error case to me?

No opinion on implementation/behavior.

#4 in reply to: ↑ 3 @roytanck
5 years ago

Replying to Rarst:

I am not sure I agree with "doing_it_wrong" handling here, since there is nothing wrong with attempting the check. The problem is with invalid state of the system, not with developer making incorrect call. Sounds like a straight error case to me?

No opinion on implementation/behavior.

Thank you. Using doing_it_wrong here was simply "ego-less programming", since the rest of the function uses this in similar situations. The point is to catch the invalid state and provide meaningful feedback. Chances are the developer doing the check is unaware of previously registered post statuses still lingering in the database.

#5 follow-up: @peterwilsoncc
5 years ago

  • Keywords needs-unit-tests added

I agree a notice would be preferable but better to stick with _doing_it_wrong() for the sake of consistency with the post_type check even though it's not quite right. (Don't get me started on the name of the function ;)

@roytanck Are you able to add some unit tests for the WP_Query and capability changes? It's no problem if you don't have time or aren't clear on the process though.

#6 in reply to: ↑ 5 @roytanck
5 years ago

Replying to peterwilsoncc:

I agree a notice would be preferable but better to stick with _doing_it_wrong() for the sake of consistency with the post_type check even though it's not quite right. (Don't get me started on the name of the function ;)

@roytanck Are you able to add some unit tests for the WP_Query and capability changes? It's no problem if you don't have time or aren't clear on the process though.

I'd love to dive into (WP) unit testing if I had more time, but somehow, WP_Query does not sound like the best place for first baby steps. Also, time unfortunately is an issue. If someone else could jump in, that would help keep this ticket moving.

#7 @jeremyfelt
5 years ago

  • Version changed from trunk to 3.0

#8 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 5.4
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#9 @SergeyBiryukov
5 years ago

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

In 47178:

Posts, Post Types: Fail gracefully when checking mapped cap against unregistered post status.

With map_meta_cap enabled for a post type, the read_post capability for posts with a public status is supposed to be mapped to the post type's read capability.

When a post is left in the database after the post status is no longer present, and WP does a read_post check against it, a PHP notice was thrown, and the cap check always failed.

As a more graceful fallback, the cap is now mapped onto edit_others_posts, which allows highly privileged users to be able to access orphaned content.

A _doing_it_wrong() notice is also added, so that developers and site administrators are aware that the cap mapping is failing in the absence of the registered post status.

Follow-up to [34091], which introduced a similar approach to checking mapped caps against an unregistered post type.

Props roytanck, SergeyBiryukov.
Fixes #48653.

#10 @SergeyBiryukov
5 years ago

In 47179:

Posts, Post Types: Fail gracefully when checking whether the post should be displayed in WP_Query::get_posts() against unregistered post status.

If the post status is not registered, assume it's not public.

Follow-up to [47178].

Props roytanck.
See #48653.

#11 @SergeyBiryukov
5 years ago

In 47180:

Posts, Post Types: Revert [47179] pending test failures investigation.

See #48653.

#12 @SergeyBiryukov
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#13 @SergeyBiryukov
5 years ago

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

In 47181:

Posts, Post Types: Fail gracefully when checking whether a single post with an unregistered post status should be displayed in WP_Query::get_posts().

If the post status is not registered, assume it's not public, but still allow access to users with edit permissions (same as for a protected post status, e.g. draft), so that they could recover orphaned content.

Add unit tests.

Follow-up to [47178].

Props roytanck, SergeyBiryukov.
Fixes #48653.

#14 follow-up: @audrasjb
5 years ago

Related: #49368

Post query is broken after [47178]. Looks like it would need some changes :-)

#15 in reply to: ↑ 14 @SergeyBiryukov
5 years ago

Replying to audrasjb:

Post query is broken after [47178]. Looks like it would need some changes :-)

Thanks for catching that! As noted in comment:4:ticket:49368, it should be fixed in [47181]. The nightly build has indeed ended up with the wrong commit, but is now refreshed.

Note: See TracTickets for help on using tickets.