Merge 3.1.x into 3.2.x #4668
Merged
Merge 3.1.x into 3.2.x #4668
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…er::getConnection($args)` params As per doctrine#4637, `vimeo/psalm:4.6.4` cannot introduce templated parameters within a re-used imported type: this is understandable, since imported types are copy-paste aided by the type checker, but do not carry any template information with them (and it would be too complex to do so anyway). Quoting the initial issue: Minimal reproducer: https://psalm.dev/r/62a0a5854d ```php <?php class Connection {} class MyConnection extends Connection{} /** * @psalm-type Params = array{ * charset?: string, * wrapperClass?: class-string<Connection>, * } */ final class DriverManager { /** * @param array<string,mixed> $params * @param Configuration|null $config The configuration to use. * @param EventManager|null $eventManager The event manager to use. * * @phpstan-param array<string,mixed> $params * @psalm-param Params $params * @psalm-return ($params is array{wrapperClass:mixed} ? T : Connection) * @template T of Connection */ public static function getConnection( array $params ): Connection { throw new \Exception('irrelevant'); } } function mkConnection(): MyConnection { return DriverManager::getConnection(['wrapperClass' => MyConnection::class]); } ``` Which produces: ``` Psalm output (using commit aa854ae): INFO: MixedInferredReturnType - 31:26 - Could not verify return type 'MyConnection' for mkConnection ``` The issue is that the `Param` alias type is not templated: this is potentially a psalm limitation, but also an understandable one, since there is no syntax to specify template arguments for an imported type (AFAIK). Here is a potential fix: https://psalm.dev/r/8ef9909bb9 ```php <?php class Connection {} class MyConnection extends Connection{} /** * @psalm-type Params = array{ * charset?: string, * wrapperClass?: class-string<Connection>, * } */ final class DriverManager { /** * @psalm-param array{ * charset?: string, * wrapperClass?: class-string<T>, * } $params * @psalm-return ($params is array{wrapperClass: mixed} ? T : Connection) * @template T of Connection */ public static function getConnection( array $params ): Connection { throw new \Exception('irrelevant'); } } function mkConnection(): MyConnection { return DriverManager::getConnection(['wrapperClass' => MyConnection::class]); } ``` It's a bit ugly, but the idea is that we replace `Params` with the concrete definition again, but this time including `T`, so that the type inference can correctly fill the type variable with the given input: ``` Psalm output (using commit aa854ae): No issues! ``` The idea therefore is to re-introduce some duplication at the advantage of better type inference later on. In order to verify that the change had an effect, we introduced a new `tests/Doctrine/StaticAnalysis` directory, to be checked via `vendor/bin/psalm -c psalm-strict.xml`, which will tell us exactly whether there are obvious type-checker issues around our code. Since `psalm.xml.dist` uses `errorLevel="2"`, it is not sufficient to integrate this directory within the normal workflow. Because this project uses a custom github action for running `vimeo/psalm`, it is not really possible to run this in CI, and therefore that part of the patch has been omitted. It is endorsed that maintainers of `doctrine/dbal` in future: 1. commit `composer.lock` 2. set up dependabot to track dependency upgrades 3. use the standard `vimeo/psalm` via `require-dev`, and run the CI static analysis checks from the same environment that runs normal unit/integration tests
I have tried making it more obvious what all the mandatory steps are to get the caching feature to work.
… types match up The normal psalm checks run in CI are not sufficient, since they run with level 2 checks (too lax), and therefore we need a dedicated action to be run when verifying the contents of `tests/Doctrine/StaticAnalysis`, which is designed to verify type inference and type-checking details provided by the library.
…hole data structure equality While it is true that such tests are accessing unknown/undocumented array keys that are not part of the psalm type definitions, we still need to verify equality on the retrieved data structure, in order to avoid accidentally introducing a BC break while messing with them. This change therefore: * makes the type-checker a bit happier (was failing before, due to undocumented keys) * makes the tests a bit more strict around potential undocumented keys in these arrays
…ore-type-inference-for-wrapped-connection-class Fix doctrine#4637 by duplicating the type definition for `DriverManager::getConnection($args)` params
…n MySQLi Connection
[doctrineGH-4645] Use error suppression instead of an error handler in MySQLi Connection
[doctrineGH-4643] Fix SQLServerPlatform::quoteIdentifier()
Make footgun more obvious
Bump doctrine/coding-standard
Table::removeUniqueConstraint() used the wrong method (hasForeignKey()) instead of hasUniqueConstraint() to check for existing unique constraint, causing removeUniqueConstraint() to fail unconditionally. Also adding tests for removeUniqueConstraint().
…aint-wrong-method Make Table::removeUniqueConstraint() actually work
* 2.13.x: Bump doctrine/coding-standard [doctrineGH-4643] Fix SQLServerPlatform::quoteIdentifier() [doctrineGH-4645] Use error suppression instead of an error handler in MySQLi Connection Rewrote `DriverManagerTest` replica/shards test scenarios to verify whole data structure equality Add type-inference tests to github actions, so that we test also that types match up Add warnings in docblocks Rework the caching documentation Fix doctrine#4637 by duplicating the type definition for `DriverManager::getConnection($args)` params Signed-off-by: Alexander M. Turek <me@derrabus.de>
Merged
morozov
approved these changes
Jun 7, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.