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

Test against PHP 8.4 #378

Open
wants to merge 2 commits into
base: 5.x
Choose a base branch
from
Open

Test against PHP 8.4 #378

wants to merge 2 commits into from

Conversation

Nek-
Copy link
Contributor

@Nek- Nek- commented Feb 20, 2025

I already use the http client with PHP 8.4. But I figured out that the devkit was limited to PHP 8.3. Let's update things!

Outch. It has been a battle to update psalm configuration. I added some new rules to avoid many many MANY useless casts.

@Nek- Nek- force-pushed the php8-4-on-ci branch 2 times, most recently from 5d8b88a to de8cda6 Compare February 20, 2025 23:29
Fix psalm config

PSAML has problems with goto analyze. It leads to wrong issues:
vimeo/psalm#11327
@@ -48,8 +48,8 @@
"phpunit/phpunit": "^9",
"amphp/http-server": "^3",
"kelunik/link-header-rfc5988": "^1",
"psalm/phar": "~5.23",
"laminas/laminas-diactoros": "^2.3"
"psalm/phar": "*",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The version will be 6 but different regarding the PHP version.

Copy link
Contributor

@danog danog Feb 21, 2025

Choose a reason for hiding this comment

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

You should at least require the latest ^6.8.5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you read the message I wrote just above?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, and your assumption is wrong: you should always require the latest available version of Psalm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The composer.json of psalm/phar does not allow this.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does, just require psalm 6.8.5

@@ -4,9 +4,12 @@
phpVersion="8.1"
resolveFromConfigFile="true"
rememberPropertyAssignmentsAfterCall="false"
ensureOverrideAttribute="false"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please set this to true, override attributes are recommended adn compatible with all PHP versions if the symfony/polyfill-php83 packages required

@@ -4,9 +4,12 @@
phpVersion="8.1"
resolveFromConfigFile="true"
rememberPropertyAssignmentsAfterCall="false"
ensureOverrideAttribute="false"
findUnusedCode="false"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please keep this on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a library, everything is unused code!

Copy link
Contributor

Choose a reason for hiding this comment

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

No, public code must be marked with @api, the remaining code is private, unused code.

Copy link
Member

@bwoebi bwoebi Feb 21, 2025

Choose a reason for hiding this comment

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

@danog I disagree - in a library everything is api, unless it's marked @internal, or is a private method.
This is littering the code with unnecessary annotations.
This brings no value as is, as I'm going to be only interested in whether my internal code is dead.

Copy link
Member

Choose a reason for hiding this comment

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

Please keep this config off unless psalm offers a way to "default-mark" everything non-internal & non-private as API.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with default-marking everything non-internal and non-private as API is with non-library codebases (that have no public surface apart from API controllers and other entry points).

Some not so clean library codebases have the same problem as well, where internal details are needlessly exposed as public and not marked as internal.

Explicitly marking public API with @api solves both issues.

Copy link
Member

Choose a reason for hiding this comment

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

@danog I think it would be a good thing to make that configurable in psalm. libraryAssumeApi="true" or such.

@@ -59,5 +70,11 @@
<directory name="src" />
</errorLevel>
</RiskyTruthyFalsyComparison>

<InvalidReturnType>
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't suppress this at the file level

</errorLevel>
</ImplicitToStringCast>

<InvalidOperand>
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't suppress this at a project level

Copy link
Contributor

Choose a reason for hiding this comment

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

vimeo/psalm#11329 will fix what you're (incorrectly) trying to fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not only that. The code often do some float * int, which is perfectly fine, btw string . float is also ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you have issues with that, just set strictBinaryOperands="false", do not suppress the issue as it also covers valid errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need lower versions of psalm for php versions compatibility. It would not work anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Psalm is capable of running scans for versions down to php 5.4 on php 8.4, just pass the correct PHP version using the --php-version parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Psalm scans should always run on PHP 8.4.4 or above to avoid platform bugs, to target lower versions of PHP simply use the --php-version parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants