Make WordPress Core

Opened 9 years ago

Closed 3 years ago

#30009 closed defect (bug) (fixed)

_wp_put_post_revision checks for errors incorrectly

Reported by: rmccue's profile rmccue Owned by: whyisjake's profile whyisjake
Milestone: 5.8 Priority: normal
Severity: minor Version: 2.6
Component: Revisions Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

_wp_put_post_revision uses wp_insert_post, then checks is_wp_error to pass it up the stack (lines 259-261). However, wp_insert_post only returns a WP_Error if the second param is true. In this case, it is not.

Introduced in r7907.

Attachments (6)

30009.diff (497 bytes) - added by rmccue 9 years ago.
Specify we want a WP_Error on insertion error
30009.2.diff (1.3 KB) - added by rmccue 9 years ago.
Correct some coding standards while we're in here
30009.3.diff (1.5 KB) - added by adamsilverstein 9 years ago.
update docblock
30009.4.diff (482 bytes) - added by adamsilverstein 8 years ago.
30009.5.diff (1.2 KB) - added by adamsilverstein 3 years ago.
30009.6.diff (1.3 KB) - added by adamsilverstein 3 years ago.

Download all attachments as: .zip

Change History (17)

@rmccue
9 years ago

Specify we want a WP_Error on insertion error

@rmccue
9 years ago

Correct some coding standards while we're in here

#1 follow-up: @rmccue
9 years ago

  • Keywords has-patch added
  • Owner set to rmccue
  • Status changed from new to assigned

Attached patches specify that we want a WP_Error back from wp_insert_post, pretty straight forward patch.

30009.diff fixes the problem, 30009.2.diff goes a bit further and corrects coding standards in the function while we're in there.

@adamsilverstein
9 years ago

update docblock

#2 in reply to: ↑ 1 @adamsilverstein
9 years ago

looks good. I updated the doc block slightly in 30009.3.diff to reflect the return value.

I tried to write a unit test for this and couldn't actually get wp_insert_post to return an error - it only does so when get_post( post_id ) fails (returns null) and that check already happens earlier in _wp_put_post_revision. For this reason, I believe we could remove the entire error check conditional, we are returning $revision_id in any case...

Good catch in any case, the check as is will never work!

Replying to rmccue:

Attached patches specify that we want a WP_Error back from wp_insert_post, pretty straight forward patch.

30009.diff fixes the problem, 30009.2.diff goes a bit further and corrects coding standards in the function while we're in there.

#3 @adamsilverstein
8 years ago

  • Keywords dev-feedback added

30009.4.diff

  • Refresh against trunk

Lets keep this patch simple and stick to adding the true value to wp_insert_post so it will potentially return the WP_Error we are looking out for.

#6 @coreymckrill
3 years ago

Just came here to report this same issue. Looks like 30009.4.diff would be a quick fix.

#7 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.8
  • Owner changed from rmccue to SergeyBiryukov
  • Status changed from assigned to reviewing

#9 @adamsilverstein
3 years ago

  • Keywords has-unit-tests commit added; dev-feedback removed

@SergeyBiryukov I added a test and am running all tests in https://github.com/WordPress/wordpress-develop/pull/1279.

If everything looks good, this is ready for commit. Before this fix is_wp_error on line 274 would never pass!

#10 @adamsilverstein
3 years ago

30009.5.diff includes a test to verify a WP_Error is returned now. I was able to trigger an error by passing an invalid post ID directly to the inner function.

#11 @whyisjake
3 years ago

  • Owner changed from SergeyBiryukov to whyisjake

#12 @whyisjake
3 years ago

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

In 51124:

Revisions: Check and return errors for insertions to revisions.

Fixes #30009.

Props rmccue, adamsilverstein, coreymckrill, whyisjake.

Note: See TracTickets for help on using tickets.