This tool is currently proof-of-concept. Your feedback and evaluation is valuable in helping to improve it and ensure its reports are meaninful.
Please click here to complete a short survey to tell us what you think. It should take less than 5 minutes and help further this research project!
GuzzleHttp\Handler\CurlMultiHandler
Detected issues
Issue | Method | Line number |
---|---|---|
Using `new` in constructor | __construct | 69 |
Code
Click highlighted lines for details
<?php
namespace GuzzleHttp\Handler;
use GuzzleHttp\Promise as P;
use GuzzleHttp\Promise\Promise;
use GuzzleHttp\Promise\PromiseInterface;
use GuzzleHttp\Utils;
use Psr\Http\Message\RequestInterface;
/**
* Returns an asynchronous response using curl_multi_* functions.
*
* When using the CurlMultiHandler, custom curl options can be specified as an
* associative array of curl option constants mapping to values in the
* **curl** key of the provided request options.
*
* @property resource|\CurlMultiHandle $_mh Internal use only. Lazy loaded multi-handle.
*
* @final
*/
class CurlMultiHandler
{
/**
* @var CurlFactoryInterface
*/
private $factory;
/**
* @var int
*/
private $selectTimeout;
/**
* @var resource|\CurlMultiHandle|null the currently executing resource in `curl_multi_exec`.
*/
private $active;
/**
* @var array Request entry handles, indexed by handle id in `addRequest`.
*
* @see CurlMultiHandler::addRequest
*/
private $handles = [];
/**
* @var array<int, float> An array of delay times, indexed by handle id in `addRequest`.
*
* @see CurlMultiHandler::addRequest
*/
private $delays = [];
/**
* @var array<mixed> An associative array of CURLMOPT_* options and corresponding values for curl_multi_setopt()
*/
private $options = [];
/**
* This handler accepts the following options:
*
* - handle_factory: An optional factory used to create curl handles
* - select_timeout: Optional timeout (in seconds) to block before timing
* out while selecting curl handles. Defaults to 1 second.
* - options: An associative array of CURLMOPT_* options and
* corresponding values for curl_multi_setopt()
*/
public function __construct(array $options = [])
{
Usage of the new keyword in a constructor
Summary of issues
- Tight Coupling
- Hidden dependencies
- Broken Encapsulatioin
Background
If a dependency is constructed inside the object that requires it rather than passed in as a reference then flexibility is lost[1][2]
public class Car {
private Engine engine;
public Car() {
this.engine = new Engine();
}
}Here, the
Car
constructor creates theEngine
instance. This is inflexible as it forces allCar
objects to use the exact sameEngine
type. Instead, it would encourage reuse if the program supported different engine types (e.g.DieselEngine
,PetrolEngine
orHybridEngine
).The same is true when an instance variable is created when the class is defined:
public class Car {
private Engine engine = new Engine();
}By using the
new
keyword to instantiate a dependency, the specific implementation of that dependency is hardcoded and cannot be substituted.Instead, the dependency should be constructed outside the class and injected in:
public class Car {
private Engine engine;
public Car(Engine engine) {
this.engine = engine;
}
}Using dependency injection it is possible to pass in any engine type:
//Instead of
Car myCar = new Car();
//It's now possible to construct different types of car:
Car petrolCar = new Car(new PetrolEngine);
Car electricCar = new Car(new ElectricEngine);A secondary advantage to Dependency Injection with regards to flexibility and encapsulation is that the class which has the dependency (
Car
, in this example) it not aware of the dependencies of theEngine
class.For example, if the
Engine
class required aGearbox
instance as a constructor argument, theCar
class would need to instantiate and pass in the relevantGearbox
instance. And provide any dependencies of theGearbox
class when instantiating it.If the constructor arguments of any of the classes which need to be instantiated are modified during development, any class which creates an instance of the class must also be modified. A change to the constructor for
Engine
would require modifying theCar
class. Instead, if the fully constructedEngine
instanceBy loosely coupling the
Engine
class to theCar
class, the author of theCar
class does not need to know anything about the implementation ofEngine
class or have knowledge of what dependencies it has.
public Car() {
this.engine = new Engine(new Gearbox());
}Further reading
Additional resources:
References
- Hevery, M. (2008) How to Think About the “new” Operator with Respect to Unit Testing [online]. Available from: http://misko.hevery.com/2008/07/08/how-to-think-about-the-new-operator/
- Hevery, M. (2008) Code Reviewers Guide [online]. Available from: http://misko.hevery.com/code-reviewers-guide/
Please note: This feature is currently proof-of-concept, the instructions may not always be completely accurate.
\GuzzleHttp\Handler\CurlFactory
is instantiated inside the constructor ofGuzzleHttp\Handler\CurlMultiHandler
1) Remove the new expression and replace it with a variable:
$this->factory = $options['handle_factory'] ?? new CurlFactory(50);
becomes:
$this->factory = $options['handle_factory'] ?? $curlFactory;
2) Add a constructor argument for the new variable: Replace:
public function __construct(array $options = [])
with:
public function __construct(\GuzzleHttp\Handler\CurlFactory $curlFactory, array $options = [])
3) Find any occurance of
new GuzzleHttp\Handler\CurlMultiHandler
and provide the new dependency.
\GuzzleHttp\Handler\CurlMultiHandler
is being instantiated in src/Utils.php:0Replace:
$handler = Proxy::wrapSync(new CurlMultiHandler(), new CurlHandler(new \GuzzleHttp\Handler\CurlFactory(3)));
With:
$handler = Proxy::wrapSync(new CurlMultiHandler(new \GuzzleHttp\Handler\CurlFactory(50)), new CurlHandler(new \GuzzleHttp\Handler\CurlFactory(3)));
\GuzzleHttp\Handler\CurlMultiHandler
is being instantiated in src/Utils.php:0Replace:
$handler = new CurlMultiHandler();
With:
$handler = new CurlMultiHandler(new \GuzzleHttp\Handler\CurlFactory(50));
Please note: This feature is currently proof-of-concept, this patch may not work, please don't blindly apply it.
diff --git a/src/Client.php b/src/Client.php index 81f4f6d..8c77d1e 100644 --- a/src/Client.php +++ b/src/Client.php @@ -257,7 +257,7 @@ class Client implements ClientInterface, \Psr\Http\Client\ClientInterface $this->config = $config + $defaults; if (!empty($config['cookies']) && $config['cookies'] === true) { - $this->config['cookies'] = new CookieJar(); + $this->config['cookies'] = new CookieJar(new \GuzzleHttp\Cookie\SetCookie(new SetCookie())); } // Add the default user-agent header. diff --git a/src/Cookie/CookieJar.php b/src/Cookie/CookieJar.php index d6757c6..a6cfe90 100644 --- a/src/Cookie/CookieJar.php +++ b/src/Cookie/CookieJar.php @@ -27,13 +27,13 @@ class CookieJar implements CookieJarInterface * arrays that can be used with the SetCookie * constructor */ - public function __construct(bool $strictMode = false, array $cookieArray = []) + public function __construct(\GuzzleHttp\Cookie\SetCookie $setCookie, bool $strictMode = false, array $cookieArray = []) { $this->strictMode = $strictMode; foreach ($cookieArray as $cookie) { if (!($cookie instanceof SetCookie)) { - $cookie = new SetCookie($cookie); + $cookie = $setCookie; } $this->setCookie($cookie); } @@ -310,4 +310,4 @@ class CookieJar implements CookieJarInterface ); } } -} +} \ No newline at end of file diff --git a/src/Handler/CurlHandler.php b/src/Handler/CurlHandler.php index 47e21f0..8d26b67 100644 --- a/src/Handler/CurlHandler.php +++ b/src/Handler/CurlHandler.php @@ -28,10 +28,10 @@ class CurlHandler * * @param array $options Array of options to use with the handler */ - public function __construct(array $options = []) + public function __construct(\GuzzleHttp\Handler\CurlFactory $curlFactory, array $options = []) { $this->factory = $options['handle_factory'] - ?? new CurlFactory(3); + ?? $curlFactory; } public function __invoke(RequestInterface $request, array $options): PromiseInterface @@ -46,4 +46,4 @@ class CurlHandler return CurlFactory::finish($this, $easy, $this->factory); } -} +} \ No newline at end of file diff --git a/src/Handler/CurlMultiHandler.php b/src/Handler/CurlMultiHandler.php index ace0d84..7657ecf 100644 --- a/src/Handler/CurlMultiHandler.php +++ b/src/Handler/CurlMultiHandler.php @@ -64,9 +64,9 @@ class CurlMultiHandler * - options: An associative array of CURLMOPT_* options and * corresponding values for curl_multi_setopt() */ - public function __construct(array $options = []) + public function __construct(\GuzzleHttp\Handler\CurlFactory $curlFactory, array $options = []) { - $this->factory = $options['handle_factory'] ?? new CurlFactory(50); + $this->factory = $options['handle_factory'] ?? $curlFactory; if (isset($options['select_timeout'])) { $this->selectTimeout = $options['select_timeout']; @@ -254,4 +254,4 @@ class CurlMultiHandler return ((int) \max(0, $nextTime - $currentTime)) * 1000000; } -} +} \ No newline at end of file diff --git a/src/Utils.php b/src/Utils.php index 91591da..ba5ee8b 100644 --- a/src/Utils.php +++ b/src/Utils.php @@ -87,11 +87,11 @@ final class Utils { $handler = null; if (\function_exists('curl_multi_exec') && \function_exists('curl_exec')) { - $handler = Proxy::wrapSync(new CurlMultiHandler(), new CurlHandler()); + $handler = Proxy::wrapSync(new CurlMultiHandler(new \GuzzleHttp\Handler\CurlFactory(50)), new CurlHandler(new \GuzzleHttp\Handler\CurlFactory(3))); } elseif (\function_exists('curl_exec')) { - $handler = new CurlHandler(); + $handler = new CurlHandler(new \GuzzleHttp\Handler\CurlFactory(3)); } elseif (\function_exists('curl_multi_exec')) { - $handler = new CurlMultiHandler(); + $handler = new CurlMultiHandler(new \GuzzleHttp\Handler\CurlFactory(50)); } if (\ini_get('allow_url_fopen')) {if (isset($options['select_timeout'])) {
$this->selectTimeout = $options['select_timeout'];
} elseif ($selectTimeout = Utils::getenv('GUZZLE_CURL_SELECT_TIMEOUT')) {
@trigger_error('Since guzzlehttp/guzzle 7.2.0: Using environment variable GUZZLE_CURL_SELECT_TIMEOUT is deprecated. Use option "select_timeout" instead.', \E_USER_DEPRECATED);
$this->selectTimeout = (int) $selectTimeout;
} else {
$this->selectTimeout = 1;
}
$this->options = $options['options'] ?? [];
}
/**
* @param string $name
*
* @return resource|\CurlMultiHandle
*
* @throws \BadMethodCallException when another field as `_mh` will be gotten
* @throws \RuntimeException when curl can not initialize a multi handle
*/
public function __get($name)
{
if ($name !== '_mh') {
throw new \BadMethodCallException("Can not get other property as '_mh'.");
}
$multiHandle = \curl_multi_init();
if (false === $multiHandle) {
throw new \RuntimeException('Can not initialize curl multi handle.');
}
$this->_mh = $multiHandle;
foreach ($this->options as $option => $value) {
// A warning is raised in case of a wrong option.
curl_multi_setopt($this->_mh, $option, $value);
}
return $this->_mh;
}
public function __destruct()
{
if (isset($this->_mh)) {
\curl_multi_close($this->_mh);
unset($this->_mh);
}
}
public function __invoke(RequestInterface $request, array $options): PromiseInterface
{
$easy = $this->factory->create($request, $options);
$id = (int) $easy->handle;
$promise = new Promise(
[$this, 'execute'],
function () use ($id) {
return $this->cancel($id);
}
);
$this->addRequest(['easy' => $easy, 'deferred' => $promise]);
return $promise;
}
/**
* Ticks the curl event loop.
*/
public function tick(): void
{
// Add any delayed handles if needed.
if ($this->delays) {
$currentTime = Utils::currentTime();
foreach ($this->delays as $id => $delay) {
if ($currentTime >= $delay) {
unset($this->delays[$id]);
\curl_multi_add_handle(
$this->_mh,
$this->handles[$id]['easy']->handle
);
}
}
}
// Step through the task queue which may add additional requests.
P\Utils::queue()->run();
if ($this->active && \curl_multi_select($this->_mh, $this->selectTimeout) === -1) {
// Perform a usleep if a select returns -1.
// See: https://bugs.php.net/bug.php?id=61141
\usleep(250);
}
while (\curl_multi_exec($this->_mh, $this->active) === \CURLM_CALL_MULTI_PERFORM);
$this->processMessages();
}
/**
* Runs until all outstanding connections have completed.
*/
public function execute(): void
{
$queue = P\Utils::queue();
while ($this->handles || !$queue->isEmpty()) {
// If there are no transfers, then sleep for the next delay
if (!$this->active && $this->delays) {
\usleep($this->timeToNext());
}
$this->tick();
}
}
private function addRequest(array $entry): void
{
$easy = $entry['easy'];
$id = (int) $easy->handle;
$this->handles[$id] = $entry;
if (empty($easy->options['delay'])) {
\curl_multi_add_handle($this->_mh, $easy->handle);
} else {
$this->delays[$id] = Utils::currentTime() + ($easy->options['delay'] / 1000);
}
}
/**
* Cancels a handle from sending and removes references to it.
*
* @param int $id Handle ID to cancel and remove.
*
* @return bool True on success, false on failure.
*/
private function cancel($id): bool
{
if (!is_int($id)) {
trigger_deprecation('guzzlehttp/guzzle', '7.4', 'Not passing an integer to %s::%s() is deprecated and will cause an error in 8.0.', __CLASS__, __FUNCTION__);
}
// Cannot cancel if it has been processed.
if (!isset($this->handles[$id])) {
return false;
}
$handle = $this->handles[$id]['easy']->handle;
unset($this->delays[$id], $this->handles[$id]);
\curl_multi_remove_handle($this->_mh, $handle);
\curl_close($handle);
return true;
}
private function processMessages(): void
{
while ($done = \curl_multi_info_read($this->_mh)) {
$id = (int) $done['handle'];
\curl_multi_remove_handle($this->_mh, $done['handle']);
if (!isset($this->handles[$id])) {
// Probably was cancelled.
continue;
}
$entry = $this->handles[$id];
unset($this->handles[$id], $this->delays[$id]);
$entry['easy']->errno = $done['result'];
$entry['deferred']->resolve(
CurlFactory::finish($this, $entry['easy'], $this->factory)
);
}
}
private function timeToNext(): int
{
$currentTime = Utils::currentTime();
$nextTime = \PHP_INT_MAX;
foreach ($this->delays as $time) {
if ($time < $nextTime) {
$nextTime = $time;
}
}
return ((int) \max(0, $nextTime - $currentTime)) * 1000000;
}
}