Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connectors: Check the request type and unregister connectors that don't support it #1269

Closed
wants to merge 1 commit into from
Closed

Connectors: Check the request type and unregister connectors that don't support it #1269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions classes/class-connector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,13 @@ abstract class Connector {
*/
public $register_frontend = true;

/**
* Register connector for API requests
*
* @var bool
*/
public $register_api = true;

/**
* Holds connector registration status flag.
*
Expand Down
39 changes: 29 additions & 10 deletions classes/class-connectors.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ class Connectors {
public function __construct( $plugin ) {
$this->plugin = $plugin;
$this->load_connectors();

// Run this after the REST API has been initialized.
add_action( 'parse_request', array( $this, 'check_request_type' ), 11 );
}

/**
Expand Down Expand Up @@ -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 ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

continue;
}

// Check if the connector events are allowed to be registered in the WP Frontend.
if ( ! is_admin() && ! $class->register_frontend ) {
continue;
}

// Run any final validations the connector may have before used.
if ( $class->is_dependency_satisfied() ) {
$classes[ $class->name ] = $class;
Expand Down Expand Up @@ -274,4 +267,30 @@ public function reload_connector( $name ) {
$this->connectors[ $name ]->register();
}
}

/**
* Determines the request type (admin, api, frontend) and unregisters connectors that don't support it.
*/
public function check_request_type() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

if ( ! did_action( 'parse_request' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this solving for? Are there cases when this could be called even when parse_request hasn't run?

return;
}

$is_admin = is_admin();
$is_api = wp_stream_is_api_request();

foreach ( $this->connectors as $connector ) {
if ( ! $connector->is_registered() ) {
continue;
}

if (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a lot of nesting and conditionals. Could we somehow refactor this for readability and maybe add unit tests?

( $is_admin && ! $connector->register_admin )
|| ( $is_api && ! $connector->register_api )
|| ( ! $is_admin && ! $is_api && ! $connector->register_frontend )
) {
$connector->unregister();
}
}
}
}
11 changes: 11 additions & 0 deletions includes/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,3 +148,14 @@ function wp_stream_min_suffix() {

return $min;
}

/**
* Check if the current request is being processed by the REST API or XMLRPC.
*
* Can only be called after priority 10 of the `parse_request` action.
*
* @return bool
*/
function wp_stream_is_api_request() {
return ( ( defined( 'XMLRPC_REQUEST' ) && XMLRPC_REQUEST ) || ( defined( 'REST_REQUEST' ) && REST_REQUEST ) );
}