Enable PHPStan strict rules

[morozov]

Q	A
Type	improvement
BC Break	yes

The initial work was done on top of master and then backported to 3.0.x to get the tests improved as well. This is an important addition on top of the existing code quality checks since it helps to find potential bugs and prevents the new code of questionable quality from being introduced.

Additionally, in master the test suite will be cleaned up since a lot of assertions are no longer needed thanks to the improved type system (#3348).

Fixed issues:

The analysis identified a couple of potential issues and deviations from the best practices:

1. The constraint $name variable declared in the foreach loop overrides the table $name method argument that is used down the line. Most likely, a table with such constraints will have an invalid name:

   dbal/src/Platforms/SqlitePlatform.php

   Lines 329 to 331 in 8be6d16

   	foreach ($options['uniqueConstraints'] as $name => $definition) {
   	$queryFields .= ', ' . $this->getUniqueConstraintDeclarationSQL($name, $definition);
   	}

2. This is unlikely a bug but usually, a loose comparison of the strpos() return value looks suspicious:

   dbal/src/Schema/OracleSchemaManager.php

   Line 127 in 8be6d16

   if (strpos($dbType, 'with time zone')) {

3. Also unlikely a bug but depending on the fetch mode, an empty value stored in the cache may be ignored:

   dbal/src/Cache/ResultCacheStatement.php

   Lines 84 to 85 in 936e7e3

   	$data = $this->resultCache->fetch($this->cacheKey);
   	if (! $data) {

4. An empty-ish comment like "0" set on a table in Oracle may be ignored:

   dbal/src/Platforms/OraclePlatform.php

   Lines 788 to 790 in 8be6d16

   	if (! $comment) {
   	continue;
   	}

5. This code is barely readable. The only situation when this expression evaluates to TRUE is when the counts are not equal which should have been coded explicitly:

   dbal/src/Platforms/SQLServer2012Platform.php

   Line 1353 in 8be6d16

   || substr_count($query, '(', $orderByPos) - substr_count($query, ')', $orderByPos)

Technically, BC-breaks:

1. A zero value of the $fetchMode argument in the Statement methods is no longer ignored. Not that anyone should have used it but in order to use the default mode, it should be omitted or expressed as NULL.
2. The empty values of the "connectstring", "host", "service" and "instancename" parameters of the oci8 and pdo_oci drivers are no longer ignored if passed explicitly.
3. The empty value of the "port" parameter of the pdo_sqlsrv driver is no longer ignored if passed explicitly.
4. The following QueryBuilder methods still accept null as an optional parameter but will no longer ignore another explicitly passed empty value: ::select(), ::addSelect(), ::insert(), ::update(), ::delete().
5. Statement::execute(), QueryBuilder::groupBy() and ::addGroupBy() will no longer an explicitly passed non-array empty value.
6. AbstractPlatform::getTrimExpression() will no longer ignore an empty value of $mode (use TrimMode::UNSPECIFIED or omit); and an empty value of $char (use false or omit).

Other empty values in various configurations are not ignored if passed explicitly

Future improvements identified

The errors suppressed in the PHPStan configuration identify the by-design issues that need to be fixed in the future:

1. Internal data structures are represented as untyped associative arrays instead of objects (e.g. connection configuration, table/column definitions). A similar improvement was recently implemented in Introduce properties for SQL parts in QueryBuilder #3836.
2. The AbstractSchemaManager::tryMethod() implements dynamic method calls and needs to be redesigned and internalized to DBAL.

Additional, not strictly relevant issues fixed:

1. The comma in the regular expression pattern doesn't need to be escaped:

   dbal/src/Schema/MySqlSchemaManager.php

   Line 146 in 936e7e3

   if (preg_match('([A-Za-z]+\(([0-9]+)\,([0-9]+)\))', $tableColumn['type'], $match)) {

TODO:

• Rebase on top of Removed the hard-coded MySQL default port and the usage of ini-settings #3933.
• Rebase on top of Removed EchoSQLLogger #3934.
• Document the no longer ignored empty values.
• When merging up to master, revisit the list of whitelisted errors. Some of them will be no longer relevant.

[greg0ire]

Document the no longer ignored empty values.

Is this something you plan to do before merging? Approving in case it isn't

I think Scrutinizer fails because if views the change in SQLSeverSchemaManager as an increase in complexity, but to me the if is easier to understand than the ternary

[morozov]

Is this something you plan to do before merging? Approving in case it isn't

I was going to put this in the PR description, no more code changes expected.

I think Scrutinizer fails because if views the change in SQLSeverSchemaManager as an increase in complexity […]

Yes, and we can't do anything else with it than ignore it.

[…] but to me the if is easier to understand than the ternary

I agree. Furthermore, the original reason for this was to pass the option down the line only if it's explicitly set. There's no point in passing the default explicitly. Also, it makes line coverage report more accurate.