Upgrades to typehints and PHP 8.4 method signatures #18
Conversation
|
I suggest also introducing a backward compatibility check. |
This is what the tool reported. |
|
I think my suggestion was too vague: the backward compatibility check should be added to the CI pipeline.
BC breaks are not allowed in a minor version therefore these changes must be reverted or a new major version is required. |
There was a problem hiding this comment.
Hey @niden thank you so much for this work! This is the first pass at a review; I expect to send a couple more before we're through.
FWIW there's no need to (as far as project standards are concerned) to do alignment of equals, or docblock params, etc.
| $result = parent::bindValue($parameter, $value, $dataType); | ||
|
|
||
| if ($result && $this->logEntry !== null) { | ||
| if ($result && !empty($this->logEntry)) { |
There was a problem hiding this comment.
I don't think empty() is quite the same thing as !== null -- can you say why the change?
There was a problem hiding this comment.
The logEntry is defined as an array in the constructor so it does not accept nulls, as such it will not be null here. We can change it if you wish but then line 29 will complain (constructor).
| } | ||
|
|
||
| public function fetch( | ||
| int $fetch_style = null, | ||
| int $fetch_style = PDO::FETCH_DEFAULT, |
There was a problem hiding this comment.
This looks like a BC break ... ?
There was a problem hiding this comment.
It is. If I run phpstan for 8.0 it complains:
Parameter #1 $mode of method PDOStatement::fetch() expects int, int|null given.
For PHP 8.4 it complains for the lack of ?int also.
I am not sure how to tackle this. Perhaps an exception in phpstan config?
| } | ||
|
|
||
| public function fetchAll( | ||
| int $fetch_style = PDO::FETCH_BOTH, | ||
| int $fetch_style = PDO::FETCH_DEFAULT, |
There was a problem hiding this comment.
This also looks like a BC break ... ?
There was a problem hiding this comment.
I was following the tests on this. If FETCH_BOTH is set then the Atlas\Pdo\PersistentLoggedStatementTest::testFetchAll test fails (because there are more data than what the test checks for). We can certainly adjust the test and leave this as it was.
| public function fetchObject( | ||
| ?string $class_name = 'stdClass', | ||
| ?array $ctor_args = [] | ||
| array $ctor_args = [] |
There was a problem hiding this comment.
For this one again phpstan complains:
Parameter #2 $constructorArgs of method PDOStatement::fetchObject() expects array, array|null given.
We can leave the original types and add an exception to phpstan.
| @@ -164,7 +203,7 @@ public function errorCode() : ?string | |||
| return $this->parent->errorCode(); | |||
| } | |||
|
|
|||
| public function errorInfo() : ?array | |||
| public function errorInfo() : array | |||
There was a problem hiding this comment.
This relates to the comment I left in the description of the PR.
Return type array|null of method Atlas\Pdo\PersistentLoggedStatement::errorInfo() is not covariant with tentative return type array of method PDOStatement::errorInfo().
Either this change or #[\ReturnTypeWillChange]
|
@pmjones Thanks for the review. Some replies above when you get a chance. |
src/Connection.php
Outdated
| * @method string quote(mixed $string, int $parameterType = PDO::PARAM_STR) | ||
| * @method mixed setAttribute(int $attribute, mixed $value) | ||
| * | ||
| * @phpstan-type dsnArgs array{ |
There was a problem hiding this comment.
For non-classlike types, I prefer to see lower-snake-case, with the type as a suffix if possible. That helps to distinguish them from classlike types, and follows the PHPStan naming style pretty closely (though we cannot use dashes in userland custom types.) So, dsn_args_array or something like that.
src/Connection.php
Outdated
| * options?: array<int, mixed> | ||
| * } | ||
| * | ||
| * @phpstan-type logEntryType array{ |
There was a problem hiding this comment.
Likewise, maybe something like log_entry_array.
src/Connection.php
Outdated
|
|
||
| public function __construct(protected PDO $pdo) | ||
| { | ||
| $this->persistent = $this->pdo->getAttribute(PDO::ATTR_PERSISTENT); | ||
| $this->persistent = (bool)$this->pdo->getAttribute(PDO::ATTR_PERSISTENT); |
There was a problem hiding this comment.
Nit: space between (bool) and $this.
src/Connection.php
Outdated
| @@ -164,6 +190,7 @@ protected function performBind( | |||
| } | |||
|
|
|||
| if (! is_array($args)) { | |||
| /** @var int|string $name */ | |||
There was a problem hiding this comment.
Might be better to put this as @param int|string $name on the method?
src/Connection.php
Outdated
| @@ -253,6 +301,7 @@ public function fetchObjects( | |||
| ) : array|false | |||
| { | |||
| $sth = $this->perform($statement, $values); | |||
| /** @var array<array-key, callable|int|string> $args */ | |||
src/Connection.php
Outdated
| @@ -262,7 +311,10 @@ public function fetchOne( | |||
| ) : array|false | |||
| { | |||
| $sth = $this->perform($statement, $values); | |||
| return $sth->fetch(PDO::FETCH_ASSOC); | |||
| /** @var array<array-key, mixed> $result */ | |||
There was a problem hiding this comment.
Might this be good as a custom type, e.g. fetch_assoc_array mixed[] and the the @return type would be fetch_assoc_array|false ?
src/Connection.php
Outdated
| @@ -306,6 +358,7 @@ public function yieldUnique( | |||
| $sth = $this->perform($statement, $values); | |||
|
|
|||
| while ($row = $sth->fetch(PDO::FETCH_UNIQUE)) { | |||
| /** @var array<string, mixed> $row */ | |||
There was a problem hiding this comment.
Maybe another opportunity for a custom type.
| class ConnectionLocator | ||
| { | ||
| /** | ||
| * @var string | ||
| */ | ||
| public const DEFAULT = 'DEFAULT'; |
There was a problem hiding this comment.
I don't think it's a BC break to add string typehints on these ... though it might not be 8.0 compatible. Thoughts?
There was a problem hiding this comment.
We cannot add public const string DEFAULT because it is not supported in php 8.0.
Sadly public const /* string */ DEFAULT = 'DEFAULT'; does not work. Phpstan still complains about it.
| @@ -79,14 +110,17 @@ public function setWriteFactory( | |||
|
|
|||
| public function getDefault() : Connection | |||
| { | |||
| if ($this->instances[static::DEFAULT] === null) { | |||
| $this->instances[static::DEFAULT] = $this->newConnection( | |||
| /** @var 'DEFAULT' $type */ | |||
There was a problem hiding this comment.
This might not be necessary with a switch to const string DEFAULT.
src/ConnectionLocator.php
Outdated
| @@ -127,7 +161,12 @@ protected function getConnection( | |||
| } | |||
|
|
|||
| if (! empty($this->instances[$type])) { | |||
| return reset($this->instances[$type]); | |||
| /** @var array<string, array<string, Connection>> $instances */ | |||
There was a problem hiding this comment.
Is this a repetition of the connectionStore type?
There was a problem hiding this comment.
Somewhat. I have created a new type to handle this and the store array.
|
@pmjones thanks for the second pass. I fixed most of it, explanations on the ones I could not. |
Changed minimum version to PHP 8.1 << Please advise if this is acceptable(review)Added version to composer.json(review)Upgraded phpunit.xml.dist for the new phpunit version(review)Added docblocks for all methods(out of scope)LoggerStatement::bindValue()to check withempty()instead ofnullPersistentStatementusages offunc_get_args()to passed parameters or vadiadic ones (soothing stan)PersistentStatement::errorInfo()to return only array (parent interface changed)PersistentStatement::fetch()to usePDO::FETCH_DEFAULTinstead ofnull.PersistentStatement::fetchAll()to returnarrayinstead ofarray|falseand usePDO::FETCH_DEFAULTinstead ofPDO::FETCH_BOTH.For this one we can also use
#[\ReturnTypeWillChange]for now, since the return typePersistentLoggedStatement::fetchAll()is not covariant withPDOStatement::fetchAll(). As for the mode change, the relevant test was failing without that change. We can either adjust the test or leave that change there.Please advise what needs to be changed.
Thanks