WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#34219 closed task (blessed) (fixed)

Move filters out of `rest_api_default_filters()`

Reported by: swissspidy Owned by: rmccue
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch commit
Focuses: Cc:

Description

I just noticed the following docblock for rest_api_default_filters():

@internal This will live in default-filters.php

I don't know the details, but it looks like this was forgotten during the merge. The attached patch moves those filters to default-filters.php. In order to achieve this, rest_handle_deprecated_function and rest_handle_deprecated_argument are modified and also made private. It also introduces a new _rest_deprecated_trigger_error function to disable errors during a REST request.

Note that I combined the two REST API sections in default-filters.php because (contrary to the inline comments) both are mostly for actions, not for filters. Having everything in one place makes it easier to find related hooks.

Another option is to just remove that @internal comment and explain why it is that way.

Attachments (3)

34219.diff (6.5 KB) - added by swissspidy 5 years ago.
34219.2.diff (1.5 KB) - added by swissspidy 5 years ago.
34219.3.diff (575 bytes) - added by rachelbaker 5 years ago.

Download all attachments as: .zip

Change History (14)

@swissspidy
5 years ago

#1 @swissspidy
5 years ago

  • Component changed from General to REST API

#2 follow-up: @rmccue
5 years ago

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

These filters are only added on an API request, since we don't need them all the time. I think what we actually want here is to remove the @internal note, and add a note about why they're hooked separately into the function description.

#3 in reply to: ↑ 2 ; follow-up: @SergeyBiryukov
5 years ago

Replying to rmccue:

These filters are only added on an API request, since we don't need them all the time.

Could we add if ( defined( 'REST_REQUEST' ) ) { ... } to default-filters.php?

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

Replying to SergeyBiryukov:

Replying to rmccue:

These filters are only added on an API request, since we don't need them all the time.

Could we add if ( defined( 'REST_REQUEST' ) ) { ... } to default-filters.php?

REST_REQUEST is defined in rest_api_init(), which is hooked to init and therefore executed after default-filters.php.

#5 @wonderboymusic
5 years ago

  • Type changed from enhancement to task (blessed)

#6 @joehoyle
5 years ago

I'm -1 on the usage of the REST_REQUEST constant to change behaviour. It makes it more difficult to test (as constants of course can't be toggled on / off), and would be more difficult for middleware / higher level plugins to use the REST API internally.

@swissspidy
5 years ago

#7 @swissspidy
5 years ago

Alright, let's just remove the @internal note and update the documentation. See 34219.2.diff

#8 follow-up: @rachelbaker
5 years ago

@swissspidy In 34219.2.diff you are going outside the scope of this ticket and are also removing the trailing spaces left in r35351. I don't know if that was intentional, or how to best strip the trailing spaces and retain an accurate history. Thoughts @johnbillion?

Last edited 5 years ago by rachelbaker (previous) (diff)

#9 in reply to: ↑ 8 @swissspidy
5 years ago

Replying to rachelbaker:

@swissspidy In 34219.2.diff you are going outside the scope of this ticket and are also removing the trailing spaces left in r35351. I don't know if that was intentional, or how to best strip the trailing spaces and retain an accurate history. Thoughts @johnbillion?

Oops, not intentional. Was looking carefully not to include it and even wanted to create a new ticket for that.

@rachelbaker
5 years ago

#10 @rachelbaker
5 years ago

  • Keywords commit added

In 34219.3.diff refreshed @swissspidy's last patch, limiting the scope.

#11 @wonderboymusic
5 years ago

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

In 35474:

REST API: remove the @internal annotation from rest_api_default_filters().

Props swissspidy, rachelbaker.
Fixes #34219.

Note: See TracTickets for help on using tickets.