-
Notifications
You must be signed in to change notification settings - Fork 53
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) | ||
{ | ||
$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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would rather use append/prependPlugin methods. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} | ||
} | ||
} |
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(); | ||
} |
There was a problem hiding this comment.
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.