Utilize composer min-php version to narrow PHP_VERSION_ID #2968

staabm · 2024-03-13T14:06:04Z

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.

ondrejmirtes

Do the upper bound too - so that we know ^8.1 cannot be 9.

staabm · 2024-03-13T14:57:12Z

@ondrejmirtes what do you think about all these

Comparison operation "<" between int<80100, max> and 80100 is always false.

errors? should we ignore them via phpstan.neon or should we add e.g. a feature flag, so we can disable the composer based min/max-php version?

ondrejmirtes · 2024-03-13T15:07:13Z

It's not really generally usable. In PHPStan and many other apps, the constraint in composer.json does nor say anything about production environment, or can be outdated.

It could be:

On in bleeding edge
But at the same time there should be a way to turn it off, or make it configurable manually, like reading the range from phpstan.neon instead of composer.json. phpVersion behaves similarly, when it's null, we try to detect it.

staabm · 2024-03-13T15:40:13Z

tests/PHPStan/Rules/Arrays/NonexistentOffsetInArrayDimFetchRuleTest.php

@@ -671,7 +671,7 @@ public function testBug8068(): void

 	public function testBug6243(): void
 	{
-		if (PHP_VERSION_ID < 704000) {


found a bug in a pre-existing test -> #3254

staabm · 2024-03-13T15:42:05Z

build/ignore-by-php-version.neon.php

+$config['parameters']['minPhpVersion'] = 70100;
+$config['parameters']['maxPhpVersion'] = 90000;


I put the min/max php version here, so we have a fixed upper/lower bound in phpstan-src, but all projects per default still use composer.json based auto-php-version bounds. I think even projects using bleeding-edge would benefit from automatic php version bounds.

the case that projects need to opt-out from this check will be rare I guess - but is possible via the new phpstan.neon parameters by defining custom versions

staabm · 2024-03-13T16:06:25Z

I think the errors we see in CI make sense. not sure what we can/should do about them.

it make sense to check for php-versions smaller then the composer.json declared supported ones, to be super safe.
maybe I should use composer-min-version - 1 as a lower bound?

edit: did just that with 0d398ab

phpstan-bot · 2024-03-13T16:06:52Z

This pull request has been marked as ready for review.

src/Php/ComposerPhpVersionFactory.php

phpstan-bot · 2024-03-18T10:03:29Z

This pull request has been marked as ready for review.

staabm · 2024-03-22T09:03:00Z

@ondrejmirtes keeping this one up2date is a bit cumbersome, because of the added composer dependency (and the conflicts I need to resolve on every other dependency update).

would be great if you could find the time to move it forward.

ondrejmirtes

Some thoughts about this PR:

There are more constants related to PHP versions: https://www.php.net/manual/en/reserved.constants.php
To be really useful, this should also influence version_compare: https://www.php.net/manual/en/function.version-compare.php A dynamic return type extension would be sufficient.
I'd like phpVersion to be either: null, a single number, or a structure with min/max

If it's null, resolve it to a single number in PHPStan 1.x (according to config.platform.php in composer.json), and to min/max structure with bleeding edge (according to require.php in composer.json).

If it's a single number, keep today's behaviour. Meaning PHP_VERSION_ID is not going to be limited.

If it's min/max, clamp PHP_VERSION_ID and other constants to be an IntegerRangeType.

Also we should have some kind of warning if the values don't make sense. max should be >= min.

staabm · 2024-03-24T08:08:20Z

thanks for the feedback.

If it's null, resolve it to a single number in PHPStan 1.x (according to config.platform.php in composer.json), and to min/max structure with bleeding edge (according to require.php in composer.json).

If it's a single number, keep today's behaviour. Meaning PHP_VERSION_ID is not going to be limited.

If it's min/max, clamp PHP_VERSION_ID and other constants to be an IntegerRangeType.

this means in case of a configured min/max version, we don't have a concrete version anymore in PhpVersion.
so all the support-methods don't work like they do today. since PhpVersion is tagged @api I cannot change it to return a Trinary..

phpstan-src/src/Php/PhpVersion.php

Lines 29 to 32 in 3e2eed9

    
           public function supportsNullCoalesceAssign(): bool 
        
           { 
        
           	return $this->versionId >= 70400; 
        
           }

ondrejmirtes · 2024-03-24T08:15:59Z

That's right. Changing all these methods to a TrinaryLogic would be a costly refactoring that would eventually lead to ReflectionClass::hasClass() also returning TrinaryLogic. I'd like that but not at this point.

Please try using the min value as the origin for these methods to decide whether to return true or false.

ondrejmirtes · 2024-03-24T08:17:09Z

If that's not going to work for some reason then I'm not interested in this PR at this point, sorry. When investing in an area we need to do something that makes sense, not just a minimal amount of work that would change the interpretation of a single constant.

staabm · 2024-03-24T08:42:39Z

Sure, lets see how it goes.

Alternatively/Additionally I will look into a narrowed PhpVersion based on scope like we brainstormed in phpstan/phpstan#4212 (reply in thread)

src/Php/ComposerPhpVersionFactory.php

ondrejmirtes · 2024-05-31T09:28:46Z

src/Php/ComposerPhpVersionFactory.php

+	)
+	{
+		if (is_int($phpVersion)) {
+			return;


I think this if can be just removed.

re-added this line, which fixed the build.

errors before the fix can be seen in e.g. https://github.com/phpstan/phpstan-src/actions/runs/9331440117/job/25686106757?pr=2968

src/Php/ComposerPhpVersionFactory.php

src/Php/PhpVersionFactoryFactory.php

staabm · 2024-06-01T15:53:14Z

All feedback should be addressed.

Yeah, please remove the -1.

~~because of this change we are now seeing a lot of errors in the test-suite, e.g.:~~

Comparison operation "<" between int<80100, 90000> and 80100 is always false.

~~should these be ignored globally? any other idea on how to handle these?~~

resolved with edde1b6, see #2968 (comment)

random idea: use a separate error-identifier for these errors?

phpstan-bot · 2024-06-01T15:53:25Z

This pull request has been marked as ready for review.

mvorisek · 2024-06-02T07:52:49Z

src/Php/ComposerPhpVersionFactory.php

+		return $composerPhpVersion;
+	}
+
+	private function buildVersion(string $minVersion): ?PhpVersion


Suggested change

private function buildVersion(string $minVersion): ?PhpVersion

private function buildVersion(string $version): ?PhpVersion

mvorisek · 2024-06-02T07:53:42Z

src/Php/PhpVersion.php

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

+	public function getMajor(): int
+	{
+		return (int) floor($this->versionId / 10000);


intdiv would be better

mvorisek · 2024-06-02T08:06:17Z

src/Php/ComposerPhpVersionFactory.php

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


phpstan/phpstan#9256 should be probably implemented in this PR as well and tested.

That's unrelated and can be done later.

fix Update e2e-tests.yml add more tests fix rename class added config parameters fix test more tests test version parameters Update e2e-tests.yml prevent 'Comparison operation "<" between int<70205, 90000> and 70205 is always false.' strict errors downgrade composer/semver for brianium/paratest:4.0.0 compat should fix the php 7.2 CI build incorporate feedback spaces to tabs support more constants Narrow `PHP_VERSION` in version_compare() another test fix

staabm · 2024-10-26T09:30:31Z

rebased for 2.x in #3584

ondrejmirtes requested changes Mar 13, 2024

View reviewed changes

clxmstaab force-pushed the min-php branch 2 times, most recently from 8c64205 to 6dbb9d1 Compare March 13, 2024 15:39

staabm commented Mar 13, 2024

View reviewed changes

clxmstaab force-pushed the min-php branch from 0674c80 to 10e3621 Compare March 13, 2024 16:03

staabm marked this pull request as ready for review March 13, 2024 16:06

staabm mentioned this pull request Mar 13, 2024

Added test-case for deprecated phpstorm stub with patch-version Roave/BetterReflection#1406

Closed

Seldaek reviewed Mar 14, 2024

View reviewed changes

src/Php/ComposerPhpVersionFactory.php Outdated Show resolved Hide resolved

staabm force-pushed the min-php branch 2 times, most recently from 982b705 to eb63e2a Compare March 18, 2024 08:09

clxmstaab force-pushed the min-php branch 2 times, most recently from e7c38fc to 0a2f82f Compare March 18, 2024 09:33

staabm marked this pull request as draft March 18, 2024 09:39

clxmstaab force-pushed the min-php branch from 0a2f82f to 68c78e0 Compare March 18, 2024 09:54

staabm marked this pull request as ready for review March 18, 2024 10:03

staabm force-pushed the min-php branch from 68c78e0 to aa74d55 Compare March 19, 2024 19:51

clxmstaab force-pushed the min-php branch from aa74d55 to 5c6a0cd Compare March 22, 2024 09:00

ondrejmirtes requested changes Mar 22, 2024

View reviewed changes

staabm force-pushed the min-php branch from 193f88d to 7ad1406 Compare March 24, 2024 10:13

ondrejmirtes requested changes May 31, 2024

View reviewed changes

staabm marked this pull request as draft June 1, 2024 15:12

staabm force-pushed the min-php branch from f0ce9f9 to c2bd605 Compare June 1, 2024 15:37

staabm marked this pull request as ready for review June 1, 2024 15:53

mvorisek reviewed Jun 2, 2024

View reviewed changes

staabm mentioned this pull request Jul 20, 2024

Fix PHP_VERSION_ID comparison in test #3254

Merged

staabm force-pushed the min-php branch from edde1b6 to 66c7ac1 Compare July 20, 2024 07:18

staabm marked this pull request as draft July 20, 2024 21:17

staabm and others added 13 commits July 20, 2024 23:20

cleanup

0aa5fd2

simplify

c38dc9e

validate php-version earlier

2a29c80

fix expectations

4f90ac0

fix e2e tests

dcd31dc

try to fix build

3f680e4

Update composer.lock

8b13baa

update composer/semver

fd369d0

try fix php 7.2 build

49f01cd

Discard changes to .github/workflows/static-analysis.yml

fa2949b

Discard changes to .github/workflows/tests.yml

e1e38ca

Discard changes to composer.lock

2a593f7

staabm force-pushed the min-php branch from 247a7dc to 2a593f7 Compare July 20, 2024 21:20

staabm closed this Oct 26, 2024

staabm deleted the min-php branch October 26, 2024 09:30

staabm mentioned this pull request Oct 26, 2024

Utilize composer min-php version to narrow PHP_VERSION_ID #3584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Utilize composer min-php version to narrow PHP_VERSION_ID #2968

Utilize composer min-php version to narrow PHP_VERSION_ID #2968

staabm commented Mar 13, 2024 •

edited

Loading

ondrejmirtes left a comment

staabm commented Mar 13, 2024 •

edited

Loading

ondrejmirtes commented Mar 13, 2024

staabm Mar 13, 2024 •

edited

Loading

staabm Mar 13, 2024 •

edited

Loading

staabm commented Mar 13, 2024 •

edited

Loading

phpstan-bot commented Mar 13, 2024

phpstan-bot commented Mar 18, 2024

staabm commented Mar 22, 2024

ondrejmirtes left a comment

staabm commented Mar 24, 2024 •

edited

Loading

ondrejmirtes commented Mar 24, 2024

ondrejmirtes commented Mar 24, 2024

staabm commented Mar 24, 2024

ondrejmirtes May 31, 2024

staabm Jun 2, 2024 •

edited

Loading

staabm commented Jun 1, 2024 •

edited

Loading

phpstan-bot commented Jun 1, 2024

mvorisek Jun 2, 2024

mvorisek Jun 2, 2024

mvorisek Jun 2, 2024

ondrejmirtes Jun 5, 2024

staabm commented Oct 26, 2024

		$config['parameters']['minPhpVersion'] = 70100;
		$config['parameters']['maxPhpVersion'] = 90000;

	private function buildVersion(string $minVersion): ?PhpVersion
	private function buildVersion(string $version): ?PhpVersion

Utilize composer min-php version to narrow PHP_VERSION_ID #2968

Utilize composer min-php version to narrow PHP_VERSION_ID #2968

Conversation

staabm commented Mar 13, 2024 • edited Loading

ondrejmirtes left a comment

Choose a reason for hiding this comment

staabm commented Mar 13, 2024 • edited Loading

ondrejmirtes commented Mar 13, 2024

staabm Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

staabm Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

staabm commented Mar 13, 2024 • edited Loading

phpstan-bot commented Mar 13, 2024

phpstan-bot commented Mar 18, 2024

staabm commented Mar 22, 2024

ondrejmirtes left a comment

Choose a reason for hiding this comment

staabm commented Mar 24, 2024 • edited Loading

ondrejmirtes commented Mar 24, 2024

ondrejmirtes commented Mar 24, 2024

staabm commented Mar 24, 2024

ondrejmirtes May 31, 2024

Choose a reason for hiding this comment

staabm Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

staabm commented Jun 1, 2024 • edited Loading

phpstan-bot commented Jun 1, 2024

mvorisek Jun 2, 2024

Choose a reason for hiding this comment

mvorisek Jun 2, 2024

Choose a reason for hiding this comment

mvorisek Jun 2, 2024

Choose a reason for hiding this comment

ondrejmirtes Jun 5, 2024

Choose a reason for hiding this comment

staabm commented Oct 26, 2024

staabm commented Mar 13, 2024 •

edited

Loading

staabm commented Mar 13, 2024 •

edited

Loading

staabm Mar 13, 2024 •

edited

Loading

staabm Mar 13, 2024 •

edited

Loading

staabm commented Mar 13, 2024 •

edited

Loading

staabm commented Mar 24, 2024 •

edited

Loading

staabm Jun 2, 2024 •

edited

Loading

staabm commented Jun 1, 2024 •

edited

Loading