Skip to content

Added PluginClientBuilder #40

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
168 changes: 168 additions & 0 deletions src/PluginClientBuilder.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
<?php

namespace Http\Client\Common;

use Http\Client\HttpClient;

/**
* A builder that help you build a PluginClient.
*
* @author Tobias Nyholm <tobias.nyholm@gmail.com>
*/
class PluginClientBuilder implements PluginClientBuilderInterface
{
/**
* @var Plugin[]
*/
protected $plugins = [];

/**
* @var bool
*/
protected $rebuildClient = true;

/**
* The last created client with the plugins
*
* @var PluginClient
*/
private $pluginClient;

/**
* The object that sends HTTP messages
*
* @var HttpClient
*/
private $httpClient;

/**
*
* @param HttpClient $httpClient
*/
public function __construct(HttpClient $httpClient = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

why is that optional? this feels wrong if we don't use discovery for it.

{
$this->httpClient = $httpClient;
}

/**
* Add a new plugin to the end of the plugin chain.
*
* @param Plugin $plugin
*/
public function addPlugin(Plugin $plugin)
{
$this->plugins[] = $plugin;
$this->rebuildClient = true;
}

/**
* Remove a plugin by its fully qualified class name (FQCN).
*
* @param string $fqcn
*/
public function removePlugin($fqcn)
{
foreach ($this->plugins as $idx => $plugin) {
if ($plugin instanceof $fqcn) {
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure this works? i thought with strings, we need is_subclass_of?

Copy link
Member

Choose a reason for hiding this comment

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

I think it does. You need is_subclass_of when you don't have an instance, just the class name.

unset($this->plugins[$idx]);
$this->rebuildClient = true;
}
}
}

/**
* Alias for removePlugin and addPlugin
*
* @param Plugin $plugin
*/
public function replacePlugin(Plugin $plugin)
{
$this->removePlugin(get_class($plugin));
Copy link
Contributor

Choose a reason for hiding this comment

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

what if there are several plugins with the same class ? we would remove all of them. not sure if that is a problem. i guess its the fuziness of builder.

we also don't respect the position, which might be more of an issue. could we put the plugin in the position where the one we remove was?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should not have this method but have people get the plugins, change the list and set plugins. RequestMatcherPlugin for example is quite likely to be used multiple times.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. I'd say make adding irrevocable.

$this->addPlugin($plugin);
}

/**
* Get all plugins
*
* @return Plugin[]
*/
public function getPlugins()
{
return $this->plugins;
}

/**
* This will overwrite all existing plugins
*
* @param Plugin[] $plugins
*
* @return PluginClientBuilder
*/
public function setPlugins($plugins)
{
$this->plugins = $plugins;
$this->rebuildClient = true;
}

/**
* @return PluginClient
*/
public function buildClient()
{
if (!$this->httpClient) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should make sure in the constructor that this can not happen

throw new \RuntimeException('No HTTP client were provided to the PluginBuilder.');
}

if ($this->rebuildClient) {
$this->rebuildClient = false;
$this->pushBackCachePlugin();

$this->pluginClient = new PluginClient($this->httpClient, $this->plugins);
}

return $this->pluginClient;
}

/**
* @return boolean
*/
public function isModified()
{
return $this->rebuildClient;
}

/**
* @param HttpClient $httpClient
*/
public function setHttpClient(HttpClient $httpClient)
{
$this->rebuildClient = true;
$this->httpClient = $httpClient;
}

/**
* @return HttpClient
*/
public function getHttpClient()
{
return $this->httpClient;
}

/**
* Make sure to move the cache plugin to the end of the chain
*/
private function pushBackCachePlugin()
{
$cachePlugin = null;
foreach ($this->plugins as $i => $plugin) {
if ($plugin instanceof Plugin\CachePlugin) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth adding an interface plugins can implement to say they need to get pushed back instead of hard coding the cache plugin like this?

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure I like the idea. Plugins should not be affected by external configuration logic. Also, there is no guarantee that cache should always be "pushed back".

Copy link
Member

Choose a reason for hiding this comment

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

I would rather use append/prependPlugin methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

if we find a way to make replace preserve the order, we would not need this.

$cachePlugin = $plugin;
unset($this->plugins[$i]);
Copy link
Contributor

Choose a reason for hiding this comment

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

this could break in some edge cases when people have more than one cache plugin, maybe extending the base class to cache some requests differently.

if somebody would do the same with RequestMatcherPlugin, the cache would not be moved to the end at all.


$this->plugins[] = $cachePlugin;

return;
}
}
}
}
70 changes: 70 additions & 0 deletions src/PluginClientBuilderInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
<?php

namespace Http\Client\Common;

use Http\Client\HttpClient;

/**
* A builder that help you build a PluginClient.
*
* @author Tobias Nyholm <tobias.nyholm@gmail.com>
*/
interface PluginClientBuilderInterface
{
/**
* Add a new plugin to the end of the plugin chain.
*
* @param Plugin $plugin
*/
public function addPlugin(Plugin $plugin);

/**
* Remove a plugin by its fully qualified class name (FQCN).
*
* @param string $fqcn
*/
public function removePlugin($fqcn);

/**
* Alias for removePlugin and addPlugin.
*
* @param Plugin $plugin
*/
public function replacePlugin(Plugin $plugin);

/**
* Get all plugins.
*
* @return Plugin[]
*/
public function getPlugins();

/**
* This will overwrite all existing plugins.
*
* @param Plugin[] $plugins
*
* @return PluginClientBuilder
*/
public function setPlugins($plugins);

/**
* @return PluginClient
*/
public function buildClient();

/**
* @param HttpClient $httpClient
*/
public function setHttpClient(HttpClient $httpClient);

/**
* @return HttpClient
*/
public function getHttpClient();

/**
* @return bool
*/
public function isModified();
}