Remove baseline

[greg0ire]

Q	A
Type	improvement
BC Break	no
Fixed issues	n / a

[greg0ire]

@morozov not sure why this is ignored:

dbal/phpstan.neon.dist

Lines 48 to 49 in 72d9081

	- '~^Class Doctrine\\Common\\(Collections\\Collection|Persistence\\Proxy) not found\.\z~'
	- '~^.+ on an unknown class Doctrine\\Common\\(Collections\\Collection|Persistence\\Proxy)\.\z~'

AFAIK, Collection is still using the old Common namespace, and we depend on that lib:

dbal/lib/Doctrine/DBAL/Tools/Dumper.php

Line 89 in 72d9081

if ($var instanceof Collection) {

IMO we should add doctrine/collection to at least require-dev (in the case that dependency is considered "optional"), and remove that exclusion rule from the phpstan config file. WDYT?

[greg0ire]

Also, I can't seem to be able to run phpstan locally:

vendor/bin/phpstan analyze
Note: Using configuration file /home/greg/dev/doctrine-dbal/phpstan.neon.dist.
PHP Fatal error:  Class mysqli_result must implement interface Traversable as part of either Iterator or IteratorAggregate in Unknown on line 0
Fatal error: Class mysqli_result must implement interface Traversable as part of either Iterator or IteratorAggregate in Unknown on line 0

[greg0ire]

@morozov I think I fixed everything I could. Please tell me if you would like some commits to move to a PR targeting 2.11.x

[morozov]

@greg0ire judging by the if-statement, it's an optional dependency. So instead of a dev-dependency, we could additionally install it for static analysis if we don't want to ignore those errors.

[greg0ire]

So instead of a dev-dependency, we could additionally install it for static analysis if we don't want to ignore those errors.

Installing it as a dev-dependency would make Psalm happy both for a Doctrine dev and for our CI, I'm not following you 🤔

[morozov]

Would it be possible to convert specific baseline errors into more generic ones that wouldn't require future maintenance? E.g. we don't care of all missing sasql_* functions and constants regardless of their name and the location. The same is for PDO::PGSQL_ATTR_DISABLE_PREPARES.

Ideally, I'd like to get rid of the baseline entirely and turn it into configuration like we do with PHPStan.

[greg0ire]

Also, I was expecting phpstan to fail because of useless ignore rules, but it doesn't 🤔

[morozov]

This is because those are disabled (reportUnmatchedIgnoredErrors: false). They may or may not be useless depending on the environment (e.g. the PDO::PGSQL_ATTR_DISABLE_PREPARES constant may or may not be missing depending on whether pdo_pgsql is installed) and I don't see another way to fix that.

[greg0ire]

Would it be possible to convert specific baseline errors into more generic ones that wouldn't require future maintenance? E.g. we don't care of all missing sasql_* functions and constants regardless of their name and the location. The same is for PDO::PGSQL_ATTR_DISABLE_PREPARES.

Ideally, I'd like to get rid of the baseline entirely and turn it into configuration like we do with PHPStan.

A quick look at the docs does not yield much…

[morozov]

Installing it as a dev-dependency would make Psalm happy both for a Doctrine dev and for our CI, I'm not following you

I just don't want to abuse dev-dependencies. This library is not a requirement for development. I'd rather ignore those errors than add the dependency.

[greg0ire]

I just don't want to abuse dev-dependencies. This library is not a requirement for development. I'd rather ignore those errors than add the dependency.

Ok, I will drop both commits.

[morozov]

A quick look at the docs does not yield much…

E.g. we can suppress all UndefinedFuncion errors in lib/Doctrine/DBAL/Driver/SQLAnywhere/. It's unlikely that we will introduce some other missing functions there than the ones defined by the extension. We may use a similar approach for the rest.

[greg0ire]

I agree but I can't find a way to do this here: https://psalm.dev/docs/running_psalm/configuration

[greg0ire]

For some reason adding PDO to the phpstan stubs did not help. I will revert that.

[morozov]

I think it's something like:

<psalm>
    <issueHandlers>
        <UndefinedFuncion>
            <errorLevel type="suppress">
                <directory name="lib/Doctrine/DBAL/Driver/SQLAnywhere"/>
            </errorLevel>
        </UndefinedFuncion>
    </issueHandlers>
</psalm>

[greg0ire]

Oh right, I didn't use the right keywords in my research (ignore vs suppress)

[greg0ire]

Ideally, I'd like to get rid of the baseline entirely and turn it into configuration like we do with PHPStan.

We may use a similar approach for the rest.

Done, I suppressed errors related to SQLAnywhere.

[morozov]

I think we should get rid of the baseline file as such and move all the suppressions in the config. The entries about PDO::PGSQL_ATTR_DISABLE_PREPARES will need to be reworked in a similar way as the ones about sasql_*. As for the remaining ones, it'd be nice to have a comment for each on why it's being left suppressed (e.g. by design or would require a BC break or a bug in Psalm, etc.). This way we know what we can improve next.

[greg0ire]

Ok, I'll do that.

[morozov]

We do test with the lowest dependencies on Travis, so I'm surprised why just fixing the lock file helps. The build would fail when I tried to update PHPUnit to ^9.1 because of a bug in PHPUnit 9.1.0. Requiring ^9.1.1 solved the problem.

Depending on the exact nature of the issue, should we introduce a requirement or a conflict, or we bump/adjust some other constraint that would eliminate the conflict?

[greg0ire]

We do test with the lowest dependencies on Travis, so I'm surprised why just fixing the lock file helps.

This helps fix a crash I had when running Psalm, and we don't run Psalm after running composer --prefer-lowest

I think we should not use a conflict when it's something that fails in dev/CI, I can add a require-dev constraint if you want me to

[morozov]

Agree. Let's do a temporary require-dev. Additionally, is it worth reporting to Psalm so that they could fix it on their end and we could remove this later? There's either a bug on their end or an incorrect version constraint for PHPParser.

[greg0ire]

Requirement added, bug reported here: vimeo/psalm#3155

[greg0ire]

@morozov 🚢 ?

[morozov]

@greg0ire, 🚢!

[vv12131415]

@morozov I think 2 PRs from phpstorm-stubs that you've mentioned here are released (talking about this JetBrains/phpstorm-stubs#732 and JetBrains/phpstorm-stubs#727 )

[morozov]

Thanks, @vladyslavstartsev. Will be addressed in #4465.