Skip to content

chore: add phpcs, phpstan, psalm, phpunit, captainhook + CI - #21

Merged
tomas862 merged 3 commits into
mainfrom
chore/add-lint-tooling
Apr 24, 2026
Merged

chore: add phpcs, phpstan, psalm, phpunit, captainhook + CI#21
tomas862 merged 3 commits into
mainfrom
chore/add-lint-tooling

Conversation

@tomas862

Copy link
Copy Markdown
Contributor

Summary

First pass of static-analysis and coding-standard tooling for bradsearch/magento-extension. Nothing about the runtime modules changes — this PR only adds config and dev dependencies so contributors and CI run the same checks. A sibling client-specific package (bradsearch/magento-verkter-price-adapter) is being extracted in parallel, and it's easier to keep rules consistent across both repos if we introduce them here at the same time.

What this adds

  • phpcs (Magento2 standard) via composer phpcs / composer phpcbf. Two sniffs scoped down for existing code:
    • Magento2.Security.Superglobal excluded on SearchGraphQl/Plugin/CatalogGraphQl/Model/Resolver/{Aggregations,Products}.php (raw \$_GET used for GraphQL nested filter handling — flagged for refactor).
    • Magento2.PHP.FinalImplementation.FoundFinal excluded on SearchGraphQl/Model/Data/* (immutable response value objects — intentional final).
    • Every new file is held to the full Magento2 standard.
  • phpstan level 5 with a 60-error baseline (phpstan-baseline.neon) of pre-existing violations. New code is strict; baselined entries should trend to zero. Runs with --memory-limit=1G. phpVersion: 80300 to avoid PHP 8.4 "implicitly nullable" deprecations on a codebase that targets 7.4–8.3.
  • psalm errorLevel 4 with a baseline (psalm-baseline.xml). Pinned to ^6.0 <6.5 because 6.5+ requires PHP 8.4.3.
  • phpunit 9.6 or 10.0 wired for ProductFeatures/Test and SearchGraphQl/Test. Not part of the default composer check and not gated in CI in this PR — existing tests require the Magento Docker container and have pre-existing failures unrelated to this change. Runnable locally via composer test.
  • captainhook + captainhook/plugin-composer. Hooks install automatically on composer install:
    • pre-commit → phpcbf + phpcs + phpstan against staged PHP files
    • pre-push → composer psalm
  • GitHub Actions (.github/workflows/ci.yml). Matrix over PHP 7.4 / 8.1 / 8.2 / 8.3 runs composer phpcs, composer phpstan, composer psalm on every PR and push to main.
  • composer.json: added require-dev, scripts, config.allow-plugins, and a repositories entry for repo.magento.com.
  • README.md gained a Development section. .gitignore widened.

Test plan

  • composer install in a clean checkout → captainhook reports 8 hooks installed, phpcs auto-registers Magento2 standard via post-install-cmd.
  • composer phpcs exits 0.
  • composer phpstan exits 0 (pre-existing errors pinned in phpstan-baseline.neon).
  • composer psalm exits 0 (pre-existing errors pinned in psalm-baseline.xml).
  • composer check (= phpcs + phpstan + psalm) exits 0.
  • Make a deliberately mis-indented PHP change and git commitphpcbf reformats it, phpcs/phpstan block bad code; commit succeeds with fixed file re-staged.
  • git push on a branch → pre-push runs composer psalm and completes.
  • Open a dummy PR and confirm GitHub Actions matrix (PHP 7.4 / 8.1 / 8.2 / 8.3) passes on all four jobs.

Follow-ups

  • Grind phpstan-baseline.neon and psalm-baseline.xml toward empty. The biggest clusters:
    • CollectionFactory / PlaceholderFactory / SyncStateFactory — Magento generated classes psalm can't see; fix with proper @method PHPDoc on the factory var or explicit type declarations.
    • Several Magento\Framework\GraphQl\Query\Resolver\ContextInterface::getExtensionAttributes() calls — needs type narrowing.
    • StoreInterface::getBaseUrl() / getCurrentCurrencyCode() — interface doesn't declare these; narrow to \Magento\Store\Model\Store at the call site.
  • Wire composer test into CI once the existing unit test failures are addressed (separate PR — they predate this one).

🤖 Generated with Claude Code

First pass of static-analysis and coding-standard tooling for the
bradsearch/magento-extension package. Nothing about the runtime modules
changes — this commit only adds config and dev dependencies so contributors
and CI run the same checks.

What it adds:

- **phpcs** (Magento2 standard): `composer phpcs` / `composer phpcbf`.
  Two sniffs scoped-down for existing code (`Magento2.Security.Superglobal`
  on Aggregations.php + Products.php where raw $_GET is used for GraphQL
  nested filters; `Magento2.PHP.FinalImplementation.FoundFinal` on the
  three SearchGraphQl/Model/Data value objects). Every new file is held to
  the full Magento2 standard.

- **phpstan** at level 5 with a 60-error baseline of pre-existing
  violations (`phpstan-baseline.neon`). New code is analyzed strictly;
  baselined entries should drop to zero over time. Runs with 1G memory
  limit. `phpVersion: 80300` so PHP 8.4 "implicitly nullable" deprecations
  don't fire while codebase targets 7.4–8.3.

- **psalm** at errorLevel 4 with a baseline (`psalm-baseline.xml`). Same
  philosophy as phpstan. Pinned to ^6.0 <6.5 — 6.5+ requires PHP 8.4.3.

- **phpunit** 9.6 || 10.0 for unit tests. Wired for ProductFeatures/Test
  and SearchGraphQl/Test. Not in the default `composer check` run (and
  skipped in CI) because existing tests depend on the Magento Docker
  container and have pre-existing failures unrelated to this PR. Run
  locally with `composer test`.

- **captainhook** + captainhook/plugin-composer. Hooks install
  automatically on `composer install`. pre-commit runs phpcbf + phpcs +
  phpstan against staged PHP files. pre-push runs `composer psalm`.

- **GitHub Actions** (.github/workflows/ci.yml). Matrix over PHP
  7.4 / 8.1 / 8.2 / 8.3 runs `composer phpcs`, `composer phpstan`,
  `composer psalm` on every PR and push to main.

- Added `require-dev`, `scripts`, `config.allow-plugins`, and a
  `repositories` entry for repo.magento.com to composer.json.
- Documented the new commands in README.md.
- Widened .gitignore.

Why now:

A sibling client-specific package (bradsearch/magento-verkter-price-adapter)
is being extracted. It is easier to keep quality rules consistent across
both repos if we introduce them here at the same time. A follow-up will
start grinding the phpstan / psalm baselines toward empty.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request introduces a comprehensive development environment for a Magento extension, adding configurations for PHP_CodeSniffer, PHPStan, Psalm, and PHPUnit, along with CaptainHook for git hooks. The review feedback identifies several documentation discrepancies in the README regarding tool levels and hook behaviors, and suggests removing redundant manual PHPCS path registration in the composer configuration.

Comment thread README.md
composer install # installs captainhook pre-commit + pre-push hooks
composer phpcbf # auto-fix Magento2 coding standard
composer phpcs # Magento2 coding standard check
composer phpstan # static analysis at level 6

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The README indicates that PHPStan runs at level 6, but the configuration in phpstan.neon.dist is set to level 5. This discrepancy should be resolved to provide accurate documentation for contributors.

Suggested change
composer phpstan # static analysis at level 6
composer phpstan # static analysis at level 5

Comment thread README.md
composer phpstan # static analysis at level 6
composer psalm # static analysis at error level 4
composer test # phpunit unit tests
composer check # all of the above

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The comment for composer check says "all of the above", which implies it includes composer test. However, the check script defined in composer.json only includes phpcs, phpstan, and psalm. The documentation should be clarified to avoid confusion.

Suggested change
composer check # all of the above
composer check # phpcs + phpstan + psalm

Comment thread README.md
```

Pre-commit hooks run `phpcbf` + `phpcs` + `phpstan` on staged PHP files.
Pre-push runs `psalm` + full test suite. CI (GitHub Actions) runs the same

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The README states that the pre-push hook runs the full test suite, but captainhook.json only configures composer psalm for the pre-push stage. Given that tests are currently excluded from the default checks due to environment requirements (as noted in the PR description), the documentation should be updated to reflect the actual hook behavior.

Suggested change
Pre-push runs `psalm` + full test suite. CI (GitHub Actions) runs the same
Pre-push runs `psalm`. CI (GitHub Actions) runs the same

Comment thread composer.json
Comment on lines +62 to +64
"post-install-cmd": "@register-phpcs-paths",
"post-update-cmd": "@register-phpcs-paths",
"register-phpcs-paths": "phpcs --config-set installed_paths vendor/magento/magento-coding-standard,vendor/phpcsstandards/phpcsutils,vendor/magento/php-compatibility-fork",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The manual registration of PHPCS paths via @register-phpcs-paths is redundant because dealerdirect/phpcodesniffer-composer-installer is included in require-dev (line 34). This plugin automatically handles the discovery and registration of all installed coding standards during the composer installation process. Removing this manual step simplifies the configuration and avoids potential issues with global PHPCS settings in shared environments.

tomas862 and others added 2 commits April 24, 2026 14:17
composer install fails in CI without Magento marketplace credentials.
Add a pre-install step that writes http-basic auth to composer's global
config using MAGENTO_PUBLIC_KEY and MAGENTO_PRIVATE_KEY secrets.

Fails fast with a clear message if either secret is missing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Running GitHub Actions CI for a Magento extension requires Magento
marketplace credentials (repo.magento.com auth) stored as repo secrets,
which is more setup than we need right now. Rely on captainhook
pre-commit (phpcbf + phpcs) and pre-push (composer psalm) hooks for
enforcement during local development.

Also drops phpstan entirely:
- removed from composer.json require-dev
- removed from composer scripts (phpstan, check)
- removed from captainhook pre-commit actions
- deleted phpstan.neon.dist and phpstan-baseline.neon

phpcs, psalm, and phpunit stay available locally via composer scripts.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@tomas862
tomas862 merged commit a3b61d6 into main Apr 24, 2026
@tomas862
tomas862 deleted the chore/add-lint-tooling branch April 24, 2026 11:24
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