Added PHPStan, apply changes for level 3#3025
Conversation
lib/Doctrine/DBAL/Connection.php
Outdated
| $version = $this->getDatabasePlatformVersion(); | ||
|
|
||
| if (null !== $version) { | ||
| if ($version !== null && $this->_driver instanceof VersionAwarePlatformDriver) { |
There was a problem hiding this comment.
Wouldn't it be enough to check if the driver is not null? Type hint should help static analysis w/o the run time check.
There was a problem hiding this comment.
$this->_platform is typed as AbstractPlatform, but createDatabasePlatformForVersion() is only defined in some drivers implementing VersionAwarePlatformDriver.
I at least replaced this by assert() inside the if since Connection::getDatabasePlatformVersion() returns null for non-version-aware drivers.
lib/Doctrine/DBAL/Connection.php
Outdated
| * version without having to query it (performance reasons). | ||
| * | ||
| * @return string|null | ||
| * @return string|bool|null |
There was a problem hiding this comment.
I'd say it's a bug and should be fixed, not documented.
There was a problem hiding this comment.
Hmm this is because SqlAnywhereConnection::getServerVersion() calls ResultStatement::fetchColumn() which returns string|bool. :| Replaced with assert there and removed here.
lib/Doctrine/DBAL/Connection.php
Outdated
| * Returns the database server version if the underlying driver supports it. | ||
| * | ||
| * @return string|null | ||
| * @return string|bool|null |
| * Master and slave connection (one of the randomly picked slaves). | ||
| * | ||
| * @var \Doctrine\DBAL\Driver\Connection[] | ||
| * @var DriverConnection[]|null[] |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| * @param int $level | ||
| * | ||
| * @return string | ||
| * @return string|int |
There was a problem hiding this comment.
Should it be fixed or silenced instead? Doesn't look like it's by design (the SQLite platform impelementation).
There was a problem hiding this comment.
I can change those ints to strings, but I really have no idea of consequences (either in DBAL or for consumers). SqlAnywherePlatoform returns ints too... Maybe better leave this to 3.x?
There was a problem hiding this comment.
I'm all for leaving it for 3.x where we could fix the actual code instead of complicating the API documentation.
| * @param string $database The name of the database to create. | ||
| * | ||
| * @return void | ||
| * @return void|bool |
There was a problem hiding this comment.
Looks like a bug. Mixing something with void doesn't make sense.
There was a problem hiding this comment.
Yep, removed. But OracleSchemaManager::createDatabase() returns true so I removed that one. Should be documented in UPGRADING?
| * @param array $table | ||
| * | ||
| * @return array | ||
| * @return mixed[]|string |
There was a problem hiding this comment.
Can elements of the array be anything else than string? string[]|string would make more sense.
There was a problem hiding this comment.
No idea since $table is uselessly types as array. If you are certain, I can change it.
There was a problem hiding this comment.
I'd look at it from the caller's standpoint: whatever callers can consume by design (string[] or string) should be valid return, the rest shouldn't.
There was a problem hiding this comment.
Please figure out the correct type from the API standpoint and ignore the deviations.
There was a problem hiding this comment.
------ ------------------------------------------------------------------------------------------------------------------------
Line lib/Doctrine/DBAL/Schema/OracleSchemaManager.php
------ ------------------------------------------------------------------------------------------------------------------------
104 Method Doctrine\DBAL\Schema\OracleSchemaManager::_getPortableTableDefinition() should return array but returns string.
------ ------------------------------------------------------------------------------------------------------------------------
------ ----------------------------------------------------------------------------------------------------------------------------
Line lib/Doctrine/DBAL/Schema/PostgreSqlSchemaManager.php
------ ----------------------------------------------------------------------------------------------------------------------------
222 Method Doctrine\DBAL\Schema\PostgreSqlSchemaManager::_getPortableTableDefinition() should return array but returns string.
------ ----------------------------------------------------------------------------------------------------------------------------
------ ---------------------------------------------------------------------------------------------------------------------------
Line lib/Doctrine/DBAL/Schema/SQLServerSchemaManager.php
------ ---------------------------------------------------------------------------------------------------------------------------
210 Method Doctrine\DBAL\Schema\SQLServerSchemaManager::_getPortableTableDefinition() should return array but returns string.
Honestly, no idea. Some managers return $table as is and some return a string (schema name + table name).
This definitely looks like a bad design (git-blame indicates addition back in 2008-2011).
Help? :)
There was a problem hiding this comment.
reverting and putting it into ignores for now, unlikely to be properly fixable in 2.x
lib/Doctrine/DBAL/Schema/Schema.php
Outdated
|
|
||
| /** | ||
| * @var \Doctrine\DBAL\Schema\SchemaConfig | ||
| * @var \Doctrine\DBAL\Schema\SchemaConfig|bool |
There was a problem hiding this comment.
A nullable type would make more sense here.
There was a problem hiding this comment.
Can't be done in 2.x I guess...
There was a problem hiding this comment.
Let's ignore it and fix instead of introducing an intermediate incorrect type.
| { | ||
| /** | ||
| * @var array | ||
| * @var DriverConnection[]|null |
There was a problem hiding this comment.
The variable could be initialized with an empty array instead.
There was a problem hiding this comment.
Probably ok here and for $connections, changed (close() now sets empty array instead of null).
|
Already prepared changes for |
lib/Doctrine/DBAL/Connection.php
Outdated
| $version = $this->getDatabasePlatformVersion(); | ||
|
|
||
| if (null !== $version) { | ||
| if ($version !== null && $this->_driver instanceof VersionAwarePlatformDriver) { |
There was a problem hiding this comment.
$this->_platform is typed as AbstractPlatform, but createDatabasePlatformForVersion() is only defined in some drivers implementing VersionAwarePlatformDriver.
I at least replaced this by assert() inside the if since Connection::getDatabasePlatformVersion() returns null for non-version-aware drivers.
lib/Doctrine/DBAL/Connection.php
Outdated
| * version without having to query it (performance reasons). | ||
| * | ||
| * @return string|null | ||
| * @return string|bool|null |
There was a problem hiding this comment.
Hmm this is because SqlAnywhereConnection::getServerVersion() calls ResultStatement::fetchColumn() which returns string|bool. :| Replaced with assert there and removed here.
lib/Doctrine/DBAL/Connection.php
Outdated
| * Returns the database server version if the underlying driver supports it. | ||
| * | ||
| * @return string|null | ||
| * @return string|bool|null |
lib/Doctrine/DBAL/Connection.php
Outdated
| * Fetches the SQLSTATE associated with the last database operation. | ||
| * | ||
| * @return int The last error code. | ||
| * @return string|bool|null The last error code. |
There was a problem hiding this comment.
This on the other hand is not a bug, some implementors explicitly return false (SQLSrvConnection::errorCode()).
There was a problem hiding this comment.
DBAL should eliminate this discrepancy. The clients will not have to know the low level details.
lib/Doctrine/DBAL/Connection.php
Outdated
| * @param string|null $seqName Name of the sequence object from which the ID should be returned. | ||
| * | ||
| * @return string A string representation of the last inserted ID. | ||
| * @return string|int|bool A string representation of the last inserted ID. |
There was a problem hiding this comment.
Same here (OCI8Connection::lastInsertId()).
| * Master and slave connection (one of the randomly picked slaves). | ||
| * | ||
| * @var \Doctrine\DBAL\Driver\Connection[] | ||
| * @var DriverConnection[]|null[] |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| * @param int $level | ||
| * | ||
| * @return string | ||
| * @return string|int |
There was a problem hiding this comment.
I can change those ints to strings, but I really have no idea of consequences (either in DBAL or for consumers). SqlAnywherePlatoform returns ints too... Maybe better leave this to 3.x?
| * @param \Doctrine\DBAL\Schema\TableDiff $diff | ||
| * | ||
| * @return array|bool | ||
| * @return string[]|false |
There was a problem hiding this comment.
As much as I hate using false as type, there is no better way around here. Usually these cases are something to replace with null for 3.x.
| * @param string $database The name of the database to create. | ||
| * | ||
| * @return void | ||
| * @return void|bool |
There was a problem hiding this comment.
Yep, removed. But OracleSchemaManager::createDatabase() returns true so I removed that one. Should be documented in UPGRADING?
| { | ||
| /** | ||
| * @var array | ||
| * @var DriverConnection[]|null |
There was a problem hiding this comment.
Probably ok here and for $connections, changed (close() now sets empty array instead of null).
|
Any updates on this one? :) |
|
I'll wait for PHPStan 0.10 (to be released next month) before finishing this. |
|
@morozov @carusogabriel Updated with PHPStan 0.10 and level 3. @morozov Please re-review and point out changes that should be explicitly ignored instead of documented in phpDoc. :) I added some new issues that appeared with 0.9->0.10 update. I also added phpstorm-stubs as a polyfill for some fancier extensions. |
| public function getServerVersion() | ||
| { | ||
| /** @var stdClass $serverInfo */ | ||
| $serverInfo = db2_server_info($this->_conn); |
There was a problem hiding this comment.
Does this function always return a stdClass? Can't we force it in PHPStan stubs?
There was a problem hiding this comment.
I didn't actually check whether it is stdClass or not, it returns some object crate: http://php.net/db2_server_info
There was a problem hiding this comment.
oh, and false, how unusual ;)
| $column = $this->_paramMap[$column] ?? $column; | ||
|
|
||
| if ($type === ParameterType::LARGE_OBJECT) { | ||
| /** @var OCI_Lob $lob */ |
There was a problem hiding this comment.
PHPStan stubs wrong?
There was a problem hiding this comment.
hmm, that's not a valid class name 🙈 moving to ignore rather than typehinting hack
| * Returns the error code associated with the last operation on the database handle. | ||
| * | ||
| * @return string|null The error code, or null if no operation has been run on the database handle. | ||
| * @return string|bool|null The error code, or null if no operation has been run on the database handle. |
There was a problem hiding this comment.
bool|null should be eliminated, not document.
| return $this->fetchColumn(); | ||
| } | ||
|
|
||
| /** @var mixed[]|false $values */ |
There was a problem hiding this comment.
Can we change the _fetch()'es phpDoc instead?
| * on the referenced table the foreign key constraint is associated with. | ||
| * | ||
| * @return string|null | ||
| * @return string|false|null |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| * on the referenced table the foreign key constraint is associated with. | ||
| * | ||
| * @return string|null | ||
| * @return string|false|null |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| * @param string $event Name of the database operation/event to return the referential action for. | ||
| * | ||
| * @return string|null | ||
| * @return string|false|null |
There was a problem hiding this comment.
adding to ignore, but it's a bug, false is explicitly returned here :/
lib/Doctrine/DBAL/Schema/Table.php
Outdated
|
|
||
| /** | ||
| * @var string | ||
| * @var string|bool |
There was a problem hiding this comment.
string|false? Or ignore and rework as string|null in 3.0?
| * @var mixed[] | ||
| */ | ||
| private $connections; | ||
| private $connections = []; |
There was a problem hiding this comment.
mixed[] $connections doesn't look good. As far as I understand, it's not "connections" but rather "connection parameters". Given it's a private property, can it be renamed accordingly?
| - '~^Method Doctrine\\DBAL\\Query\\QueryBuilder::execute\(\) should return Doctrine\\DBAL\\Driver\\Statement\|int but returns Doctrine\\DBAL\\Driver\\ResultStatement\.\z~' | ||
|
|
||
| # http://php.net/manual/en/pdo.sqlitecreatefunction.php | ||
| - '~^Call to an undefined method Doctrine\\DBAL\\Driver\\PDOConnection::sqliteCreateFunction\(\)\.\z~' |
There was a problem hiding this comment.
Should this be reported somewhere upstream?
phpstan.neon.dist
Outdated
| - '~^Method Doctrine\\DBAL\\Platforms\\AbstractPlatform::getListTableForeignKeysSQL\(\) invoked with \d+ parameters, 1 required\.\z~' | ||
|
|
||
| # this method is undeclared, relies on PDO ancestor in implementors | ||
| - '~^Call to an undefined method Doctrine\\DBAL\\Driver\\Statement::nextRowset\(\)\.\z~' |
There was a problem hiding this comment.
Can this be replaced with assert($statement instanceof PDOStatement) in the code?
| reportUnmatchedIgnoredErrors: false | ||
| ignoreErrors: | ||
| # extension not available | ||
| - '~^(Used )?(Function|Constant) sasql_\S+ not found\.\z~i' |
There was a problem hiding this comment.
Could we generate a stub in the longer term? As long as we officially support the driver.
There was a problem hiding this comment.
I have no idea. PhpStorm stubs don't have sasql and I didn't find any documentation either, nor PECL extension. Working with SAP SQL Anywhere is really hard. :(
There was a problem hiding this comment.
For future reference, I found a few working links at https://archive.sap.com/documents/docs/DOC-40537. I'll try to drop the Windows DLL into PHP 5 and see if I can dump the signatures. Sometime.
There was a problem hiding this comment.
The link lead me to: http://dcx.sap.com/index.html#sa160/en/dbprogramming/php-api.html
This looks good enough to build the stubs. Maybe i.e. @carusogabriel may want to help here? :)
But we should probably consider dropping the platform/driver altogether.
Given that DBAL 3 will require PHP 7.2, and SQL Anywhere is barely keeping up (the link only lists PHP 5 modules)...
There was a problem hiding this comment.
Nevermind, did this during Slack outage. 😛
JetBrains/phpstorm-stubs#382
| - '~^Method Doctrine\\DBAL\\Driver\\PDOConnection::\w+\(\) should return Doctrine\\DBAL\\Driver\\Statement but returns PDOStatement\.\z~' | ||
|
|
||
| # may not exist when pdo_sqlsrv is not loaded | ||
| - '~^Access to undefined constant PDO::SQLSRV_ENCODING_BINARY\.\z~' |
There was a problem hiding this comment.
Could we generate a stub in the longer term?
There was a problem hiding this comment.
Not sure if possible for a class constant...
There was a problem hiding this comment.
It's missing from the PDO stub as well as many other driver-specific symbols. I'll file a PR for JetBrains/phpstorm-stubs later.
There was a problem hiding this comment.
Not sure if possible for a class constant…
Oh, I see what you're saying. The class stub will not be loaded for PDO if the extension itself is loaded, even if it's compiled w/o the missing constants. PDO is so nicely designed.
There was a problem hiding this comment.
It's definitely missing in the stubs, but it is also missing in PHP itself unless oci extension is loaded. Who said multiple inheritance in PHP is not possible? 😁
tests/phpstan-polyfill.php
Outdated
|
|
||
| (function () : void { | ||
| static $map = [ | ||
| 'db2' => __DIR__ . '/../vendor/jetbrains/phpstorm-stubs/ibm_db2/ibm_db2.php', |
There was a problem hiding this comment.
Is it really 'db2', not 'ibm_db2'?
var_dump(extension_loaded('db2'));
// bool(false)
var_dump(extension_loaded('ibm_db2'));
// bool(true)
tests/phpstan-polyfill.php
Outdated
| 'db2' => __DIR__ . '/../vendor/jetbrains/phpstorm-stubs/ibm_db2/ibm_db2.php', | ||
| 'mysqli' => __DIR__ . '/../vendor/jetbrains/phpstorm-stubs/mysqli/mysqli.php', | ||
| 'oci8' => __DIR__ . '/../vendor/jetbrains/phpstorm-stubs/oci8/oci8.php', | ||
| 'sqlsrv' => __DIR__ . '/../vendor/jetbrains/phpstorm-stubs/sqlsrv/sqlsrv.php', |
There was a problem hiding this comment.
Given that the stub files are nicely organized, in order to avoid copy/paste and simplify maintenance, can this be reworked as:
$extensions = ['ibm_db2', ...];
foreach ($extensions as $extension) {
if (extension_loaded($extension)) {
continue;
}
require sprintf(__DIR__ . '/../vendor/jetbrains/phpstorm-stubs/%1$s/%1$s.php', $extension);
}| - '~^Method Doctrine\\DBAL\\Driver\\SQLSrv\\SQLSrvConnection::errorCode\(\) should return string\|null but returns false\.\z~' | ||
|
|
||
| # actually OCI-Lob - not even a valid class name... | ||
| - '~^Call to an undefined method object::writeTemporary\(\)\.\z~' |
There was a problem hiding this comment.
Hmm… all generated stubs have it as OCI_Lob (probably to make it valid), shouldn't then PHPStan just use the "fixed" names? The oci_new_descriptor() is stubbed as oci_new_descriptor() : OCI_Lob, and OCI_Lob::writeTemporary() is also stubbed, so I'd expect not seeing this error.
There was a problem hiding this comment.
There was a problem hiding this comment.
Not really sure what to think about this. :)
|
CI passed. 💚 |
|
@morozov Do you want this in 2.8 or wait for release and then merge into 2.9? |
|
@Majkl578 I understand it as a non-breaking low-risk, mostly documentation improvement. Unless it's not true, I'm fine with having it in |
|
Yup, and no tests changed so it should be fine. Let's ship it. 🚢 |
|
👍 |
Release v2.8.0 [](https://travis-ci.org/doctrine/dbal) This is a minor release of Doctrine DBAL that aggregates over 30 fixes and improvements developed over the last 3 months. This release includes all changes of the 2.7.x series, as well as feature additions and improvements that couldn’t land in patch releases. **Backwards Compatibility Breaks** This doesn't contain any intentional Backwards Compatibility (BC) breaks. **Dependency Changes** * The dependency on [doctrine/common](https://github.com/doctrine/common) is removed. DBAL now depends on [doctrine/cache](https://github.com/doctrine/cache) and [doctrine/event-manager](https://github.com/doctrine/event-manager) instead. Please see details in the [UPGRADE.md](UPGRADE.md) documentation. **Deprecations** * The usage of binary fields whose length exceeds the maximum field size on a given platform is deprecated. Please use binary fields of a size which fits all target platforms, or use blob explicitly instead. * The usage of DB-generated UUIDs is deprecated. Their format is inconsistent across supported platforms and therefore the feature is not portable. Use a PHP library (e.g. [ramsey/uuid](https://packagist.org/packages/ramsey/uuid)) to generate UUIDs on the application side. **New features** * Initial support of MySQL 8. * Initial support of PostgreSQL 11. * Ability to evaluate arbitrary SQL expressions via `AbstractPlatform::getDummySelectSQL()`. **Improvements and Fixes** * Improved support of binary fields on Oracle and IBM DB2. * Improved SQL Server configuration capabilities via both `sqlsrv` and `pdo_sqlsrv`. * Improved handling of `AUTOINCREMENT`ed primary keys in SQLite. * Integration tests are run against IBM DB2 on Travis CI. * Code coverage is collected for the Oracle platform on continuousphp. Total issues resolved: **33** **Deprecations:** - [3187: Deprecate usage of binary fields whose length exceeds maximum](doctrine#3187) thanks to @morozov - [3188: Deprecated usage of binary fields whose length exceeds the platform maximum](doctrine#3188) thanks to @morozov - [3192: Added more information to the deprecation notice](doctrine#3192) thanks to @morozov - [3212: Deprecated usage of DB-generated UUIDs](doctrine#3212) thanks to @morozov **New Features:** **Bug Fixes:** - [3149: Introduced binary binding type to support binary parameters on Oracle](doctrine#3149) thanks to @morozov - [3178: Fix incorrect exception thrown from SQLAnywhere16Platform](doctrine#3178) thanks to @Majkl578 - [3044: Functional test for allowing dynamic intervals in date sub/add](doctrine#3044) thanks to @AshleyDawson - [3049: Test failures caused by invalid database connection result in fatal error](doctrine#3049) thanks to @Majkl578 **Improvements:** - [3033: Added support for available DSN parameters for the PDOSqlsrv driver](doctrine#3033) thanks to @aashmelev - [3128: Add MySQL 8 reserved keywords](doctrine#3128) thanks to @mlocati - [3143: initialize sql array into platform files](doctrine#3143) thanks to @AlessandroMinoccheri - [3173: Fix composer branch aliases](doctrine#3173) thanks to @Majkl578 - [3157: When building a limit query, zero offset without a limit should be ignored](doctrine#3157) thanks to @morozov - [3109: Allow to specify arbitrary SQL expression in AbstractPlatform::getDummySelectSQL()](doctrine#3109) thanks to @morozov - [3141: allow creating PRIMARY KEY AUTOINCREMENT fields for sqlite (unit tests)](doctrine#3141) thanks to @TimoBakx - [3180: Import simplified version of Common\Util\Debug for var dumping purposes](doctrine#3180) thanks to @Majkl578 **Documentation Improvements:** - [3117: Added badges for the develop branch in README](doctrine#3117) thanks to @morozov - [3125: Upgrading docs](doctrine#3125) thanks to @jwage - [3144: added improvement type into pull request template](doctrine#3144) thanks to @AlessandroMinoccheri **Code Quality Improvements:** - [3025: Added PHPStan, apply changes for level 3](doctrine#3025) thanks to @Majkl578 - [3200: Php Inspections (EA Ultimate): minor code tweaks](doctrine#3200) thanks to @kalessil - [3204: Fix typo in AbstractPlatform](doctrine#3204) thanks to @Majkl578 - [3205: Ignore OCI-* classes in static analysis (no stubs)](doctrine#3205) thanks to @Majkl578 **Continuous Integration Improvements:** - [3102: Use newer PHPUnit to prevent crashes on failures](doctrine#3102) thanks to @Majkl578 - [3112: Removed hard-coded configuration filenames from the test runner](doctrine#3112) thanks to @morozov - [3133: Travis DB2](doctrine#3133) thanks to @Majkl578, @morozov - [3135: AppVeyor tweaks, retry coverage upload on failure](doctrine#3135) thanks to @Majkl578 - [3137: Workaround for the inability to use a post-PHPUnit script on ContinuousPHP](doctrine#3137) thanks to @morozov - [3151: MSSQL DLL 5.2.0 has been released.](doctrine#3151) thanks to @photodude - [3160: Test against Postgres 11](doctrine#3160) thanks to @Majkl578 **Dependencies** - [3193: DBAL 2.8 needs Common 2.9](doctrine#3193) thanks to @Majkl578 - [3176: Eliminate dependency on doctrine/common](doctrine#3176) thanks to @Majkl578 - [3181: Remove dependency on doctrine/common](doctrine#3181) thanks to @Majkl578 # gpg: Signature made Fri Jul 13 06:02:10 2018 # gpg: using RSA key 374EADAF543AE995 # gpg: Can't check signature: public key not found # Conflicts: # .gitignore # README.md # lib/Doctrine/DBAL/Version.php # tests/appveyor/mssql.sql2008r2sp2.sqlsrv.appveyor.xml # tests/appveyor/mssql.sql2012sp1.sqlsrv.appveyor.xml # tests/appveyor/mssql.sql2017.pdo_sqlsrv.appveyor.xml # tests/appveyor/mssql.sql2017.sqlsrv.appveyor.xml # tests/travis/mariadb.mysqli.travis.xml
Mostly annotation changes/fixes to match what the methods/properties really are used for. Added few asserts and instanceof.
Not really sure if you want to consider this as BC issue - I'd hope not, but if so we can retarget to develop instead.
Level 3, not going further as it requires actually changing code itself, that should follow later in develop with strict typing.
There is a PHP notice currently since Travis doesn't have OCI driver: