Skip to content

enh: Add phpmd, phpstan and phpcs - #11

Open
kofimokome wants to merge 2 commits into
devfrom
enhancements
Open

kofimokome wants to merge 2 commits into
devfrom
enhancements

Conversation

@kofimokome

Copy link
Copy Markdown
Collaborator

No description provided.

@sourceant

sourceant Bot commented Feb 21, 2026

Copy link
Copy Markdown

Code Review Summary

This PR introduces a robust CI/CD pipeline for linting, static analysis (PHPStan), mess detection (PHPMD), and code style (PHPCS/Pint). It also updates various dependencies in composer.json and adjusts the test matrix to include newer PHP versions.

🚀 Key Improvements

  • Added PHPStan, PHPMD, and PHPCS for comprehensive code quality checks.
  • Configured GitHub Actions to run these checks on PRs to main and dev.
  • Updated project to target PHP 8.3+.

💡 Minor Suggestions

  • The phpunit.xml test suffix change should be reverted to prevent non-test files from being executed.
  • Group trait usage in Event classes for better readability.

🚨 Critical Issues

  • Invalid dependency on Workbench namespaces in core src/ files. This will break the package when installed as a vendor dependency.
  • Excessive suppression of PHPStan errors using @phpstan-ignore-next-line instead of fixing type definitions.

@sourceant sourceant Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review complete. See the overview comment for a summary.

echo round(\$coverage, 2);
")
echo "Code coverage: $COVERAGE%"
MIN_COVERAGE=59.1

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The minimum coverage threshold is set to a very specific value (59.1). It is recommended to round this to an integer or use a more standard threshold (e.g., 60 or 80) to avoid failures on minor, non-logical line shifts.

Suggested change
MIN_COVERAGE=59.1
MIN_COVERAGE=60

Comment thread src/Models/Workspace.php
use Illuminate\Database\Eloquent\Relations\MorphTo;
use Illuminate\Database\Eloquent\SoftDeletes;

use Workbench\App\Models\User;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A model within the src/ directory (package core) should not depend on Workbench\App\Models\User. This creates a circular dependency or assumes the consumer has the same namespace. Use the config or a contract for the User model.

Suggested change
use Workbench\App\Models\User;
use Illuminate\Foundation\Auth\User;

@sourceant sourceant Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review complete. See the overview comment for a summary.

php-lint-sniffs:
name: PHP (Sniffs)
runs-on: ubuntu-latest
# timeout-minutes: 20

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Uncomment the timeout or remove it if not needed. It's better to have a reasonable timeout (e.g., 10 minutes) to prevent hung actions from consuming billing minutes.

Suggested change
# timeout-minutes: 20
timeout-minutes: 10

Comment thread composer.json
"vendor/bin/phpmd src text phpmd.xml"
],
"phpstan": "phpstan analyse src --memory-limit=2048M --level=5",
"phpcs": "phpcbf src",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The phpcs script is configured to use phpcbf, which is a fixer, not a checker. This is confusing as the script name implies checking. Usually, phpcs should be for checking and phpcbf for fixing.

Suggested change
"phpcs": "phpcbf src",
"phpcs:fix": "phpcbf src",

'invited_by' => $inv->invitedBy?->name,
'expires_at' => $inv->expires_at,
'created_at' => $inv->created_at,
/**

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Instead of using @phpstan-ignore-next-line, you can improve the type safety of the collection. Using map(fn (WorkspaceInvitation $inv) => ...) inside a Laravel collection after a query should work correctly if PHPDocs are defined on the model or via generics in the get() method.

Suggested change
/**
->map(function (WorkspaceInvitation $inv) {

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.

1 participant