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

Consider using objects for external dependencies #94

Open
GaryJones opened this issue May 16, 2020 · 0 comments
Open

Consider using objects for external dependencies #94

GaryJones opened this issue May 16, 2020 · 0 comments
Assignees

Comments

@GaryJones
Copy link
Contributor

GaryJones commented May 16, 2020

I was looking through the recent Support Label feature (which will be great when released), and noticed how some expectations of the Repo Metas API were hard-coded.

For example, the endpoint path where the meta data is collected from, the names of the headers for user ID and access token, the fact that the response is expected back as JSON, or that the response should have status and data->support_tier fields.

This leaking of how this external dependency works, makes re-using vip-go-ci by others much trickier. They would have to have an API that was setup identical to VIP's.

I know that vip-go-ci isn't object-orientated, but I wanted to share an idea anyway.


How about if we could encapsulate this knowledge about the external dependency (repo meta API) into a class?

For the particular case of the WPVIP repo meta, we could have an class like:

<?php
final class WPVIP_Repo_Meta_API {
	private $base_url = '...';
	private $endpoint_path = '/v1/sites?active=1&page=1&pagesize=20&source_repo=';
	private $user_id_header: 'API-User-ID';
	private $access_token_header: 'Access-Token:'
	private $response_type = 'json';

	public function get_endpoint( string $repo_owner, string $repo_name ): string {
		return $this->endpoint_path . $repo_owner . '/' . $repo_name;
	}

	// Basic getters for each of the private properties would go here.
}

Now, someone else might have their own class for their API:

<?php
final class Gamajo_Repo_Meta_API {
	private $base_url = 'https://api.example.com';
	private $endpoint_path = '/v3/sites/%repo-owner%/%repo-name%';
	private $user_id_header: 'User-ID';
	private $access_token_header: 'Access-Code:'
	private $response_type = 'xml';

	public function get_endpoint( string $repo_owner, string $repo_name ): string {
		return str_replace(
			[ '%repo-owner%', '%repo-name%' ],
			[ $repo_owner, $repo_name ],
			$this->endpoint_path
		);
	}

	// Basic getters for each of the private properties would go here.
}

They have a different api version number, a different endpoint URL format, different header names and a different response type.

Now, in the vip-go-ci code, you could then have a Repo Meta Fetcher object - that is, a class which takes the configuration object, and uses it to do the fetching - wherever it may be.

But first, since we'll need the Fetcher to interact with the config classes to pull out the values, we'll need to ensure that we have a contract:

<?php
final interface Repo_Meta_API {
	public function get_endpoint(): string;
	public function get_base_url(): string;
	public function get_user_id_header(): string;
	public function get_access_token_header(): string;
	public function get_reponse_type(): string;
}

// Then the existing config classes can implement this.
final class WPVIP_Repo_Meta_API implements Repo_Meta_API {...}

final class Gamajo_Repo_Meta_API implements Repo_Meta_API {...}

Now the Fetcher can be created, and say that it should expect an implementation of this API config interface, and can use it when doing the fetching:

<?php
final class Repo_Meta_API_Fetcher {
	private $repo_meta_api;

	public function __construct( Repo_Meta_API $repo_meta_api ) {
		$this->repo_meta_api = $repo_meta_api;
	}

	public function fetch( $repo_owner, $repo_name, $user_id, $access_token ): string {
		// Now can set up to use cURL, and can pull the base URL, endpoint,
		// header names, etc. from the $this->repo_meta_api property.

		...

		$response = curl_exec( $ch );

		return $response; // Could be a string of actual data, or change to an empty string on failure.
	}
}

The Fetcher would go off and just fetch the data - but not do anything with it. We'd want to be able to save that into the existing cache that vip-go-ci utilises, so we need to be able to parse the response. By default, we'd have a JSON parser:

<?php
final class WPVIP_Repo_Meta_API_JSON_Parser {
	public function parse( string $response ) {
		// json_decode() the response into an array.
	}
}

But maybe someone else wants to support an API that returns XML instead:

<?php
final class Gamajo_Repo_Meta_API_XML_Parser {
	public function parse( string $response ) {
		// SimpleXML or xml_parse_into_struct() to convert it into an array.
	}
}

Or maybe someone else want to provide a parser that handles the API data when it returns plain CSV text:

<?php
final class Gamajo_Repo_Meta_API_CSV_Parser {
	public function parse( string $response ) {
		return explode( ',', $response );
	}
}

Or even a different JSON parser, because the JSON from their API is structured differently.

The aim would be to simply get that string, and turn it into an array that contains the data, and right now, the only data field needed here is support_tier. Instead of an array, you could get the parsers to return a particular object:

<?php
final class Repo_Meta_API_Data {
	
}

And add an interface for all of those parsers:

<?php
final interface Repo_Meta_API_Parser {
	public function parse( string $response ): Repo_Meta_API_Data;
}

// Then update the return type for the parse() method on the parsers.

Youre existing code could then take that array of data (which was always our goal here), and save it to the cache.

In terms of wiring this up within the existing procedural code, you would would allow CLI options that accepted a path to a class file (and if these were set, they would override the existing CLI options you recently added):

--repo-meta-api-class=path/to/concrete/instance/of/Repo_Meta_API
--repo-meta-api-parser=path/to/concrete/instance/of/Repo_Meta_API_Fetcher

These class files could be require_once()'d' in vip-go-ci.php, and then the classes instantiated and passed as optional arguments to vipgoci_repo_meta_api_data_fetch().


This was more about a thought experiment than a strong recommendation, though I do think following the usual good practices of encapsulation, inversion of dependencies and so on, would make for stronger, more extensible code that would allow others to use this Support Label feature with their own API that differs from what VIP has.

There could equally be done with others APIs, such as hashes or even GitHub (in theory, this project could support self-hosted GitLab, or Bitbucket), or how logging is implemented (format of entry, or send to remote logging like Logstash or Graylog instead of simply echoing locally), or other implementations of the cache (support external cache, not just in-memory), or consistency of how to send messages to IRC, Pixel, etc., using different authentication methods (not tied to OAuth1), and so on.

@gudmdharalds gudmdharalds self-assigned this Jan 26, 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

2 participants