Upgrades to typehints and PHP 8.4 method signatures

[niden]

• Changed minimum version to PHP 8.1 << Please advise if this is acceptable (review)
• Added version to composer.json (review)
• Upgraded phpstan and phpunit (composer)
• Added github actions workflow to run phpstan and the tests
• Upgraded phpunit.xml.dist for the new phpunit version (review)
• Added docblocks for all methods (out of scope)
• Added phpstan types for logEntry, connection instances etc.
• Adjusted tests
• Changed implicitly nullable variables to accept null also (PHP 8.4)
• Changed logic in LoggerStatement::bindValue() to check with empty() instead of null
• Changed PersistentStatement usages of func_get_args() to passed parameters or vadiadic ones (soothing stan)
• Changed PersistentStatement::errorInfo() to return only array (parent interface changed)
• Changed PersistentStatement::fetch() to use PDO::FETCH_DEFAULT instead of null.
• Changed PersistentStatement::fetchAll() to return array instead of array|false and use PDO::FETCH_DEFAULT instead of PDO::FETCH_BOTH.

For this one we can also use #[\ReturnTypeWillChange] for now, since the return type PersistentLoggedStatement::fetchAll() is not covariant with PDOStatement::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

[froschdesign]

I suggest also introducing a backward compatibility check.

[niden]

I suggest also introducing a backward compatibility check.

This is what the tool reported.

[BC] CHANGED: Default parameter value for parameter $fetch_style of Atlas\Pdo\PersistentLoggedStatement#fetch()
    changed from NULL to 0
[BC] CHANGED: The parameter $fetch_style of Atlas\Pdo\PersistentLoggedStatement#fetch() changed from int|null 
    to a non-contravariant int
[BC] CHANGED: The parameter $fetch_style of Atlas\Pdo\PersistentLoggedStatement#fetch() changed from int|null 
    to int
[BC] CHANGED: Default parameter value for parameter $fetch_style of Atlas\Pdo\PersistentLoggedStatement#fetchAll()
     changed from 4 to 0
[BC] CHANGED: The return type of Atlas\Pdo\PersistentLoggedStatement#fetchAll() changed from array|false 
    to array
[BC] CHANGED: The parameter $ctor_args of Atlas\Pdo\PersistentLoggedStatement#fetchObject() changed from 
    array|null to a non-contravariant array
[BC] CHANGED: The parameter $ctor_args of Atlas\Pdo\PersistentLoggedStatement#fetchObject() changed from 
    array|null to array
[BC] CHANGED: The return type of Atlas\Pdo\PersistentLoggedStatement#errorInfo() changed from array|null 
    to array

[froschdesign]

I think my suggestion was too vague: the backward compatibility check should be added to the CI pipeline.

This is what the tool reported.

BC breaks are not allowed in a minor version therefore these changes must be reverted or a new major version is required.

[pmjones]

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.

[pmjones]

I don't think empty() is quite the same thing as !== null -- can you say why the change?

[niden]

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).

[pmjones]

This looks like a BC break ... ?

[niden]

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?

[pmjones]

This also looks like a BC break ... ?

[niden]

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.

[pmjones]

Also a BC break ... ?

[niden]

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.

[pmjones]

BC break ... ?

[niden]

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]

[niden]

@pmjones Thanks for the review. Some replies above when you get a chance.

[pmjones]

Second pass, still more to go.

In some other projects I have started added a {Project}CustomTypes interface or final class, and then import everything from that. Something to consider here, I think.

[pmjones]

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.

[pmjones]

Likewise, maybe something like log_entry_array.

[pmjones]

Nit: space between (bool) and $this.

[pmjones]

Might be better to put this as @param int|string $name on the method?

[pmjones]

Would this @var be better as @param on the method signature?

[pmjones]

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 ?

[pmjones]

Maybe another opportunity for a custom type.

[pmjones]

I don't think it's a BC break to add string typehints on these ... though it might not be 8.0 compatible. Thoughts?

[niden]

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.

[pmjones]

This might not be necessary with a switch to const string DEFAULT.

[pmjones]

Is this a repetition of the connectionStore type?

[niden]

Somewhat. I have created a new type to handle this and the store array.

[niden]

@pmjones thanks for the second pass. I fixed most of it, explanations on the ones I could not.