Connectors: Check the request type and unregister connectors that don't support it #1269
Conversation
There was a problem hiding this comment.
This is great! Thanks so much for working on this @coreymckrill!
Please see my initial feedback inline the pull request.
| continue; | ||
| } | ||
|
|
||
| if ( |
There was a problem hiding this comment.
This looks like a lot of nesting and conditionals. Could we somehow refactor this for readability and maybe add unit tests?
| * Determines the request type (admin, api, frontend) and unregisters connectors that don't support it. | ||
| */ | ||
| public function check_request_type() { | ||
| if ( ! did_action( 'parse_request' ) ) { |
There was a problem hiding this comment.
What is this solving for? Are there cases when this could be called even when parse_request hasn't run?
| /** | ||
| * Determines the request type (admin, api, frontend) and unregisters connectors that don't support it. | ||
| */ | ||
| public function check_request_type() { |
There was a problem hiding this comment.
The naming of this suggests that it only checks for the request type but it does that and also unloading of the connectors. Could we somehow decouple these two actions to make this easier to test?
| @@ -119,16 +122,6 @@ public function load_connectors() { | |||
| continue; | |||
| } | |||
|
|
|||
| // Check if the connector events are allowed to be registered in the WP Admin. | |||
| if ( is_admin() && ! $class->register_admin ) { | |||
There was a problem hiding this comment.
By registering all connectors during the plugin init and deregistering some during parse_request introduces a period when all connectors are active for some period of time which can lead to issues and situations which are hard to debug.
Could we maybe delay registration of all connectors until parse_request when the context of the request (admin, REST, frontend) is truly known. Need to verify it, though.
|
This has been fixed by #1264. |
This is an alternative approach to #1264. Since requests to WP APIs are not considered to be
is_admin, connectors that are disabled for the "frontend" are also disabled for API requests, which is not always (or maybe ever) intended. In this PR we allow all connectors to be registered, regardless ofis_admin, and then take the following steps when theparse_requestaction is fired (this is when the REST API is initialized):register_admin,register_frontend, and the newregister_apiclass properties)This addresses the issue of the Posts connector not logging post changes that happen via the REST API, without changing its behavior for actual frontend requests.
Fixes #1195
Fixes #1250
Checklist
contributing.md).