Discussion: Interface Discrepancies

[MGatner]

Introduction

I want to begin a "big picture" discussion about something that has caused numerous issues in the past: framework interfaces. Let me also point out up front that this is currently a problem. We have done a handful of patch jobs to "make things work" but they have not addressed the underlying issues. A few references for example:

• #4329
• #4280
• #4285
• #3900

Interfaces

An interface is a contract between the code and our developers:

1. A class that implements an interface must have all the defined methods.
2. An interface cannot be changed without a major release.
3. Anything that uses an interface in a typehint or docblock must use only that interface's methods.

Problem

Number three is where we get in trouble. Due to the nature of frameworks we need to move classes around a lot. In order to allow extending/replacing core components we use interfaces. In our current code base, sometimes this has been done well and maintained and sometimes it has not. Two examples...

Examples

CacheInterface

The CacheInterface is an example of interfacing handled well. The interface has 10 public methods, all the handlers implement those methods without embellishment, and our Cache Service returns the interface: https://github.com/codeigniter4/CodeIgniter4/blob/ecb250777679fa0d2c8638bd3d4012c67cf5a706/system/Config/Services.php#L90-L99

This means that other components like Throttler can use CacheInterface safely: https://github.com/codeigniter4/CodeIgniter4/blob/ecb250777679fa0d2c8638bd3d4012c67cf5a706/system/Throttle/Throttler.php#L30-L35

... and any developer who wished to use custom caching can define their own.

ConnectionInterface

I'm going to pick on the database layer because of this recent issue (#4329) and because it is a bit of a mess generally. ConnectionInterface has 16 public methods but they are not nearly comprehensive enough to support all our required database methods so we use BaseConnection implements ConnectionInterface and then have handlers extend BaseConnection. This way when MigrationRunner needs to check for the "migrations" table is can use the embellished method provided by BaseConnection: https://github.com/codeigniter4/CodeIgniter4/blob/ecb250777679fa0d2c8638bd3d4012c67cf5a706/system/Database/MigrationRunner.php#L935-L944

This works because MigrationRunner knows its protected $db property is a BaseConnection: https://github.com/codeigniter4/CodeIgniter4/blob/ecb250777679fa0d2c8638bd3d4012c67cf5a706/system/Database/MigrationRunner.php#L70-L76

That looks fine, but OOPS! our migration service expects a ConnectionInterface: https://github.com/codeigniter4/CodeIgniter4/blob/ecb250777679fa0d2c8638bd3d4012c67cf5a706/system/Config/Services.php#L425-L434

Now we have a conundrum. Do we rework dependent classes so they only use the interface methods? Or do we redefine all services to use BaseConnection so we have access to these embellished methods? If we do the latter, then why have ConnectionInterface at all?

Road Forward

Unfortunately ConnectionInterface and MigrationRunner aren't an isolate incident. We have numerous infractions of this time throughout the framework, the ones I am most aware of being in the database and HTTP layers. Given the current "broken" state of things I think we have some leeway in the changes we can make, but since we are not ready for version 5 yet these will have to be handled delicately. Here's my proposal but I'd like to hear ideas from others as well:

1. Identify comprehensive and intact interfaces (like Cache) and standardize their references through the code
2. Identify abstract and base classes that should be used instead of interfaces, update all references and deprecated the interfaces
3. Identify interfaces we want to keep and push intermediate implementations back as far as possible (e.g. no "MyClass extends MyImplementationOfInterface")
4. Identify non-comprehensive interfaces that cannot be replaced and make new interfaces (extensions) to be used instead

What are your thoughts? criticisms? solutions?

[najdanovicivan]

I'd add a suggestion when it comes to interfaces.

We have BaseConnection and it should be as simple as possible then we can add BaseSQLConnection If we need to handle something only SQL related. For Migration I'd add separate interface MigrationConnection and so on. It's better to have some classes implement multiple simple interface instead of one huge cause it gives much more space for future extension.

The one issue I had faced is that I wanted to use migrations with custom ElasticSearch driver. It did not allowed it to run until I defined Forge class which was basically passing everything to ElasticSearch\Client with magic __call.

So we need to do some investigation so that we can come up with the best possible solution here as we cannot just add all methods in the interface as for some things it might not make any sense at all. For example ElasticSearch does not have databases and BaseConnection makes it required so I have this

	/**
	 * Set Database
	 *
	 * @param string $databaseName Name
	 *
	 * @return boolean
	 */
	public function setDatabase(string $databaseName) : bool
	{
		if (empty($this->connID))
		{
			$this->initialize();
		}

		return true;
	}

Basically true is always returned just so that it meets the interface requirements.

[kenjis]

I think this is a very difficult question.

I have found the following. https://github.com/codeigniter4/CodeIgniter4/blob/4cbfe2cd25ff8b08eee323cc734a2113cead8eb1/phpstan.neon.dist#L42-L44

[WinterSilence]

