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

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.

Seldaek · 2024-03-14T13:05:30Z

src/Php/ComposerPhpVersionFactory.php

+			// allow checks with < below lower bound to prevent errors like
+			// 'Comparison operation "<" between int<70205, 90000> and 70205 is always false.'
+			if ($minVersion !== null) {
+				$this->minVersion = new PhpVersion($minVersion->getVersionId() - 1);


This subtraction seems very dodgy to me. If I get it correctly this is to fix warnings where if (PHP_VERSION_ID < 70100) where I have a require of php >=7.1 for example. But why should it not warn? Because clearly per the requirement the code should not be allowed to run with a version lower than 70100.

Re-reading your comment above:

it make sense to check for php-versions smaller then the composer.json declared supported ones, to be super safe.

Yes it may make sense in some cases, but then you want to explicitly override PHPStan and add this in the baseline IMO. In most cases I'd think you will want PHPStan to tell you that this check can be removed when you bump your PHP version requirement, which suddenly would not work anymore here due to the -1.

relevant examples from previous CI failures are

https://github.com/phpstan/phpstan-src/actions/runs/8267804971/job/22619260364?pr=2968 (in composer)

https://github.com/phpstan/phpstan-src/actions/runs/8267804971/job/22619308636?pr=2968 (in PrestaShop)

I am fine with leaving this error and let the projects put them into a baseline, but my thinking was, that it might be pretty common that code tries to detect whether it is invoked from a not supported php-version (so whether PHP_VERSION is below the lower limit of composer.json requirements)

Right yeah those two examples for Composer are definitely things I'd be ok ignoring if it gets me visibility on other things I forgot to delete.

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)

phpstan-bot · 2024-03-24T21:18:17Z

This pull request has been marked as ready for review.

staabm · 2024-03-24T21:19:27Z

I think all the feedback is adressed

mvorisek · 2024-03-31T11:29:36Z

e2e/composer-min-max-version/test.php

+\PHPStan\Testing\assertType('int<80099, 80299>', PHP_VERSION_ID);
+\PHPStan\Testing\assertType('8', PHP_MAJOR_VERSION);
+\PHPStan\Testing\assertType('int<0, 2>', PHP_MINOR_VERSION);


Suggested change

\PHPStan\Testing\assertType('int<80099, 80299>', PHP_VERSION_ID);

\PHPStan\Testing\assertType('8', PHP_MAJOR_VERSION);

\PHPStan\Testing\assertType('int<0, 2>', PHP_MINOR_VERSION);

\PHPStan\Testing\assertType('int<80100, 80299>', PHP_VERSION_ID);

\PHPStan\Testing\assertType('8', PHP_MAJOR_VERSION);

\PHPStan\Testing\assertType('int<1, 2>', PHP_MINOR_VERSION);

AFAK PHP_VERSION_ID starts with the major/minor even if run on alpha release

PHP_MINOR_VERSION should be narrowed if PHP_MAJOR_VERSION is one constant number

See #2968 (comment)

I saw it. But I do not support it. The reason is composer already check this during runtime and if the check is really, really wanted, the user should ignore that error instead.

… is always false.' strict errors

should fix the php 7.2 CI build

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

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

staabm marked this pull request as draft March 24, 2024 10:20

staabm marked this pull request as ready for review March 24, 2024 21:18

clxmstaab force-pushed the min-php branch 2 times, most recently from 1279561 to 764bb38 Compare March 27, 2024 08:25

mvorisek reviewed Mar 31, 2024

View reviewed changes

clxmstaab force-pushed the min-php branch from 764bb38 to b6a26ab Compare April 5, 2024 08:54

staabm and others added 19 commits April 6, 2024 11:04

Utilize composer min-php version to narrow PHP_VERSION_ID

9dd5d00

fix

d19e18b

Update e2e-tests.yml

914c39d

add more tests

5bf2853

fix

282b350

rename class

1081cf1

added config parameters

9cba0cf

fix test

739cedf

more tests

a21f6bb

test version parameters

a0a0181

Update e2e-tests.yml

5c454b8

prevent 'Comparison operation "<" between int<70205, 90000> and 70205…

fdcb1d2

… is always false.' strict errors

downgrade composer/semver for brianium/paratest:4.0.0 compat

aad4e8b

should fix the php 7.2 CI build

incorporate feedback

819de2f

spaces to tabs

7cb54fe

support more constants

6ebf7ca

Narrow PHP_VERSION in version_compare()

f0a6d44

another test

e6307b2

fix

0c9f369

staabm force-pushed the min-php branch from b6a26ab to 0c9f369 Compare April 6, 2024 09:04

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

ondrejmirtes left a comment

staabm commented Mar 13, 2024 •

edited

ondrejmirtes commented Mar 13, 2024

staabm Mar 13, 2024

staabm Mar 13, 2024 •

edited

staabm commented Mar 13, 2024 •

edited

phpstan-bot commented Mar 13, 2024

Seldaek Mar 14, 2024

Seldaek Mar 14, 2024

staabm Mar 14, 2024

Seldaek Mar 14, 2024

phpstan-bot commented Mar 18, 2024

staabm commented Mar 22, 2024

ondrejmirtes left a comment

staabm commented Mar 24, 2024 •

edited

ondrejmirtes commented Mar 24, 2024

ondrejmirtes commented Mar 24, 2024

staabm commented Mar 24, 2024

phpstan-bot commented Mar 24, 2024

staabm commented Mar 24, 2024

mvorisek Mar 31, 2024

staabm Mar 31, 2024

mvorisek Mar 31, 2024

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

Utilize composer min-php version to narrow PHP_VERSION_ID #2968

Are you sure you want to change the base?

Utilize composer min-php version to narrow PHP_VERSION_ID #2968

Conversation

staabm commented Mar 13, 2024 • edited

ondrejmirtes left a comment

Choose a reason for hiding this comment

staabm commented Mar 13, 2024 • edited

ondrejmirtes commented Mar 13, 2024

Choose a reason for hiding this comment

staabm Mar 13, 2024 • edited

Choose a reason for hiding this comment

staabm commented Mar 13, 2024 • edited

phpstan-bot commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

ondrejmirtes commented Mar 24, 2024

ondrejmirtes commented Mar 24, 2024

staabm commented Mar 24, 2024

phpstan-bot commented Mar 24, 2024

staabm commented Mar 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm commented Mar 13, 2024 •

edited

staabm commented Mar 13, 2024 •

edited

staabm Mar 13, 2024 •

edited

staabm commented Mar 13, 2024 •

edited

staabm commented Mar 24, 2024 •

edited