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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: Introduce PHP compatibility check #7844

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Wirone
Copy link
Member

@Wirone Wirone commented Feb 17, 2024

Inspired by this finding, bug introduced in #7616, reproduced here - we have invalid return type in code and none of the tools caught this. I thought this is because we run PHPStan on 8.3 which then assumes it's valid return type, but I modified dev-tools/composer.json, allowed PHP 7.4 and checked on it, but PHPStan still did not report it, even on max level. I believe this is a bug (CC: @ondrejmirtes), but anyway I thought maybe it's a good idea to introduce PHPCompatibility check, which is more focused on differences between PHP versions.

This is a draft and CI will fail, because we need to decide how we want to handle dynamic usages of PHP consts etc. I believe that sniffs should not report these problems if constant is used in if condition, like:

if (\defined('T_READONLY')) {
    // Skip readonly classes for PHP 8.2+
    $acceptTypes[] = T_READONLY;
}

but this is separate problem that should be addressed and fixed in long-term (CC: @jrfnl).

PS. There is 10.x-dev as 9.99.0 constraint because 10.x development version has more checks, but at the same time is not compatible with phpcompatibility/phpcompatibility-symfony which handles Symfony polyfills. We've been using this setup for months in GetResponse and it helped us with migration from 7.4 to 8.2, so I assume it as safe enough 馃檪.

@Wirone Wirone added the topic/ci Github Actions, tooling label Feb 17, 2024
@Wirone Wirone self-assigned this Feb 17, 2024
@Wirone Wirone force-pushed the codito/phpcompatibility branch 2 times, most recently from 787f171 to ad6eaf7 Compare February 17, 2024 20:57
@coveralls
Copy link

coveralls commented Feb 17, 2024

Coverage Status

coverage: 94.769%. remained the same
when pulling 9cdb005 on Wirone:codito/phpcompatibility
into ea01071 on PHP-CS-Fixer:master.

@ondrejmirtes
Copy link

Please don't cc me on other repositories. If you want, you can isolate the bug, reproduce it and report it properly on PHPStan's issue tracker.

composer.json Show resolved Hide resolved
Comment on lines +24 to +25
<file>../../src/</file>
<file>../../tests/</file>
Copy link
Member

Choose a reason for hiding this comment

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

I would include the main file too:

Suggested change
<file>../../src/</file>
<file>../../tests/</file>
<file>../../src/</file>
<file>../../tests/</file>
<file>../../php-cs-fixer</file>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/ci Github Actions, tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants