Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#45265 closed defect (bug) (fixed)

REST API: register_rest_route should warn when used improperly.

Reported by: kraftbj's profile kraftbj Owned by: desrosj's profile desrosj
Milestone: 5.1 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch has-dev-note
Focuses: rest-api Cc:

Description

According to the documentation, REST API endpoints should be hooked onto the rest_api_init action.

When this function is fired without a hook or on the wrong hook, it can result in unexpected behavior. For example, calling it directly from a mu-plugin to add an endpoint on a namespace that is also defined on the rest_api_init hook will result in the mu-plugin directly-called version completely overriding the namespace, instead of the expected merging to the namespace.

To help ensure intended use, we should _doing_it_wrong when register_rest_route is used outside of the expected hook.

Attachments (4)

45265.diff (1.1 KB) - added by kraftbj 6 years ago.
Throw a _doing_it_wrong. Assuming 5.1.0.
45265.2.diff (3.6 KB) - added by desrosj 6 years ago.
45265.3.diff (3.7 KB) - added by desrosj 6 years ago.
45265.4.diff (1.4 KB) - added by desrosj 6 years ago.

Download all attachments as: .zip

Change History (17)

@kraftbj
6 years ago

Throw a _doing_it_wrong. Assuming 5.1.0.

#1 @SergeyBiryukov
6 years ago

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

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


6 years ago

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


6 years ago

@desrosj
6 years ago

#4 @desrosj
6 years ago

  • Keywords needs-dev-note added
  • Owner changed from SergeyBiryukov to desrosj

Thanks @kraftbj! This was discussed today's bug scrub and then again after in #core-restapi. Here is a summary of the discussion:

  • This _doing_it_wrong() notice should not be accompanied by a return. That could be a pretty bad breaking change for some setups. The other notices already in the register_rest_route() function have been in place and enforced with a return since the REST API was merged into Core.
  • I moved the new notice below the pre-existing ones. If the preexisting ones are already being triggered on a site, they should continue to be the notice that appears until fixed.

When I first started testing this, I realized that 66 of the test methods in Core were failing. This is because the methods do not call register_rest_route() on the action hook. It was decided that if ( ! did_action( 'rest_api_init' ) && ! doing_action( 'rest_api_init' ) ) was a better approach and would cut down on the number of notices. This change brought the number of failures down to 11, and those are fixed in 45265.2.diff by including @expectedIncorrectUsage notations.

I am marking this as needs-dev-note mainly because of the potential for these failures to start happening in plugin and theme test suites that call register-rest_route() in a similar manner. But, this is also a good opportunity to educate on why this is the correct way to register routes. A few reasons that were discussed:

  • Performance implications: One call to register_rest_route causes all plugins to register their routes.
  • If called incorrectly before plugins are able to register their rest_api_init hooks (say in an mu-plugin file), then the hook will not fire a second time and those endpoints will never be registered.

I will prepare a rough draft dev note. If it can be published in the next 5-7 days, I think that this could still go into 5.1.

@desrosj
6 years ago

#5 @desrosj
6 years ago

Sorry for the noise. I forgot to include an added line for the inline documentation that specifies the function should not be called before rest_api_init. This follows a similar pattern on register_post_type() and register_taxonomy(). 45265.3.diff includes that.

@desrosj
6 years ago

#6 @desrosj
6 years ago

In 45265.4.diff I was able to fix the failing tests without using @expectedIncorrectUsage. Other REST API test classes manually do_action( 'rest_api_init' ) in the setUp() method, but Tests_REST_API was not. Adding this fixes the failures.

Also, doing_action() is not needed when did_action() is called. The action counter is incremented at the start of the action, so did_action() will be true at the same time doing_action() is true for the first time.

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


6 years ago

#8 @TimothyBlynJacobs
6 years ago

The do_action( 'rest_api_init' ) in the test case should have the spy server passed to match the core behavior in rest_get_server().

#9 @desrosj
6 years ago

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

In 44568:

REST API: Encourage proper usage of register_rest_route().

Calling register_rest_route() too early in the loading process has the potential to cause some unintentional problems and pitfalls. Because register_rest_route() calls rest_get_server() (which creates the WP_REST_Server instance), calling the function directly and/or before rest_api_init should be discouraged.

For example, if register_rest_route () is called on init, the REST API server instance is set up (and all functions added to rest_api_init and other related hooks are invoked), even though the current request may not be a REST request. Also, if register_rest_route() is called even earlier (say, in an mu-plugin file), required endpoints may be missing since normal plugins have not yet been loaded and have not had a chance to register their own action hooks.

This adds a _doing_it_wrong() notice the first time register_rest_route() is called before rest_api_init in a request to encourage best practices for registering REST API routes.

Props kraftbj, desrosj, timothyblynjacobs.
Fixes #45265.

#10 @desrosj
6 years ago

  • Keywords needs-dev-note removed

#11 @desrosj
6 years ago

  • Keywords has-dev-note added

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


6 years ago

#13 @ocean90
6 years ago

In 44698:

REST API: Prevent translating the hook name in a _doing_it_wrong() message by using a placeholder.

See #45265.

Note: See TracTickets for help on using tickets.