Skip to content
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

Merge 3.1.x into 3.2.x #4668

Merged
merged 17 commits into from
Jun 7, 2021
Merged

Merge 3.1.x into 3.2.x #4668

merged 17 commits into from
Jun 7, 2021

Commits on May 6, 2021

  1. Fix doctrine#4637 by duplicating the type definition for `DriverManag…

    …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
    Ocramius committed May 6, 2021
    Configuration menu
    Copy the full SHA
    af2a31e View commit details
    Browse the repository at this point in the history
  2. Rework the caching documentation

    I have tried making it more obvious what all the mandatory steps are to
    get the caching feature to work.
    greg0ire committed May 6, 2021
    Configuration menu
    Copy the full SHA
    3a79540 View commit details
    Browse the repository at this point in the history
  3. Add warnings in docblocks

    greg0ire committed May 6, 2021
    Configuration menu
    Copy the full SHA
    9402241 View commit details
    Browse the repository at this point in the history

Commits on May 7, 2021

  1. Add type-inference tests to github actions, so that we test also that…

    … 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.
    Ocramius committed May 7, 2021
    Configuration menu
    Copy the full SHA
    5c4063f View commit details
    Browse the repository at this point in the history
  2. Rewrote DriverManagerTest replica/shards test scenarios to verify w…

    …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
    Ocramius committed May 7, 2021
    Configuration menu
    Copy the full SHA
    cb8b389 View commit details
    Browse the repository at this point in the history

Commits on May 9, 2021

  1. Merge pull request doctrine#4638 from Ocramius/fix/doctrine#4637-rest…

    …ore-type-inference-for-wrapped-connection-class
    
    Fix doctrine#4637 by duplicating the type definition for `DriverManager::getConnection($args)` params
    morozov committed May 9, 2021
    Configuration menu
    Copy the full SHA
    b728198 View commit details
    Browse the repository at this point in the history

Commits on May 12, 2021

  1. Configuration menu
    Copy the full SHA
    b9b30b2 View commit details
    Browse the repository at this point in the history

Commits on May 13, 2021

  1. Merge pull request doctrine#4647 from morozov/issues/4645

    [doctrineGH-4645] Use error suppression instead of an error handler in MySQLi Connection
    morozov committed May 13, 2021
    Configuration menu
    Copy the full SHA
    21f5eb5 View commit details
    Browse the repository at this point in the history

Commits on May 14, 2021

  1. Configuration menu
    Copy the full SHA
    bd18ec4 View commit details
    Browse the repository at this point in the history
  2. Merge pull request doctrine#4648 from morozov/issues/4643

    [doctrineGH-4643] Fix SQLServerPlatform::quoteIdentifier()
    morozov committed May 14, 2021
    Configuration menu
    Copy the full SHA
    69ad868 View commit details
    Browse the repository at this point in the history

Commits on May 15, 2021

  1. Merge pull request doctrine#4640 from greg0ire/make-footgun-more-obvious

    Make footgun more obvious
    greg0ire committed May 15, 2021
    Configuration menu
    Copy the full SHA
    2c19354 View commit details
    Browse the repository at this point in the history

Commits on Jun 5, 2021

  1. Configuration menu
    Copy the full SHA
    ce7db44 View commit details
    Browse the repository at this point in the history
  2. Merge pull request doctrine#4663 from derrabus/bump/cs

    Bump doctrine/coding-standard
    morozov committed Jun 5, 2021
    Configuration menu
    Copy the full SHA
    2f286f4 View commit details
    Browse the repository at this point in the history

Commits on Jun 6, 2021

  1. Make Table::removeUniqueConstraint() actually work

    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().
    hschletz authored and greg0ire committed Jun 6, 2021
    Configuration menu
    Copy the full SHA
    0e398ec View commit details
    Browse the repository at this point in the history
  2. Merge pull request doctrine#4667 from hschletz/fix-removeUniqueConstr…

    …aint-wrong-method
    
    Make Table::removeUniqueConstraint() actually work
    greg0ire committed Jun 6, 2021
    Configuration menu
    Copy the full SHA
    7ef2a33 View commit details
    Browse the repository at this point in the history

Commits on Jun 7, 2021

  1. Merge branch '2.13.x'

    * 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>
    derrabus committed Jun 7, 2021
    Configuration menu
    Copy the full SHA
    e6add29 View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    490f725 View commit details
    Browse the repository at this point in the history