yiisoft/yii2-symfonymailer

Messy implementation of private $_transport

SamMousa opened this issue · 3 comments

What steps will reproduce the problem?

Inspect the code; it is functional but the implementation is messy.

First of all note that the variable is private:

    /**
     * @var TransportInterface|array Symfony transport instance or its array configuration.
     */
    private $_transport = [];

This means the only way it ever gets set is via setTransport:

/**
     * @param array|TransportInterface $transport
     * @throws InvalidConfigException on invalid argument.
     */
    public function setTransport($transport): void
    {
        if (!is_array($transport) && !$transport instanceof TransportInterface) {
            throw new InvalidConfigException('"' . get_class($this) . '::transport" should be either object or array, "' . gettype($transport) . '" given.');
        }
        if ($transport instanceof TransportInterface) {
            $this->_transport = $transport;
        } elseif (is_array($transport)) {
            $this->_transport = $this->createTransport($transport);
        }

        $this->symfonyMailer = null;
    }

Looking at this code we can actually conclude that $this->_transport will never be an array.
In php 8.1 we could write it like this:

private TransportInterface $_transport;

public function setTransport(TransportInterface|array $transport): void
{
    $this->_transport = $transport instanceof TransportInterface ? $transport : $this->createTransport($transport);
}

Given that createTransport() always returns a created transport, or throws an exception we can be 100% sure that after a call to setTransport() we get either an exception or the transport was successfully created and set.

This means we can, and should remove getTransport() alternatively, at least we should simplify it:

public function getTransport(): TransportInterface
{
    return $this->_transport;
}

Last but not least, Yii uses a pattern where init() is called after configuration which means we should use it to test for correct configuration. This will throw an error closer to the error (on / after instantiation instead of on use)

public function init() {
    if (!isset($this->_transport)) {
        throw new InvalidConfigException("No transport configured");
    }
}

@SamMousa, thank you for your constructive criticism. Judging by the content #27 fixes most of this.

Yeah, turned out to be a bit of a full rewrite though 😅

Fixed in #27