-
Notifications
You must be signed in to change notification settings - Fork 50
Discovery bugfixes and more flexible. #87
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 6 commits
d49f7d6
c1f0982
5c015a2
c8359e7
5509df4
a665340
27b6238
5c1a5b9
cb37a48
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 |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
use Http\Client\Common\HttpMethodsClient; | ||
use Http\Client\Common\Plugin\AuthenticationPlugin; | ||
use Http\Client\Common\PluginClient; | ||
use Http\Discovery\HttpAsyncClientDiscovery; | ||
use Http\HttplugBundle\ClientFactory\DummyClient; | ||
use Http\HttplugBundle\Collector\DebugPlugin; | ||
use Http\Message\Authentication\BasicAuth; | ||
|
@@ -65,6 +66,7 @@ public function load(array $configs, ContainerBuilder $container) | |
|
||
$this->configurePlugins($container, $config['plugins']); | ||
$this->configureClients($container, $config); | ||
$this->configureAutoDiscoveryClients($container, $config); | ||
} | ||
|
||
/** | ||
|
@@ -278,4 +280,53 @@ private function registerDebugPlugin(ContainerBuilder $container, $name) | |
|
||
return $serviceIdDebugPlugin; | ||
} | ||
|
||
/** | ||
* Make sure we inject the debug plugin for clients found by auto discovery. | ||
* | ||
* @param ContainerBuilder $container | ||
* @param array $config | ||
*/ | ||
private function configureAutoDiscoveryClients(ContainerBuilder $container, array $config) | ||
{ | ||
$httpClient = $config['discovery']['client']; | ||
if ($httpClient === 'auto') { | ||
$httpClient = $this->registerAutoDiscoverableClientWithDebugPlugin($container, 'client'); | ||
} elseif ($httpClient !== null) { | ||
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. in this case i would do |
||
$httpClient = new Reference($httpClient); | ||
} | ||
|
||
$asyncHttpClient = $config['discovery']['async_client']; | ||
if ($asyncHttpClient === 'auto') { | ||
$asyncHttpClient = $this->registerAutoDiscoverableClientWithDebugPlugin($container, 'async_client'); | ||
} elseif ($asyncHttpClient !== null) { | ||
$asyncHttpClient = new Reference($httpClient); | ||
} | ||
|
||
$container->getDefinition('httplug.strategy') | ||
->addArgument($httpClient) | ||
->addArgument($asyncHttpClient); | ||
} | ||
|
||
/** | ||
* @param ContainerBuilder $container | ||
* @param $name | ||
* | ||
* @return Reference | ||
*/ | ||
private function registerAutoDiscoverableClientWithDebugPlugin(ContainerBuilder $container, $name) | ||
{ | ||
$definition = $container->register('httplug.auto_discovery_'.$name.'.pure', DummyClient::class); | ||
$definition->setPublic(false); | ||
$definition->setFactory([HttpAsyncClientDiscovery::class, 'find']); | ||
|
||
$serviceIdDebugPlugin = $this->registerDebugPlugin($container, 'auto_discovery_'.$name); | ||
$container->register('httplug.auto_discovery_'.$name.'.plugin', PluginClient::class) | ||
->setPublic(false) | ||
->addArgument(new Reference('httplug.auto_discovery_'.$name.'.pure')) | ||
->addArgument([]) | ||
->addArgument(['debug_plugins' => [new Reference($serviceIdDebugPlugin)]]); | ||
|
||
return new Reference('httplug.auto_discovery_'.$name.'.plugin'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
namespace Http\HttplugBundle\Discovery; | ||
|
||
use Http\Client\HttpClient; | ||
use Http\Client\HttpAsyncClient; | ||
use Http\Discovery\HttpClientDiscovery; | ||
use Http\Discovery\Strategy\DiscoveryStrategy; | ||
use Symfony\Component\Console\ConsoleEvents; | ||
|
@@ -24,24 +25,37 @@ class ConfiguredClientsStrategy implements DiscoveryStrategy, EventSubscriberInt | |
private static $client; | ||
|
||
/** | ||
* @param HttpClient $httpClient | ||
* @var HttpAsyncClient | ||
*/ | ||
public function __construct(HttpClient $httpClient) | ||
private static $asyncClient; | ||
|
||
/** | ||
* @param HttpClient $httpClient | ||
* @param HttpAsyncClient $asyncClient | ||
*/ | ||
public function __construct(HttpClient $httpClient = null, HttpAsyncClient $asyncClient = null) | ||
{ | ||
static::$client = $httpClient; | ||
static::$asyncClient = $asyncClient; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public static function getCandidates($type) | ||
{ | ||
if (static::$client !== null && $type == HttpClient::class) { | ||
if (static::$client !== null && $type === HttpClient::class) { | ||
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. Should the type check rather come first? This way the first condition is evaluated even if the type is a MessageFactory. |
||
return [['class' => function () { | ||
return static::$client; | ||
}]]; | ||
} | ||
|
||
if (static::$asyncClient !== null && $type === HttpAsyncClient::class) { | ||
return [['class' => function () { | ||
return static::$asyncClient; | ||
}]]; | ||
} | ||
|
||
return []; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,6 @@ | |
|
||
<services> | ||
<service id="httplug.strategy" class="Http\HttplugBundle\Discovery\ConfiguredClientsStrategy" public="true"> | ||
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. Is public necessary here? 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. Yes, all event subscribers have to be public. 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. But they are public by default, aren't they? 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. Ah, correct.
|
||
<argument type="service" id="httplug.client"/> | ||
<tag name="kernel.event_subscriber"/> | ||
</service> | ||
|
||
|
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 not auto as well?
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.
Because if no AsyncClient is found we get an exception. I do not think I can catch that.
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.
okay. but then lets change the info to mention that you should set this to auto if you know that you have an async client available.
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.
Or should I put that in the docs?
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.
I added it in the docs:
https://github.com/php-http/documentation/pull/118/files#diff-33b9b1161d0ce83931e73c48bf9e6900R71