Question on coupling of static classes

[gregor-j]

I recently stumbled across a problem applying the Dependency Inversion Principle (DIP) trying to write classes for communication with either local (fopen()) or remote (fsockopen()) serial ports. (In the following example I'll stick to the Tcp class using fsockopen() and leave out the interfaces.)

<?php
namespace Gregor\ExampleSrc;

final class Socket implements SocketFunctionsInterface
{
    public static function fsockopen($hostname, $port, &$errno, &$errstr, $timeout): ?SocketFunctionsInterface
    {
        $socket = fsockopen($hostname, $port, $errno, $errstr, $timeout);
        if (is_resource($socket)) {
            return new self($socket);
        }
        return null;
    }

    /**
     * @var resource
     */
    private $socket;

    public function __construct($socket)
    {
        $this->socket = $socket;
    }

    public function __destruct()
    {
        fclose($this->socket);
        $this->socket = null;
    }

    public function fwrite($string, $length)
    {
        return fwrite($this->socket, $string, $length);
    }

    public function fgetc()
    {
        return fgetc($this->socket);
    }
}

<?php
namespace Gregor\ExampleSrc;

final class Tcp implements CommunicationInterface
{
    /**
     * @var SocketFunctionsInterface
     */
    private $socket;

    /**
     * @var SocketFunctionsInterface
     */
    private static $socketClass = Socket::class;

    public function __construct(string $host, int $port)
    {
        //... code preparing the class, that needs to be tested
        $socketClass = self::$socketClass;
        while (($this->socket = $socketClass::fsockopen($host, $port, $errNo, $errMessage, $timeout)) === null) {
            //... lots of code, that needs to be tested
        }
    }

    public static function setSocketFunctionsClass(string $className): void
    {
        $class = new ReflectionClass($className);
        if (!$class->implementsInterface(SocketFunctionsInterface::class)) {
            throw new InvalidConfigException(sprintf(
                'The given class %s doesn\'t implement %s!',
                $className,
                SocketFunctionsInterface::class
            ));
        }
        self::$socketClass = $className;
    }

    //... code using $this->socket->... methods, that needs to be tested
}

In order to test the code of the Tcp class, I moved fsockopen() fgetc() and frwite() into a separate class Socket. Socket mustn't contain any code, that would need testing. However, in order to create an instance of that Socket class, fsockopen() needs to be called and that can't be done outside of the Tcp class.

My concern is: The case of injecting a static class before the constructor is called, in this case self::$socketClass, is not covered by DIP. The solution above doesn't look clean to me.

What can I do?

[peter-gribanov]

1. The Socket::__construct() is a public method, so we can break the expected behavior by substituting a resource from another source.

   $socket = new Socket(mysql_connect());
   

2. The Socket::fsockopen() method is a kind of factory. This is not a bad solution. In some cases, factories are very convenient. But such factories are not compatible with DI and method Tcp::setSocketFunctionsClass() is a good example of this.

DI is dependency injection. There is no injection of dependencies in your code. You initialize the dependency by the class name. Dependency injection would be if you had service SocketFunctionsInterface passed in constructor arguments like this:

final class Tcp implements CommunicationInterface
{
    private SocketFunctionsInterface $socket;

    public function __construct(SocketFunctionsInterface $socket)
    {
        $this->socket = $socket;
    }
}

I would implement it somehow like this:

interface Stream
{
    public function open(): void;

    public function close(): void;

    public function write(string $string): bool;

    public function read(): string;
}

final class SocketStream implements Stream
{
    private string $hostname;
    private int $port;
    private int $timeout;
    private ?resource $socket = null;

    public function __construct(string $hostname, int $port, int $timeout)
    {
        $this->hostname = $hostname;
        $this->port = $port;
        $this->timeout = $timeout;
    }

    public function open(): void
    {
        if (is_resource($this->socket)) {
            throw new StreamStateException('Stream already opened.');
        }

        $socket = fsockopen($this->hostname, $this->port, $errno, $errstr, $this->timeout);

        if (!is_resource($socket)) {
            throw new OpenStreamException($errstr, $errno);
        }

        $this->socket = $socket;
    }

    public function close(): void
    {
        if (is_resource($socket)) {
            fclose($this->socket);
            $this->socket = null;
        }
    }

    public function write(string $string): bool
    {
        if (!is_resource($this->socket)) {
            throw new StreamStateException('Stream not opened.');
        }

        return (bool) fwrite($this->socket, $string);
    }

    public function read(): string
    {
        if (!is_resource($this->socket)) {
            throw new StreamStateException('Stream not opened.');
        }

        return (string) fgetc($this->socket);
    }

    public function __destruct()
    {
        $this->close();
    }
}

final class Tcp implements Communication
{
    private Stream $stream;

    public function __construct(Stream $stream)
    {
        $this->stream = $stream;
    }

    public function open(): void
    {
        // Infinite loop to try to establish a connection. I do not advise doing this
        while (true) {
            try {
                $this->stream->open();
                break;
            } catch (OpenStreamException $e) {
                // ignore connection errors
                usleep(100000); // wait before next connection
            }
        }
    }
}

I prefer not to initialize the connection in the constructor and only initialize it when really needed.

[gregor-j]

Thank you for the fast reply.

I was a bit reluctant, when it came to opening the connection outside of the constructor. That way I would be able to rely, that the connection already has been established once I got an object, avoiding connection related exceptions and code handling the connection status outside of connection-related methods. However that reluctance finally lead to my dilemma (and a bug regarding the Socket constructor :facepalm:).

Thanks for opening my eyes. My question has been answered.

[peter-gribanov]

The main problem is that you want to retry the connection if the connection fails. This requires a method to explicitly make the connection, such as Stream::open().

final class Tcp implements Communication
{
    private Stream $stream;

    public function __construct(Stream $stream)
    {
        $this->stream = $stream;

        // Infinite loop to try to establish a connection
        while (true) {
            try {
                $this->stream->open();
                break;
            } catch (OpenStreamException $e) {
                // ignore connection errors
                usleep(100000); // wait before next connection
            }
        }
    }
}

Or you can reconnect in the Stream service.

final class SocketStream implements Stream
{
    private resource $socket;

    public function __construct(string $hostname, int $port, int $timeout)
    {
        // Infinite loop to try to establish a connection
        while (true) {
            $socket = fsockopen($hostname, $port, $errno, $errstr, $timeout);

            if (is_resource($socket)) {
                $this->socket = $socket;
                break;
            }

            // ignore connection errors
            usleep(100000); // wait before next connection
        }
    }

    // ...
}

final class Tcp implements Communication
{
    private Stream $stream;

    public function __construct(Stream $stream)
    {
        // connection established
        $this->stream = $stream;
    }
}

Or you can make a connection factory

interface StreamFactory
{
    public function open(): Stream;
}

final class SocketStream implements Stream
{
    private resource $socket;

    public function __construct(string $hostname, int $port, int $timeout)
    {
        $socket = fsockopen($hostname, $port, $errno, $errstr, $timeout);

        if (!is_resource($socket)) {
            throw new OpenStreamException($errstr, $errno);
        }

        $this->socket = $socket;
    }

    // ...
}

final class Tcp implements Communication
{
    private Stream $stream;

    public function __construct(StreamFactory $factory)
    {
        // Infinite loop to try to establish a connection
        while (true) {
            try {
                $stream = $factory->open();
                break;
            } catch (OpenStreamException $e) {
                // ignore connection errors
                usleep(100000); // wait before next connection
            }
        }

        $this->stream = $stream;
    }
}

I also recommend limiting the number of connection attempts. You can specify a negative number to make the loop endless.

final class Tcp implements Communication
{
    private Stream $stream;

    public function __construct(Stream $stream, int $attempts)
    {
        $this->stream = $stream;

        while ($attempts--) {
            try {
                $this->stream->open();
                break;
            } catch (OpenStreamException $e) {
                // ignore connection errors
                usleep(100000); // wait before next connection
            }
        }
    }
}

[gregor-j]

I wrote a Socket class implementing a Stream interface according to your example. You are mentioned as one of the authors, because the DIP solution was yours. I hope, that's ok.

I solved the connection retry problem using a recursive method call in the SerialPort constructor (formerly known as Tcp class).

Features:

• There is always at least one connection attempt, which makes checking for $attempts > 0 obsolete.
• After the connection attempts are used up, the last exception gets thrown.
• The constructor accepts the retry wait time in seconds which then get converted to microseconds. That's supposed to make the class more useable and less prone to errors due to forgotten zeros.

final class SerialPort implements Communication
{
    private const MICROSECONDS_PER_SECOND = 1000000;

    public const DEFAULT_ATTEMPTS = 3;

    public const DEFAULT_RETRY_WAIT = 1.0;

    private $stream;

    public function __construct(Stream $stream, int $attempts = null, float $retryWait = null)
    {
        $this->stream = $stream;
        $attempts = $attempts ?: self::DEFAULT_ATTEMPTS;
        $retryWait = $retryWait ?: self::DEFAULT_RETRY_WAIT;
        $retryWait *= self::MICROSECONDS_PER_SECOND;
        $this->connect($attempts, (int)$retryWait);
    }

    private function connect(int $attemptsLeft, int $retryWait): void
    {
        try {
            $this->stream->open();
        } catch (OpenStreamException $exception) {
            --$attemptsLeft;
            if ($attemptsLeft < 1) {
                throw $exception;
            }
            usleep($retryWait);
            $this->connect($attemptsLeft, $retryWait);
        }
    }
    //...
}