#45265 closed defect (bug) (fixed)
REST API: register_rest_route should warn when used improperly.
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (17)
#1
@
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
#4
@
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 areturn
. That could be a pretty bad breaking change for some setups. The other notices already in theregister_rest_route()
function have been in place and enforced with areturn
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 anmu-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.
#5
@
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.
#6
@
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
@
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()
.
#10
@
6 years ago
- Keywords needs-dev-note removed
The dev note has been posted: https://make.wordpress.org/core/2019/01/11/new-rest-api-notice-in-5-1/.
Throw a _doing_it_wrong. Assuming 5.1.0.