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

Update phpstan-baseline.neon in order to remove ignored error #2346

Merged
merged 1 commit into from
Nov 29, 2021

Conversation

phansys
Copy link
Collaborator

@phansys phansys commented Nov 27, 2021

@phansys phansys marked this pull request as ready for review November 27, 2021 00:25
@phansys phansys force-pushed the postgresql_platform branch from 8cdd61c to 9c020bb Compare November 27, 2021 01:14
@mbabker
Copy link
Contributor

mbabker commented Nov 27, 2021

If you're bumping the minimum to 3.2, you should also remove the type check that only exists for 3.1. https://github.com/doctrine-extensions/DoctrineExtensions/blob/main/src/Translatable/Query/TreeWalker/TranslationWalker.php#L470-L475

@phansys
Copy link
Collaborator Author

phansys commented Nov 27, 2021

If you're bumping the minimum to 3.2, you should also remove the type check that only exists for 3.1. https://github.com/doctrine-extensions/DoctrineExtensions/blob/main/src/Translatable/Query/TreeWalker/TranslationWalker.php#L470-L475

Please, note that the bump only affects the dev requirements. The users consuming this package as a dependency are still able to install doctrine/dbal 3.1.

@mbabker
Copy link
Contributor

mbabker commented Nov 27, 2021

In that case, you can remove the error from the baseline as PHPStan in the current setup is always intended to run against the latest stable dependencies, no need to change the Composer manifest.

@phansys
Copy link
Collaborator Author

phansys commented Nov 27, 2021

In that case, you can remove the error from the baseline as PHPStan in the current setup is always intended to run against the latest stable dependencies, no need to change the Composer manifest.

IMO, if the dev constraint does not guarantee the absence of the error, we can not trust on the behavior, as some dependency could block doctrine/dbal at < 3.2 in future builds.
That's why I prefer to declare a dev dependency that ensures the proper behavior.

@mbabker
Copy link
Contributor

mbabker commented Nov 27, 2021

That's why I prefer to declare a dev dependency that ensures the proper behavior.

Except this change, on its own, isn't doing anything to ensure proper behavior. It's "fixing" the PHPStan analysis when run on an environment where DBAL 3.2 is installed, but if PHPStan were run on an environment where DBAL 3.1 is installed, that specific error would still exist. And if PHPStan were run on a DBAL 2.13 environment, there would be another family of errors entirely.

  • The composer.json manifest declares that the development environment requires ^2.13.1 || ^3.2 while allowing downstream users to install ^2.13.1 || ^3.1, effectively allowing an unsupported version to be used by downstream users
  • The GitHub Actions configuration explicitly sets a build to ^3.1, which will override the declared development dependency anyway for that particular build

Because PHPStan is only being run with the latest stable versions of all dependencies (as of this writing, that would be Doctrine's Cache 1.12, DBAL 3.2, ORM 2.10, and MongoDB ODM 2.2, as well as Symfony 5.3 components, PHPUnit 9.5, among other dependencies), it is only detecting errors with that combination of dependencies. It is not detecting errors when installed with DBAL 2.13, or Symfony 4.4 components, or PHPUnit 8.5, and the baseline will most likely change again when MongoDB ODM 2.3 is released which unblocks Doctrine Cache 2.0, or when Symfony 5.4 and 6.0 release and the environment is allowed to start pulling those versions.

@phansys
Copy link
Collaborator Author

phansys commented Nov 27, 2021

Except this change, on its own, isn't doing anything to ensure proper behavior. It's "fixing" the PHPStan analysis when run on an environment where DBAL 3.2 is installed, but if PHPStan were run on an environment where DBAL 3.1 is installed, that specific error would still exist. And if PHPStan were run on a DBAL 2.13 environment, there would be another family of errors entirely.

The composer.json manifest declares that the development environment requires ^2.13.1 || ^3.2 while allowing downstream users to install ^2.13.1 || ^3.1, effectively allowing an unsupported version to be used by downstream users
The GitHub Actions configuration explicitly sets a build to ^3.1, which will override the declared development dependency anyway for that particular build
Because PHPStan is only being run with the latest stable versions of all dependencies (as of this writing, that would be Doctrine's Cache 1.12, DBAL 3.2, ORM 2.10, and MongoDB ODM 2.2, as well as Symfony 5.3 components, PHPUnit 9.5, among other dependencies), it is only detecting errors with that combination of dependencies. It is not detecting errors when installed with DBAL 2.13, or Symfony 4.4 components, or PHPUnit 8.5, and the baseline will most likely change again when MongoDB ODM 2.3 is released which unblocks Doctrine Cache 2.0, or when Symfony 5.4 and 6.0 release and the environment is allowed to start pulling those versions.

That's true. Are you interested in updating the build matrix in order to run PHPStan against all the lowest and highest dependencies?

@mbabker
Copy link
Contributor

mbabker commented Nov 27, 2021

Yeah, I can try to come up with something next week.

@franmomu
Copy link
Collaborator

Shall we just drop support for doctrine/dbal 3.1 (with conflict)? doctrine/orm is about to do it doctrine/orm#9203

@mbabker
Copy link
Contributor

mbabker commented Nov 28, 2021

It honestly wouldn't hurt. Since there isn't a tagged release yet with full DBAL 3 support, it's not like someone gets stuck with older dependencies.

@phansys phansys force-pushed the postgresql_platform branch 2 times, most recently from dd41e2a to 240c05f Compare November 28, 2021 23:59
@phansys
Copy link
Collaborator Author

phansys commented Nov 29, 2021

Shall we just drop support for doctrine/dbal 3.1 (with conflict)? doctrine/orm is about to do it doctrine/orm#9203

I added the conflict.

"doctrine/dbal": "<2.13.1 || ^3.0 <3.2",

@franmomu
Copy link
Collaborator

I added the conflict.

"doctrine/dbal": "<2.13.1 || ^3.0 <3.2",

#2346 (comment) can be done now I guess.

@phansys phansys force-pushed the postgresql_platform branch 2 times, most recently from e7e08ba to 521a633 Compare November 29, 2021 08:48
@franmomu
Copy link
Collaborator

sorry to bother again, just realised that we should also change to be in sync

- deps: "highest"
php-version: "8.0"
dbal-version: "^3.1"
and then LGTM

@phansys phansys force-pushed the postgresql_platform branch from 521a633 to 20fb9af Compare November 29, 2021 09:02
@phansys
Copy link
Collaborator Author

phansys commented Nov 29, 2021

sorry to bother again, just realised that we should also change to be in sync

Good catch. Thank you.

@phansys phansys merged commit fd386a0 into doctrine-extensions:main Nov 29, 2021
@phansys phansys deleted the postgresql_platform branch November 29, 2021 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants