-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added PHPStan, apply changes for level 3 #3025
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say, most if not all places where false
/bool
is added as part of the union type are not by design and should be fixed in a BC way (if possible) or silenced instead of being documented.
lib/Doctrine/DBAL/Connection.php
Outdated
@@ -402,7 +403,7 @@ private function detectDatabasePlatform() | |||
{ | |||
$version = $this->getDatabasePlatformVersion(); | |||
|
|||
if (null !== $version) { | |||
if ($version !== null && $this->_driver instanceof VersionAwarePlatformDriver) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$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
@@ -419,7 +420,7 @@ private function detectDatabasePlatform() | |||
* or the underlying driver connection cannot determine the platform | |||
* version without having to query it (performance reasons). | |||
* | |||
* @return string|null | |||
* @return string|bool|null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say it's a bug and should be fixed, not documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -478,7 +479,7 @@ private function getDatabasePlatformVersion() | |||
/** | |||
* Returns the database server version if the underlying driver supports it. | |||
* | |||
* @return string|null | |||
* @return string|bool|null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boolean is a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
@@ -85,7 +86,7 @@ class MasterSlaveConnection extends Connection | |||
/** | |||
* 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.
@@ -2716,7 +2716,7 @@ public function getCurrentTimestampSQL() | |||
* | |||
* @param int $level | |||
* | |||
* @return string | |||
* @return string|int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all for leaving it for 3.x where we could fix the actual code instead of complicating the API documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please ignore instead.
@@ -423,7 +423,7 @@ public function dropView($name) | |||
* | |||
* @param string $database The name of the database to create. | |||
* | |||
* @return void | |||
* @return void|bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a bug. Mixing something with void doesn't make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, removed. But OracleSchemaManager::createDatabase()
returns true so I removed that one. Should be documented in UPGRADING?
@@ -917,7 +917,7 @@ protected function _getPortableTablesList($tables) | |||
/** | |||
* @param array $table | |||
* | |||
* @return array | |||
* @return mixed[]|string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can elements of the array be anything else than string? string[]|string
would make more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea since $table
is uselessly types as array
. If you are certain, I can change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please figure out the correct type from the API standpoint and ignore the deviations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
------ ------------------------------------------------------------------------------------------------------------------------
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverting and putting it into ignores for now, unlikely to be properly fixable in 2.x
lib/Doctrine/DBAL/Schema/Schema.php
Outdated
@@ -73,7 +73,7 @@ class Schema extends AbstractAsset | |||
protected $_sequences = []; | |||
|
|||
/** | |||
* @var \Doctrine\DBAL\Schema\SchemaConfig | |||
* @var \Doctrine\DBAL\Schema\SchemaConfig|bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nullable type would make more sense here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't be done in 2.x I guess...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's ignore it and fix instead of introducing an intermediate incorrect type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please ignore.
@@ -65,17 +66,17 @@ | |||
class PoolingShardConnection extends Connection | |||
{ | |||
/** | |||
* @var array | |||
* @var DriverConnection[]|null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable could be initialized with an empty array instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -402,7 +403,7 @@ private function detectDatabasePlatform() | |||
{ | |||
$version = $this->getDatabasePlatformVersion(); | |||
|
|||
if (null !== $version) { | |||
if ($version !== null && $this->_driver instanceof VersionAwarePlatformDriver) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$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
@@ -419,7 +420,7 @@ private function detectDatabasePlatform() | |||
* or the underlying driver connection cannot determine the platform | |||
* version without having to query it (performance reasons). | |||
* | |||
* @return string|null | |||
* @return string|bool|null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -478,7 +479,7 @@ private function getDatabasePlatformVersion() | |||
/** | |||
* Returns the database server version if the underlying driver supports it. | |||
* | |||
* @return string|null | |||
* @return string|bool|null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
lib/Doctrine/DBAL/Connection.php
Outdated
@@ -1132,7 +1133,7 @@ public function getTransactionNestingLevel() | |||
/** | |||
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This on the other hand is not a bug, some implementors explicitly return false
(SQLSrvConnection::errorCode()
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DBAL should eliminate this discrepancy. The clients will not have to know the low level details.
lib/Doctrine/DBAL/Connection.php
Outdated
@@ -1163,7 +1164,7 @@ public function errorInfo() | |||
* | |||
* @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here (OCI8Connection::lastInsertId()
).
@@ -85,7 +86,7 @@ class MasterSlaveConnection extends Connection | |||
/** | |||
* 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.
@@ -2716,7 +2716,7 @@ public function getCurrentTimestampSQL() | |||
* | |||
* @param int $level | |||
* | |||
* @return string | |||
* @return string|int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -899,7 +899,7 @@ public function getAlterTableSQL(TableDiff $diff) | |||
/** | |||
* @param \Doctrine\DBAL\Schema\TableDiff $diff | |||
* | |||
* @return array|bool | |||
* @return string[]|false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -423,7 +423,7 @@ public function dropView($name) | |||
* | |||
* @param string $database The name of the database to create. | |||
* | |||
* @return void | |||
* @return void|bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, removed. But OracleSchemaManager::createDatabase()
returns true so I removed that one. Should be documented in UPGRADING?
@@ -65,17 +66,17 @@ | |||
class PoolingShardConnection extends Connection | |||
{ | |||
/** | |||
* @var array | |||
* @var DriverConnection[]|null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
@@ -74,6 +75,7 @@ public function __construct(array $params, $username, $password, $driverOptions | |||
*/ | |||
public function getServerVersion() | |||
{ | |||
/** @var stdClass $serverInfo */ | |||
$serverInfo = db2_server_info($this->_conn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this function always return a stdClass
? Can't we force it in PHPStan stubs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, and false
, how unusual ;)
@@ -300,6 +301,7 @@ public function bindParam($column, &$variable, $type = ParameterType::STRING, $l | |||
$column = $this->_paramMap[$column] ?? $column; | |||
|
|||
if ($type === ParameterType::LARGE_OBJECT) { | |||
/** @var OCI_Lob $lob */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHPStan stubs wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually OCI-Lob
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, that's not a valid class name 🙈 moving to ignore rather than typehinting hack
@@ -99,7 +99,7 @@ public function rollBack(); | |||
/** | |||
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool|null
should be eliminated, not document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignored
@@ -287,7 +288,9 @@ public function fetch($fetchMode = null, $cursorOrientation = \PDO::FETCH_ORI_NE | |||
return $this->fetchColumn(); | |||
} | |||
|
|||
/** @var mixed[]|false $values */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change the _fetch()
'es phpDoc instead?
@@ -333,7 +333,7 @@ public function getOptions() | |||
* Returns the referential action for UPDATE operations | |||
* 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.
@@ -344,7 +344,7 @@ public function onUpdate() | |||
* Returns the referential action for DELETE operations | |||
* 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.
@@ -357,7 +357,7 @@ public function onDelete() | |||
* | |||
* @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
false|null
is undesirable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding to ignore, but it's a bug, false is explicitly returned here :/
lib/Doctrine/DBAL/Schema/Table.php
Outdated
@@ -62,7 +62,7 @@ class Table extends AbstractAsset | |||
protected $_indexes = []; | |||
|
|||
/** | |||
* @var string | |||
* @var string|bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string|false
? Or ignore and rework as string|null
in 3.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will ignore
*/ | ||
private $connections; | ||
private $connections = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
phpstan.neon.dist
Outdated
- '~^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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we generate a stub in the longer term? As long as we officially support the driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we generate a stub in the longer term?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if possible for a class constant...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed JetBrains/phpstorm-stubs#381.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 [![Build Status](https://travis-ci.org/doctrine/dbal.svg?branch=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: