Open MGatner opened 3 years ago
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.
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
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.
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 {}
@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.
@ MGatner I'm have same problems before and discover how do it other teams:
@deprecated
tags to mark changes by next minor versionIf 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.
Now we are fixing Interfaces in 4.3
branch.
Ref #6776
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.
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:
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-L99This means that other components like
Throttler
can useCacheInterface
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 useBaseConnection implements ConnectionInterface
and then have handlers extendBaseConnection
. This way whenMigrationRunner
needs to check for the "migrations" table is can use the embellished method provided byBaseConnection
: https://github.com/codeigniter4/CodeIgniter4/blob/ecb250777679fa0d2c8638bd3d4012c67cf5a706/system/Database/MigrationRunner.php#L935-L944This works because
MigrationRunner
knows its protected$db
property is aBaseConnection
: https://github.com/codeigniter4/CodeIgniter4/blob/ecb250777679fa0d2c8638bd3d4012c67cf5a706/system/Database/MigrationRunner.php#L70-L76That looks fine, but OOPS! our migration service expects a
ConnectionInterface
: https://github.com/codeigniter4/CodeIgniter4/blob/ecb250777679fa0d2c8638bd3d4012c67cf5a706/system/Config/Services.php#L425-L434Now 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 haveConnectionInterface
at all?Road Forward
Unfortunately
ConnectionInterface
andMigrationRunner
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:What are your thoughts? criticisms? solutions?