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

Utilize composer min-php version to narrow PHP_VERSION_ID #3584

Open
wants to merge 6 commits into
base: 2.0.x
Choose a base branch
from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Oct 26, 2024

idea is to catch errors like Roave/BetterReflection#1406 (comment) in which bogus conditions like if (PHP_VERSION_ID >= 80100) {} have been used in tests.

it's a bug in the linked case, because the project itself only supports "php": "~8.2.0 || ~8.3.2",.
Since CI only is running against in composer.json declared supported versions this error was not spotted before.


rebased version of #2968 for 2.x

@staabm
Copy link
Contributor Author

staabm commented Oct 26, 2024

PHPStan 2.x seems the perfect point in time to re-vive this PR, as it found some dead tests in phpstan-extension repos :) (because of the raised min-php version)

try {
$composerJsonContents = FileReader::read($composerJsonPath);
$composer = Json::decode($composerJsonContents, Json::FORCE_ARRAY);
$requiredVersion = $composer['require']['php'] ?? null;
Copy link
Contributor

Choose a reason for hiding this comment

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

php-64bit should be supported as well - the minimal version should be used of these two if both are present

@staabm staabm marked this pull request as ready for review October 26, 2024 16:42
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@@ -3,3 +3,5 @@ parameters:
bleedingEdge: true
skipCheckGenericClasses!: []
stricterFunctionMap: true
narrowPhpVersionFromComposerJson: true
Copy link
Member

Choose a reason for hiding this comment

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

You have based this PR on 2.0.x. You shouldn't be adding a new flag to bleedingEdge.

Copy link
Contributor Author

@staabm staabm Nov 5, 2024

Choose a reason for hiding this comment

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

my thinking was, that this feature was never part of any stable 1.x release and therefore could need some time to stabilize within bleeding edge.

dropped the toggle now

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Now I can finally review it :) Hopefully it makes 2.0, otherwise we'll have to add the bleeding edge toggles again 😂

@@ -0,0 +1,6 @@
<?php

\PHPStan\Testing\assertType('int<80100, max>', PHP_VERSION_ID);
Copy link
Member

Choose a reason for hiding this comment

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

This could be until 80499 which is the max version allowed in the config. It'd help us prevent more typos and be consistent.

@@ -0,0 +1,6 @@
<?php

\PHPStan\Testing\assertType('int<80100, 90000>', PHP_VERSION_ID);
Copy link
Member

Choose a reason for hiding this comment

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

80499 as max

@@ -41,11 +41,26 @@ public function getVersionId(): int
return $this->versionId;
}

public function getMajor(): int
Copy link
Member

Choose a reason for hiding this comment

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

should say "getMajorVersionId"


public function getMinor(): int
{
return (int) floor(($this->versionId % 10000) / 100);
Copy link
Member

Choose a reason for hiding this comment

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

dtto


public function getPatch(): int
{
return (int) floor($this->versionId % 100);
Copy link
Member

Choose a reason for hiding this comment

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

dtto

return;
}

$this->maxVersion = $this->buildVersion($constraint->getUpperBound()->getVersion());
Copy link
Member

Choose a reason for hiding this comment

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

Don't do this work in the construtor, but lazily in the getters. (Of course it should still be cached in the properties.)

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.

4 participants