Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#45611 closed defect (bug) (fixed)

REST API: Attachment controller calls undefined parent method

Reported by: dlh's profile dlh Owned by: kadamwhite's profile kadamwhite
Milestone: 5.2 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch
Focuses: Cc:

Description

WP_REST_Attachments_Controller contains a public validate_user_can_query_private_statuses() method that ends with a call to parent::validate_user_can_query_private_statuses(). However, the parent method was removed in [39104].

It seems possibly unintentional that the method still exists at all in WP_REST_Attachments_Controller; I don't see any uses of it in core.

The attached patch would update the method to pass to the parent sanitize_post_statuses() method that was added in [39104]. (Perhaps validate_user_can_query_private_statuses() should also be deprecated?)

Attachments (2)

45611.diff (734 bytes) - added by dlh 5 years ago.
45611.2.diff (1.1 KB) - added by dlh 5 years ago.

Download all attachments as: .zip

Change History (16)

@dlh
5 years ago

#1 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 5.1

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


5 years ago

#3 @desrosj
5 years ago

  • Keywords has-unit-tests added

This is looking good. It would be great to have some tests, though. @dlh are you able to tackle that?

#4 @dlh
5 years ago

  • Keywords needs-unit-tests added; has-unit-tests removed

I'll do my best to add tests in time for 5.1!

#5 @pento
5 years ago

  • Milestone changed from 5.1 to 5.2

Aye, this should probably be deprecated. I'm nearly inclined to just remove it, except that it is technically possible to call it without causing a PHP error.

Either way, I'd prefer to land this early, so moving to 5.2.

#6 @kadamwhite
5 years ago

Agree this should be deprecated. We can get this in for 5.2 soon if we can get some tests; @dlh are you still interested in adding those, or would you like some assistance?

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


5 years ago

#8 @kadamwhite
5 years ago

Update: After searching through the plugin directory I do not see any usage of this method. (Every occurrence of validate_user_can_query_private_statuses is in relation to a plugin-defined local class method, all of them copy-pasted from the original V2 REST API and none of which reference this controller or any parent.) On those grounds I'm going to agree with @pento that I think we can remove it entirely.

#9 @dlh
5 years ago

I'd still be interested, although it might be a couple weeks until I'm able to write them, so anyone else who has time before then is definitely welcome.

I'm not certain I follow, though, whether tests would still be needed if the method is removed? Would these be additional tests for sanitize_post_statuses() or for something else?

This ticket was mentioned in Slack in #core-restapi by dlh. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


5 years ago

@dlh
5 years ago

#13 @dlh
5 years ago

  • Keywords needs-unit-tests removed

45611.2.diff removes the method per comment:8.

Also removing the needs-unit-tests keyword per discussion in Slack.

#14 @kadamwhite
5 years ago

  • Owner set to kadamwhite
  • Resolution set to fixed
  • Status changed from new to closed

In 44934:

REST API: Remove unused attachments controller method.

The validate_user_can_query_private_statuses method is itself unused, and calls a parent class method previously removed in r39104.

Props dlh.
Fixes #45611.

Note: See TracTickets for help on using tickets.