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.

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

Keyring_Error is sometimes instantiated in a way that is incompatible with usage of WP_Error #103

Open
jorgeatorres opened this issue Mar 2, 2022 · 0 comments

Comments

@jorgeatorres
Copy link

jorgeatorres commented Mar 2, 2022

In the implementation for most Keyring services (maybe all?) a request that results in an error returns an instance of Keyring_Error.

This is all good until you see that Keyring_Error is a subclass of WP_Error with no specific implementation, and as such, you would expect that it'd be compatible with how WP_Error is used in WP. For example, the following standard error-checking pattern from the WP world can result in a PHP notice/warning/error if $result is of type Keyring_Error:

if ( is_wp_error( $result ) ) {
    echo $result->get_error_message();
}

In particular, because there are a bunch of places in the codebase where Keyring_Error is instantiated passing an error code as first argument and the HTTP response object as second argument, the "error message" stored in the object is frequently not a string. This can happen, for example, if the server returns something other than a 2xx code.

For comparison, this is WP_Error's constructor signature:

public function __construct( $code = '', $message = '', $data = '' )

The second argument should be a string, which would also enable Keyring_Error::get_error_message() to return something that can be printed on screen or logged to a log file.

Most likely, the response was meant to go into the 3rd parameter ($data), which can in fact be of any type.


Full example:

add_action(
	'keyring_load_services',
	function() {

		class Keyring_Dummy_Service extends Keyring_Service_OAuth2 {

			public function get_name() {
				return 'dummy';
			}

			public function requires_token($does_it = null) {
				return false;
			}

		}

		Keyring_Dummy_Service::init();
	}
);

add_action( 'init', function() {

	Keyring::init();

	$k = Keyring::get_service_by_name( 'dummy' );
	$result = $k->request( 'https://amazon.com/nothinghere');

	if ( is_wp_error( $result ) ) {
		// Results in "Array to string conversion" PHP notice.
		echo $result->get_error_message();
	}

} );
@jorgeatorres jorgeatorres changed the title Keyring_Error is sometimes instantiated in a way that is incompatible with the definition of WP_Error Keyring_Error is sometimes instantiated in a way that is incompatible with usage of WP_Error Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant