Skip to content

Add getter/setter to the plugin client #43

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

Closed
wants to merge 5 commits into from
Closed

Add getter/setter to the plugin client #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ad39792
Add getter/setter to the plugin client
Baachi Jan 13, 2016
16c25b5
Use a mock object instead of the LoopPlugin
Baachi Jan 13, 2016
9558ed7
Add validation for PluginClient::setPlugins method
Baachi Jan 13, 2016
a70d550
Provide better docblock for PluginClient::addPlugin
Baachi Jan 13, 2016
5208c06
Fix tests
Baachi Jan 13, 2016
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
15 changes: 15 additions & 0 deletions spec/PluginClientSpec.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,19 @@ function it_throws_loop_exception(HttpClient $client, RequestInterface $request)

$this->shouldThrow('Http\Client\Plugin\Exception\LoopException')->duringSendRequest($request);
}

function it_adds_a_plugin(Plugin $plugin)
Copy link
Author

Choose a reason for hiding this comment

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

Not sure if it is the right way, i never worked with phpspec.

Copy link
Member

Choose a reason for hiding this comment

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

This is fine.

{
$this->addPlugin($plugin);

$this->getPlugins()->shouldReturn([$plugin]);
}

function it_sets_new_plugins(Plugin $plugin)
{
$plugins = [$plugin];
$this->setPlugins($plugins);

$this->getPlugins()->shouldReturn($plugins);
}
}
36 changes: 34 additions & 2 deletions src/PluginClient.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ final class PluginClient implements HttpClient, HttpAsyncClient
*
* @var Plugin[]
*/
private $plugins;
private $plugins = [];

/**
* A list of options.
Expand All @@ -57,10 +57,42 @@ public function __construct($client, array $plugins = [], array $options = [])
throw new \RuntimeException('Client must be an instance of Http\\Client\\HttpClient or Http\\Client\\HttpAsyncClient');
}

$this->plugins = $plugins;
$this->setPlugins($plugins);
$this->options = $this->configure($options);
}

/**
* Append a plugin to the end of the queue.
*
* @param Plugin $plugin
*/
public function addPlugin(Plugin $plugin)
Copy link
Member

Choose a reason for hiding this comment

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

How about renaming to "appendPlugin"? Because you are appending items to the end of a queue. It would be more clear that the order of the plugins is important.

Copy link
Author

Choose a reason for hiding this comment

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

I come from the symfony world and they use always add or addXXX but appendPlugin sounds reasonable.
@sagikazarmark What do you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

I don't really think it matters, both sounds OK. The reason why I usually prefer add is that it fits into the three character long actions: add, get, set, has

Copy link
Member

Choose a reason for hiding this comment

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

Sure, use addPlugin. But make sure to write a comment that the plugin added is appended to the end of the plugin queue

Copy link
Member

Choose a reason for hiding this comment

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

👍 Agree, this behavior is crucial to be mentioned as the order of plugins matter. Probably this is a reason against allowing to add plugins runtime: it can cause confusions.

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 decide to want this functionality, i would call this appendPlugin, i find that more precise than add. we might also want prependPlugin at some point...

Copy link
Member

Choose a reason for hiding this comment

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

With prepend added it would indeed make sense.

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 be ok with addPlugin in normal.

However, like described in our docs, order of plugins matters, so big 👍 for the appendPlugin / preprendPlugin method name.

{
$this->plugins[] = $plugin;
}

/**
* Get all active plugins.
*
* @return Plugin[]
*/
public function getPlugins()
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 the use case of this? checking if a specific plugin is already in the chain?

Copy link
Author

Choose a reason for hiding this comment

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

Yep this was the idea behind it

{
return $this->plugins;
}

/**
* Assign plugins to the client.
*
* @param Plugin[] $plugins
*/
public function setPlugins(array $plugins = [])
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be appendPlugins, setPlugins makes me expect that this would replace the existing plugins (which would be a bad idea)

{
foreach ($plugins as $plugin) {
$this->addPlugin($plugin);
}
}

/**
* {@inheritdoc}
*/
Expand Down