Insphpect

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\HandlerStack

Detected issues

Issue Method Line number
Use of static methods self 47
Use of static methods stack 191

Code

Click highlighted lines for details

<?phpnamespace GuzzleHttp;use GuzzleHttp\Promise\PromiseInterface;use Psr\Http\Message\RequestInterface;use Psr\Http\Message\ResponseInterface;/** * Creates a composed Guzzle handler function by stacking middlewares on top of * an HTTP handler function. * * @final */class HandlerStack{    /**     * @var null|callable(RequestInterface, array): PromiseInterface     */    private $handler;    /**     * @var array{(callable(callable(RequestInterface, array): PromiseInterface): callable), (string|null)}[]     */    private $stack = [];    /**     * @var null|callable(RequestInterface, array): PromiseInterface     */    private $cached;    /**     * Creates a default handler stack that can be used by clients.     *     * The returned handler will wrap the provided handler or use the most     * appropriate default handler for your system. The returned HandlerStack has     * support for cookies, redirects, HTTP error exceptions, and preparing a body     * before sending.     *     * The returned handler stack can be passed to a client in the "handler"     * option.     *     * @param null|callable(RequestInterface, array): PromiseInterface $handler HTTP handler function to use with the stack. If no     *                                                                          handler is provided, the best handler for your     *                                                                          system will be utilized.     */

Static methods

Summary of issues

  • Tight Coupling
  • Hidden dependencies
  • Global state (if also using static variables)

Tight Coupling

Use of static methods always reduces flexibility by introducing tight coupling[1]. A static method tightly couples the calling code to the specific class the method exists in.

 
function totalAbs(double valuedouble value2) {
    return 
abs(value) + abs(value2);
}

Here, the method totalAbs has a dependency on the Math class and the .abs() method will always be called. Although for testing purposes this may not be a problem, the coupling reduces flexibility because the total method can only work with doubles/integers, as that's all the Math.abs() function can use. Although type coercion will allow the use of any primitive numeric type, these types have limitations. It's impossible to use another class such as BigInteger or a class for dealing with greater precision decimals or even alternative numbering systems such as Roman numerals.

The totalAbs function takes two doubles and converts them to their absolute values before adding them. This is inflexible because it only works with doubles. It's tied to doubles because that's what the Math.abs() static method requires. If, instead, using OOP an interface was created to handle any number that had this method:

 interface Numeric {
    public function 
abs(): Numeric;
}

It would then be possible to rewrite the totalAbs method to work with any kind of number:

 function totalAbs(Numeric valueNumeric value): Numeric {
    return 
value.abs() + value2.abs();
}

By removing the static method and using an instance method in its place the totalAbs method is now agnostic about the type of number it is dealing with. It could be called with any of the following (assuming they implement the Numeric interface)

 
totalAbs(new Integer(4), new Integer(-53));

totalAbs(new Double(34.4), new Integer(-2));

totalAbs(new BigInteger('123445454564765739878989343225778'), new Integer(2343));

totalAbs(new RomanNumeral('VII'), new RomanNumeral('CXV'));

Making the method reusable in a way that it wasn't when static methods were being used. By changing the static methods to instance methods, flexibility has been enhanced as the method can be used with any numeric type, not just numeric types that are supported by the Math.abs() method.

Broken encapsulation

Static methods also break encapsulation. Encapsulation is defined by Rogers[2] as:

the bundling of data with the methods that operate on that data

By passing the numeric value into the abs method, the data being operated on is being separated from the methods that operate on it, breaking encapsulation. Instead using num.abs() the data is encapsulated in the num instance and its type is not visible or relevant to the outside world. abs() will work on the data and work regardless of num's type, providing it implements the abs method.

This is a simple example, but applies to all static methods. Use of polymorphic instance methods that work on encapsulated data will always be more flexible than static method calls which can only ever deal with specific pre-defined types.

Further reading

Exceptions

The only exception to this rule is when a static method is used for object creation in place of the new keyword[3]. This is because the new keyword is already a static call. However, even here a non-static factory is often preferable for testing purposes[4][5].

References

  1. Popov, N. (2014) Don't be STUPID: GRASP SOLID! [online]. Available from: https://nikic.github.io/2011/12/27/Dont-be-STUPID-GRASP-SOLID.html
  2. Rogers, P. (2001) Encapsulation is not information hiding [online]. Available from: http://www.javaworld.com/article/2075271/core-java/encapsulation-is-not-information-hiding.html
  3. Sonmez, J. (2010) Static Methods Will Shock You [online]. Available from: http://simpleprogrammer.com/2010/01/29/static-methods-will-shock-you/
  4. Hevery, M. (2008) Static Methods are Death to Testability [online]. Available from: http://misko.hevery.com/2008/12/15/static-methods-are-death-to-testability/
  5. Butler, T. (2013) Are Static Methods/Variables bad practice? [online]. Available from: https://r.je/static-methods-bad-practice.html
{ $stack = new self($handler ?: Utils::chooseHandler()); $stack->push(Middleware::httpErrors(), 'http_errors'); $stack->push(Middleware::redirect(), 'allow_redirects'); $stack->push(Middleware::cookies(), 'cookies'); $stack->push(Middleware::prepareBody(), 'prepare_body'); return $stack; } /** * @param null|callable(RequestInterface, array): PromiseInterface $handler Underlying HTTP handler. */ public function __construct(callable $handler = null) { $this->handler = $handler; } /** * Invokes the handler stack as a composed handler * * @return ResponseInterface|PromiseInterface */ public function __invoke(RequestInterface $request, array $options) { $handler = $this->resolve(); return $handler($request, $options); } /** * Dumps a string representation of the stack. * * @return string */ public function __toString() { $depth = 0; $stack = []; if ($this->handler !== null) { $stack[] = "0) Handler: " . $this->debugCallable($this->handler); } $result = ''; foreach (\array_reverse($this->stack) as $tuple) { $depth++; $str = "{$depth}) Name: '{$tuple[1]}', "; $str .= "Function: " . $this->debugCallable($tuple[0]); $result = "> {$str}\n{$result}"; $stack[] = $str; } foreach (\array_keys($stack) as $k) { $result .= "< {$stack[$k]}\n"; } return $result; } /** * Set the HTTP handler that actually returns a promise. * * @param callable(RequestInterface, array): PromiseInterface $handler Accepts a request and array of options and * returns a Promise. */ public function setHandler(callable $handler): void { $this->handler = $handler; $this->cached = null; } /** * Returns true if the builder has a handler. */ public function hasHandler(): bool { return $this->handler !== null ; } /** * Unshift a middleware to the bottom of the stack. * * @param callable(callable): callable $middleware Middleware function * @param string $name Name to register for this middleware. */ public function unshift(callable $middleware, ?string $name = null): void { \array_unshift($this->stack, [$middleware, $name]); $this->cached = null; } /** * Push a middleware to the top of the stack. * * @param callable(callable): callable $middleware Middleware function * @param string $name Name to register for this middleware. */ public function push(callable $middleware, string $name = ''): void { $this->stack[] = [$middleware, $name]; $this->cached = null; } /** * Add a middleware before another middleware by name. * * @param string $findName Middleware to find * @param callable(callable): callable $middleware Middleware function * @param string $withName Name to register for this middleware. */ public function before(string $findName, callable $middleware, string $withName = ''): void { $this->splice($findName, $withName, $middleware, true); } /** * Add a middleware after another middleware by name. * * @param string $findName Middleware to find * @param callable(callable): callable $middleware Middleware function * @param string $withName Name to register for this middleware. */ public function after(string $findName, callable $middleware, string $withName = ''): void { $this->splice($findName, $withName, $middleware, false); } /** * Remove a middleware by instance or name from the stack. * * @param callable|string $remove Middleware to remove by instance or name. */ public function remove($remove): void { if (!is_string($remove) && !is_callable($remove)) { trigger_deprecation('guzzlehttp/guzzle', '7.4', 'Not passing a callable or string to %s::%s() is deprecated and will cause an error in 8.0.', __CLASS__, __FUNCTION__); } $this->cached = null; $idx = \is_callable($remove) ? 0 : 1; $this->stack = \array_values(\array_filter( $this->stack,

Static methods

Summary of issues

  • Tight Coupling
  • Hidden dependencies
  • Global state (if also using static variables)

Tight Coupling

Use of static methods always reduces flexibility by introducing tight coupling[1]. A static method tightly couples the calling code to the specific class the method exists in.

 
function totalAbs(double valuedouble value2) {
    return 
abs(value) + abs(value2);
}

Here, the method totalAbs has a dependency on the Math class and the .abs() method will always be called. Although for testing purposes this may not be a problem, the coupling reduces flexibility because the total method can only work with doubles/integers, as that's all the Math.abs() function can use. Although type coercion will allow the use of any primitive numeric type, these types have limitations. It's impossible to use another class such as BigInteger or a class for dealing with greater precision decimals or even alternative numbering systems such as Roman numerals.

The totalAbs function takes two doubles and converts them to their absolute values before adding them. This is inflexible because it only works with doubles. It's tied to doubles because that's what the Math.abs() static method requires. If, instead, using OOP an interface was created to handle any number that had this method:

 interface Numeric {
    public function 
abs(): Numeric;
}

It would then be possible to rewrite the totalAbs method to work with any kind of number:

 function totalAbs(Numeric valueNumeric value): Numeric {
    return 
value.abs() + value2.abs();
}

By removing the static method and using an instance method in its place the totalAbs method is now agnostic about the type of number it is dealing with. It could be called with any of the following (assuming they implement the Numeric interface)

 
totalAbs(new Integer(4), new Integer(-53));

totalAbs(new Double(34.4), new Integer(-2));

totalAbs(new BigInteger('123445454564765739878989343225778'), new Integer(2343));

totalAbs(new RomanNumeral('VII'), new RomanNumeral('CXV'));

Making the method reusable in a way that it wasn't when static methods were being used. By changing the static methods to instance methods, flexibility has been enhanced as the method can be used with any numeric type, not just numeric types that are supported by the Math.abs() method.

Broken encapsulation

Static methods also break encapsulation. Encapsulation is defined by Rogers[2] as:

the bundling of data with the methods that operate on that data

By passing the numeric value into the abs method, the data being operated on is being separated from the methods that operate on it, breaking encapsulation. Instead using num.abs() the data is encapsulated in the num instance and its type is not visible or relevant to the outside world. abs() will work on the data and work regardless of num's type, providing it implements the abs method.

This is a simple example, but applies to all static methods. Use of polymorphic instance methods that work on encapsulated data will always be more flexible than static method calls which can only ever deal with specific pre-defined types.

Further reading

Exceptions

The only exception to this rule is when a static method is used for object creation in place of the new keyword[3]. This is because the new keyword is already a static call. However, even here a non-static factory is often preferable for testing purposes[4][5].

References

  1. Popov, N. (2014) Don't be STUPID: GRASP SOLID! [online]. Available from: https://nikic.github.io/2011/12/27/Dont-be-STUPID-GRASP-SOLID.html
  2. Rogers, P. (2001) Encapsulation is not information hiding [online]. Available from: http://www.javaworld.com/article/2075271/core-java/encapsulation-is-not-information-hiding.html
  3. Sonmez, J. (2010) Static Methods Will Shock You [online]. Available from: http://simpleprogrammer.com/2010/01/29/static-methods-will-shock-you/
  4. Hevery, M. (2008) Static Methods are Death to Testability [online]. Available from: http://misko.hevery.com/2008/12/15/static-methods-are-death-to-testability/
  5. Butler, T. (2013) Are Static Methods/Variables bad practice? [online]. Available from: https://r.je/static-methods-bad-practice.html
return $tuple[$idx] !== $remove; } )); } /** * Compose the middleware and handler into a single callable function. * * @return callable(RequestInterface, array): PromiseInterface */ public function resolve(): callable { if ($this->cached === null) { if (($prev = $this->handler) === null) { throw new \LogicException('No handler has been specified'); } foreach (\array_reverse($this->stack) as $fn) { /** @var callable(RequestInterface, array): PromiseInterface $prev */ $prev = $fn[0]($prev); } $this->cached = $prev; } return $this->cached; } private function findByName(string $name): int { foreach ($this->stack as $k => $v) { if ($v[1] === $name) { return $k; } } throw new \InvalidArgumentException("Middleware not found: $name"); } /** * Splices a function into the middleware list at a specific position. */ private function splice(string $findName, string $withName, callable $middleware, bool $before): void { $this->cached = null; $idx = $this->findByName($findName); $tuple = [$middleware, $withName]; if ($before) { if ($idx === 0) { \array_unshift($this->stack, $tuple); } else { $replacement = [$tuple, $this->stack[$idx]]; \array_splice($this->stack, $idx, 1, $replacement); } } elseif ($idx === \count($this->stack) - 1) { $this->stack[] = $tuple; } else { $replacement = [$this->stack[$idx], $tuple]; \array_splice($this->stack, $idx, 1, $replacement); } } /** * Provides a debug string for a given callable. * * @param callable|string $fn Function to write as a string. */ private function debugCallable($fn): string { if (\is_string($fn)) { return "callable({$fn})"; } if (\is_array($fn)) { return \is_string($fn[0]) ? "callable({$fn[0]}::{$fn[1]})" : "callable(['" . \get_class($fn[0]) . "', '{$fn[1]}'])"; } /** @var object $fn */ return 'callable(' . \spl_object_hash($fn) . ')'; }}