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.

Sign up for GitHub

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

Jump to bottom

Since annotations outside phpstub should not infer php version #10769

Merged
merged 2 commits into from Mar 2, 2024
Merged

Since annotations outside phpstub should not infer php version #10769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d751c20
fix unrelated Config bug not honoring PHP composer versions 8.2 + 8.3
kkmuffme Mar 2, 2024
a1848a1
@since annotations should only infer PHP version in .phpstub files or…
kkmuffme Mar 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 16 additions & 2 deletions src/Psalm/Config.php
View file
Open in desktop
Expand Up @@ -2781,8 +2781,22 @@ public function getPHPVersionFromComposerJson(): ?string
$version_parser = new VersionParser();

$constraint = $version_parser->parseConstraints($php_version);

foreach (['5.4', '5.5', '5.6', '7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1'] as $candidate) {
$php_versions = [
'5.4',
'5.5',
'5.6',
'7.0',
'7.1',
'7.2',
'7.3',
'7.4',
'8.0',
'8.1',
'8.2',
'8.3',
];
Comment on lines +2784 to +2797
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note to self: we keep forgetting to update PHP version numbers in different places. This means we need to centralize PHP version handling in a single place.


foreach ($php_versions as $candidate) {
if ($constraint->matches(new Constraint('<=', "$candidate.0.0-dev"))
|| $constraint->matches(new Constraint('<=', "$candidate.999"))
) {
Expand Down
17 changes: 12 additions & 5 deletions src/Psalm/Internal/PhpVisitor/Reflector/FunctionLikeDocblockParser.php
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@
use function explode;
use function implode;
use function in_array;
use function pathinfo;
use function preg_last_error_msg;
use function preg_match;
use function preg_replace;
Expand All @@ -37,6 +38,8 @@
use function substr_count;
use function trim;

use const PATHINFO_EXTENSION;

/**
* @internal
*/
Expand Down Expand Up @@ -417,11 +420,15 @@ public static function parse(

if (isset($parsed_docblock->tags['since'])) {
$since = trim(reset($parsed_docblock->tags['since']));
if (preg_match('/^[4578]\.\d(\.\d+)?$/', $since)) {
$since_parts = explode('.', $since);

$info->since_php_major_version = (int)$since_parts[0];
$info->since_php_minor_version = (int)$since_parts[1];
// only for phpstub files or @since 8.0.0 PHP
// since @since is commonly used with the project version, not the PHP version
// https://docs.phpdoc.org/3.0/guide/references/phpdoc/tags/since.html
// https://github.com/vimeo/psalm/issues/10761
if (preg_match('/^([4578])\.(\d)(\.\d+)?(\s+PHP)?$/i', $since, $since_match)
&& isset($since_match[1])&& isset($since_match[2])
&& (!empty($since_match[4]) || pathinfo($code_location->file_name, PATHINFO_EXTENSION) === 'phpstub')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'll do for now, but we should probably limit this to only the stubs Psalm itself ships in future. When I renamed the stubs I didn't want .phpstub to be anything specific to Psalm, but hoped to establish a naming convention for any stubs across the ecosystem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think this is a positive, but unintended side effect - we use stubs for a couple PHP extensions where we use the @SInCE annotation for the PHP version this feature become available and named all those .phpstub too.

I think it would be ideal if .phpstub would refer to stubs that are related to PHP core and/or PHP extensions - since this is basically what happens now already, with only psalm internally using this extension mostly and this would allow using @SInCE in those like it's currently used.

Therefore this won't require a change in the future, just some documentation about the .phpstub file extension somewhere?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Many plugins use .phpstub extension, and not for built-in classes / extensions: https://github.com/search?q=org%3Apsalm%20phpstub&type=code

$info->since_php_major_version = (int)$since_match[1];
$info->since_php_minor_version = (int)$since_match[2];
}
}

Expand Down
10 changes: 10 additions & 0 deletions tests/AnnotationTest.php
View file
Open in desktop
Expand Up @@ -1384,6 +1384,16 @@ class Bar {}
class Foo {}',
'assertions' => [],
],
'sinceTagNonPhpVersion' => [
'code' => '<?php
class Foo {
/**
* @since 8.9.9
*/
public function bar() : void {
}
};',
],
];
}

Expand Down