What's problem? SRP + ISP(SOLID) - separate interfaces and use as compositions. Sometimes possible downgrade type of parameters in extends: interface::method(array $rows); > class::method(iterable $rows); + type covariance >= 7.4. Abstract classes are not replacements for interfaces - they less flexible. In case ConnectionInterface we have 3 in 1: query(query, simpleQuery), connetion(connect, reconnect), info/helper(escape, getPlatform). It's similar to http request: message/request - query, client - connection, helper - it's private toolbox, strategy, bridge.

[WinterSilence]

Real problem is methods like as BaseConnection::query - @return BaseResult|Query|boolean - no common entry points, that's code hard to validate and maintain.

interface ResultInterface
{
    public function getState(): bool;
    public function getError(): ?string /* ?Stringable */;
    public function getAffectedRows(): int;
    public function __get(string $key)/* : mixed */;
}

interface SelectResultInterface extends ResultInterface, IteratorAggregate {}
interface InsertResultInterface extends ResultInterface {}
interface DeleteResultInterface extends ResultInterface  {}

class SelectResult implements SelectResultInterface
{
    protected $rows;
    protected $error;
    protected $state;
    public function __construct($raw, $error = null)
    {
        $this->rows = is_callable($raw) ? $raw($this) : $raw;
        if (!is_iterable($this->rows)) {
            throw new InvalidArgumentException('Invalid type of RAW data');
        }
        $this->state = $error === null;
        if ($error !== null) {
            $this->error = (string) $error;
        }
    }

class SelectResultArrayDecorator extends SelectResult, implements ArrayAccess, Countable {}

[MGatner]

@kenjis Those are great examples of the problem: the interface itself has a method definition but in actuality it is a base class being passed around which has embellished versions of the methods. Because of the explicit interface typehint PHPStan expects the interface version of the method to be used. These would be places the framework would "break" if someone provided their own interface via Services.

@WinterSilence Thank you for your input. A couple responses.

separate interfaces and use as compositions

The overall structure isn't the problem, it is the discrepancy between what is supplied via Services (in most cases) or injected as a dependency (in a few cases) versus the actual interface.

Abstract classes are not replacements for interfaces - they less flexible

Abstract classes and interfaces offer two very different toolsets. One should not "replace" the other, but in reality they are currently used interchangeably, which is the core of the problem and hence this discussion

interface ResultInterface

I appreciate the example. One of the core issues brought up for this discussion is that we cannot release version 5, so we are stuck with non-breaking changes. I would love to have a fresh pass at all the interfaces and abstract base classes and "do things right", but we don't have that flexibility. Most of us are relatively new to the project and have inherited these problems, but it's still ours to deal with. We need a solution to hold things together for now, and in version 5 we can rework all these properly.

[WinterSilence]

@ MGatner I'm have same problems before and discover how do it other teams:

• start voting "What's better: rework to modern solutions or keep BC & legacy code"
• break BC only in major version (your already did it in 4.0)
• create migration tools and and add @deprecated tags to mark changes by next minor version

If PHP grow up and break BS then frameworks must support it. I think breaking BS rules at every 3-4 years it's normal practice. It's better then zombie projects without users.

Sad but true: if team don't constantly add new whistles and fakes to project, then top bloggers will not post about framework = employers/HR's too be ignore it.

My little contribution:

/**
 * Call this within your method to mark it deprecated.
 * 
 * @param string $replacement Optional replacement function to use instead
 * @param string $since Version since this function shall be marked deprecated
 */
function deprecated(string $replacement = '', string $since = '5.0.0')
{
    $context = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 2)[1];
    $context['since'] = $since;
    $msg = 'Method {class}::{function}() is deprecated since version {since}'
        . ' and will be removed within the next major release.';
    if ($replacement) {
        $msg .= ' Please consider replacing it with "{replacement}".';
        $context['replacement'] = $replacement;
    }
    Services::logger()->warning($msg, $context);
    if (CI_DEBUG) {
        trigger_error("Method {$class}::{$function}() is deprecated, check log", E_USER_NOTICE);
    }
}

usage:

function _legacy_code()
{
     deprecated('newClass::modernMethod()');
     ...
}

Also, i have plugin to check user's code and append comment to property/function by name. It's can be use as Composer's command running on update. If need I try find that's solution.

[kenjis]

Now we are fixing Interfaces in 4.3 branch.

[kenjis]

Ref #6776

[kenjis]

The one issue I had faced is that I wanted to use migrations with custom ElasticSearch driver. It did not allowed it to run until I defined Forge class which was basically passing everything to ElasticSearch\Client with magic __call.

Real problem is methods like as BaseConnection::query - @return BaseResult|Query|boolean - no common entry points, that's code hard to validate and maintain.

Abstract classes and interfaces offer two very different toolsets. One should not "replace" the other, but in reality they are currently used interchangeably, which is the core of the problem and hence this discussion

It seems we need to learn ISP.