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

Support PHPStan 2.0 #144

Closed
wants to merge 13 commits into from
Closed

Conversation

zonuexe
Copy link
Contributor

@zonuexe zonuexe commented Nov 13, 2024

All packages that are not compatible with PHPStan 2.0 have been temporarily removed.

refs rectorphp/rector#8815

@zonuexe zonuexe marked this pull request as draft November 13, 2024 07:42
@zonuexe
Copy link
Contributor Author

zonuexe commented Nov 13, 2024

I thought I needed a RuleErrorBuilderRector that works with PHPStan 1.x 😜

@zonuexe zonuexe force-pushed the migrate/phpstan-2.0 branch from 9dce5c1 to c823f54 Compare November 13, 2024 10:52
@zonuexe
Copy link
Contributor Author

zonuexe commented Nov 13, 2024

All tests pass in my environment.

スクリーンショット 2024-11-13 19 52 21

@zonuexe zonuexe marked this pull request as ready for review November 13, 2024 10:54
Comment on lines 43 to 45
/**
* @param FileNode $node
* @return string[]
*/
public function processNode(Node $node, Scope $scope): array
Copy link
Member

@TomasVotruba TomasVotruba Nov 13, 2024

Choose a reason for hiding this comment

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

The @param should remain, so IDE can fill the methods on $node

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, that doesn't make sense for an IDE that supports generics, but let's restore it.

Copy link
Member

@TomasVotruba TomasVotruba Nov 13, 2024

Choose a reason for hiding this comment

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

My lattest PHPStorm still doesn't. What IDE do you use?


Without param type:

Screenshot from 2024-11-13 12-16-33

With param type:

Screenshot from 2024-11-13 12-17-43

Choose a reason for hiding this comment

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

This is why: #144 (review)

@zonuexe zonuexe force-pushed the migrate/phpstan-2.0 branch 2 times, most recently from e66aa45 to 7986458 Compare November 13, 2024 11:13
@zonuexe
Copy link
Contributor Author

zonuexe commented Nov 13, 2024

I have to hurry up and catch a long-distance bus now, but I've finished what I can do.

use Symplify\RuleDocGenerator\Contract\DocumentedRuleInterface;
use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;

/**
* @implements <FileNode>

Choose a reason for hiding this comment

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

The autocompletion does not work because this should say @implements Rule<FileNode>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@@ -17,7 +17,7 @@
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;

/**
* @implements <FileNode>
* @implements Node<FileNode>

Choose a reason for hiding this comment

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

Shouldn't this be Rule<FileNode>

Choose a reason for hiding this comment

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

Yes. Running PHPStan on the code will report that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

"symplify/rule-doc-generator": "^12.2",
"phpunit/phpunit": "^10.5",
"symfony/framework-bundle": "6.1.*",
"rector/rector": "^1.2.3",
Copy link
Member

Choose a reason for hiding this comment

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

could you use

        "rector/rector": "dev-main as 1.2.10",

to test rector for CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied it to f83aafa

@@ -22,6 +22,7 @@ jobs:

-
name: 'Rector'
if: ${{ false }} # Temporarily disabled as it is not compatible with PHPStan 2.0
Copy link
Member

Choose a reason for hiding this comment

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

Could you revert this, rector should run now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted a6be6a1 d8fb908

@zonuexe zonuexe requested a review from samsonasik November 27, 2024 05:57
@@ -69,7 +67,7 @@ public function processNode(Node $node, Scope $scope): array
return [];
}

return [self::ERROR_MESSAGE];
return [RuleErrorBuilder::message(self::ERROR_MESSAGE)->build()];
Copy link
Member

Choose a reason for hiding this comment

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

add identifier here as well

@@ -53,7 +50,7 @@ public function processNode(Node $node, Scope $scope): array
return [];
}

return [self::ERROR_MESSAGE];
return [RuleErrorBuilder::message(self::ERROR_MESSAGE)->build()];
Copy link
Member

Choose a reason for hiding this comment

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

add identifier here as well

@@ -132,7 +131,7 @@ private function processClassNameAndShort(ClassReflection $classReflection): arr
}

$errorMessage = sprintf(self::ERROR_MESSAGE, $expectedSuffix);
return [$errorMessage];
return [RuleErrorBuilder::message($errorMessage)->build()];
Copy link
Member

Choose a reason for hiding this comment

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

add identifier here as well

@@ -58,7 +56,7 @@ public function processNode(Node $node, Scope $scope): array
return [];
}

return [self::ERROR_MESSAGE];
return [RuleErrorBuilder::message(self::ERROR_MESSAGE)->build()];
Copy link
Member

Choose a reason for hiding this comment

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

add identifier here as well

@@ -74,6 +75,6 @@ public function processNode(Node $node, Scope $scope): array
return [];
}

return [self::ERROR_MESSAGE];
return [RuleErrorBuilder::message(self::ERROR_MESSAGE)->build()];
Copy link
Member

Choose a reason for hiding this comment

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

add identifier here as well

@@ -77,6 +75,6 @@ public function processNode(Node $node, Scope $scope): array
return [];
}

return [self::ERROR_MESSAGE];
return [RuleErrorBuilder::message(self::ERROR_MESSAGE)->build()];
Copy link
Member

Choose a reason for hiding this comment

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

add identifier here as well, basically, add unique identifier on message error

@zonuexe zonuexe marked this pull request as draft November 27, 2024 07:49
@mitelg
Copy link

mitelg commented Nov 29, 2024

hey @zonuexe

thanks for your current work! 💪 we are currently waiting for this extension to be compatible with PHPStan 2, so I want to offer my help 🙂 let me know, if I can somehow support here

@samsonasik samsonasik mentioned this pull request Dec 12, 2024
@samsonasik
Copy link
Member

I cherry-picked your commits at new PR to continue work on it:

@staabm
Copy link
Contributor

staabm commented Dec 13, 2024

this PR can be closed I guess?

@samsonasik
Copy link
Member

Yes 👍

@samsonasik samsonasik closed this Dec 13, 2024
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.

7 participants