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

Run static analysis on PHP 8.1 #9310

Merged
merged 1 commit into from
Jan 1, 2022
Merged

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Jan 1, 2022

This will make it easier to add code that leverages features only
defined since that version of PHP.

This will make it easier to add code that leverages features only
defined since that version of PHP.
@greg0ire greg0ire requested review from derrabus and beberlei January 1, 2022 17:41
@derrabus
Copy link
Member

derrabus commented Jan 1, 2022

I agree that we should analyze the codebase on PHP 8.1. While PHP 7.1 is the lowest supported version, the repository contains features that require a more recent PHP version and we are currently hardly able to run static analysis on those parts.

Just raising the PHP interpreter version is not enough however. We need to toll both tools to apply PHP 8.1 rules. This is where you can do this for PHPStan:

phpVersion: 70100

For Psalm, you need to add the attribute phpVersion="8.1" to the top-level <psalm> element of psalm.xml. If you don't do that, Psalm will infer PHP 7.1 as language level from composer.json.

@derrabus
Copy link
Member

derrabus commented Jan 1, 2022

Moreover, rasining the language level should allow us to drop some hacks from Psalm's configuration:

orm/psalm.xml

Lines 90 to 96 in d30e748

<!-- Workaround for https://github.com/vimeo/psalm/issues/7026 -->
<ReservedWord>
<errorLevel type="suppress">
<directory name="lib"/>
<directory name="tests"/>
</errorLevel>
</ReservedWord>

orm/psalm.xml

Lines 103 to 108 in d30e748

<UndefinedAttributeClass>
<errorLevel type="suppress">
<!-- The class was added in PHP 8.1 -->
<referencedClass name="ReturnTypeWillChange"/>
</errorLevel>
</UndefinedAttributeClass>

@greg0ire
Copy link
Member Author

greg0ire commented Jan 1, 2022

@derrabus I'm not aiming at testing PHP 8.1 features with this PR, I'm aiming at preventing them from making the baseline grow. If we want to test our usage of PHP 8.1 features, while still checking that our code is compatible with our lowest supported version (we want to do that, right?), we will had to rework the static analysis jobs.

Do we still want to have jobs with 7.1 as a target version?

@derrabus
Copy link
Member

derrabus commented Jan 1, 2022

@derrabus I'm not aiming at testing PHP 8.1 features with this PR, I'm aiming at preventing them from making the baseline grow.

Does it really make a difference if we bump the PHP runtime only? Both tools will still assume PHP 7.1 as language level.

Do we still want to have jobs with 7.1 as a target version?

Right now, letting the tools operate PHP 7.1 as language level causes more pain than it solves problems imho. Especially Psalm has a history of reporting funny errors if the dependencies are on a higher language level. I'd suggest to let the test suite cover PHP 7.1 support and let the SA tools operate on PHP 8.1.

@derrabus
Copy link
Member

derrabus commented Jan 1, 2022

If you like, we can merge this PR as-is and I work out a PR for bumping the language level for the SA tools.

Copy link
Member

@beberlei beberlei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree about PHPUnit covering 7.1 is good enough.

@derrabus derrabus merged commit d40f9e5 into doctrine:2.10.x Jan 1, 2022
@derrabus
Copy link
Member

derrabus commented Jan 1, 2022

I've implemented my suggestion as #9314.

derrabus added a commit to derrabus/orm that referenced this pull request Jan 2, 2022
* 2.11.x:
  Run PHP CodeSniffer on PHP 8.1 (doctrine#9317)
  Psalm 4.17.0 (doctrine#9315)
  Run static analysis on PHP 8.1 (doctrine#9310)
@greg0ire greg0ire deleted the sa-on-81 branch January 2, 2022 16:32
derrabus added a commit to derrabus/orm that referenced this pull request Jan 2, 2022
* 2.11.x:
  Run static analysis with language level PHP 8.1 (doctrine#9314)
  Document LockMode enums (doctrine#9319)
  Document PHPUnit mocks with intersection types (doctrine#9318)
  Run PHP CodeSniffer on PHP 8.1 (doctrine#9317)
  Psalm 4.17.0 (doctrine#9315)
  Run static analysis on PHP 8.1 (doctrine#9310)
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