ci: PHP 8.5 を matrix に追加し、フォーマット検査を lint job に分ける - #92
Conversation
パイプ演算子 (#76) のテストは 8.4 では RequiresPhp で skip されるため、 CI で一度も実行されていない。 8.5 を matrix に足して実際に走らせる。 php-cs-fixer は 8.4 の 1 回だけ実行する。 3.89 は PHP 8.5 の構文を まだサポートしておらず、 8.5 で走らせると「supports PHP syntax only up to PHP 8.4」と表示して検査せずに exit 0 で終わるため、 通すと偽の成功になる (docker php:8.5.8 で実測)。 フォーマット検査は 1 回で足りる。 paths に .github/workflows/check.yml を追加した。 workflow 自身の変更で CI が走らないと、 matrix が動くことを PR で検証できない。 docker php:8.5-cli (8.5.8) で composer install / phpstan / phpunit が通ることを確認済み ([OK] No errors / OK (104 tests, 109 assertions))。 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @Javakky's task in 2m 17s —— View job レビュー結果
結論: LGTM。 変更は妥当で、意図(8.5 限定テストを CI で回す / 偽の成功を作らない)も達成できています。ブロッカーはありません。以下は改善提案です。 良い点
提案 1(中): php-cs-fixer は matrix 依存のステップではなく独立ジョブにしたい
- name: Run PHP CS Fixer
if: matrix.php == '8.4'いまフォーマット検査は「matrix に 独立した
というメリットがあります。今回の PR スコープを超えるなら別 PR でも構いません。 提案 2(小):
|
if: matrix.php == 8.4 は「matrix に 8.4 という文字列が存在すること」への 暗黙依存だった。 将来 matrix を [8.5, 8.6] に更新すると if が全て偽になり、 エラーも警告も出さずにフォーマット検査だけが消える (CI は緑のまま)。 独立 job にすれば matrix の中身と無関係に必ず 1 回走る。 phpstan.neon と .php-cs-fixer.dist.php も paths に追加した。 check.yml を足した理由 (その変更で CI が走らないと検証できない) が そのまま当てはまる。 level を変えても Check が起動しない状態だった。 Refs #76 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @Javakky's task in 2m 19s —— View job レビュー結果(追加コミット
|
| 前回の提案 | 状態 | 備考 |
|---|---|---|
| 提案1: php-cs-fixer を独立 job に | ✅ 反映 | lint job として分離。php-version: '8.4' を直接指定しており、matrix の中身に依存しなくなった |
提案2: paths に設定ファイル追加 |
✅ 反映 | phpstan.neon / .php-cs-fixer.dist.php を追加。phpstan.neon の paths: [src, tests] と cs-fixer の Finder->in([src, tests]) を確認したので、これで検査対象と paths が一致 |
| 提案3: 8.5 限定テストの実証 | ⏳ 未(想定内) | 後述 |
| 提案4: コメントの解除条件 | ✅ 反映 | 「php-cs-fixer が 8.5 をサポートしたら php-version を上げる (この job を消す必要はない)」まで書かれており、将来 matrix に戻す誤りも防げている |
lint を独立させたことで、matrix から '8.4' が消えてもフォーマット検査が黙って消えるという前回の懸念は解消されました。並列化されて総ウォールクロックも短くなります。
提案 A(小): concurrency がこのワークフローだけ無い
.github/workflows/check.yml:1-14
このリポジトリの他の 5 ワークフロー(agent.yml / agent-review.yml / agent-plan.yml / agent-breakdown.yml / sync_templates.yml)はいずれも concurrency を持っていますが、check.yml にはありません。既存の話ではあるものの、この PR で job 数が 1 → 3 に増えたため、連続 push 時の無駄な並走コストも 3 倍になります。
concurrency:
group: check-${{ github.event.pull_request.number || github.ref }}
cancel-in-progress: true提案 B(小): permissions が未指定
.github/workflows/check.yml:1-14
他ワークフローは permissions を明示していますが、check.yml は未指定のためリポジトリ既定値が使われます。このワークフローは checkout と静的解析しかしないので contents: read で足ります。権限の緩和ではなく最小化なので .claude/rules/30_security.md にも反しません。
permissions:
contents: read提案 C(小): composer install が 3 回走るのでキャッシュが欲しい
.github/workflows/check.yml:36-37, 61-62
分離の代償として composer install が 1 回 → 3 回になりました。composer.lock があるので素直にキャッシュが効きます。
- name: Setup PHP
uses: shivammathur/setup-php@v2
with:
php-version: '8.4'
tools: composer
coverage: none
- name: Install dependencies
uses: ramsey/composer-install@v3ramsey/composer-install@v3 は composer.lock のハッシュでキャッシュキーを作るので、PHP バージョンごとに勝手に分かれます。actions/cache を直書きする場合は キーに matrix.php を含めるのを忘れずに(8.4/8.5 で解決結果が変わりうるため)。
提案 D(nit): lint の php-version と composer.json の下限が連動しない
.github/workflows/check.yml:32
php-version: '8.4' はハードコードなので、将来 composer.json の require.php を >=8.5 に上げると lint job の composer install だけが解決に失敗します。いま直す必要はありませんが、下限を上げるときの TODO として覚えておくとよいです(コメントに一言足しておくのも手です)。
提案 E(小): PR 本文の「注意」が古くなっています
本文では checks → checks (8.4) / checks (8.5) の 2 つと書かれていますが、lint を切り出した結果、実際に登録すべき required status check は 3 つになります。
lintchecks (8.4)checks (8.5)
lint の登録漏れは「フォーマット検査が必須でなくなる」という形で静かに効いてくるので、本文を更新しておくとマージ時の取りこぼしを防げます。なお paths フィルタ付きのため、src/** 等に触れない PR では job が起動せず required が Expected のまま止まる点も従来どおりです。
提案 F(小・再掲): この PR 単体では 8.5 限定テストの実行は証明できない
このブランチの tests/ を再検索しましたが #[RequiresPhp] は依然 0 件で、#76 は未マージです。この PR の CI が緑になっても確認できるのは「8.5 job が完走する」ところまでです。インフラを先に入れる順序自体は正しいので変更不要ですが、#76 マージ後に checks (8.5) のテスト件数が checks (8.4) より多いことを一度確認しておくと、ゲートが効いている実証になります。
確認したこと
composer.lockのplatformは{"php": ">=8.4"}、composer.jsonにconfig.platformのピン留めもなし → 8.5 での依存解決は素直に通ります(PR 本文の実測どおり)。- ロックされた php-cs-fixer は
v3.89.1。コメント中の「3.89 時点」という記述と一致しています。 phpstan.neonにphpVersionの指定がないため、PHPStan は実行中のランタイムを前提に解析します。つまり 8.4 / 8.5 の 2 回実行は冗長ではなく、バージョン依存の解析差分を拾えるので matrix に乗せる価値があります。check.ymlの YAML 構造(lint/checksの 2 job、fail-fast: false、step の入れ子)に破綻はありません。前コミットでchecksjob 内にあった PHPStan/PHPUnit step が正しく新 job 側へ移っており、lint側に取り残されたステップもありません。
なお .github/workflows/ 配下は GitHub App の権限上わたしから直接コミットできないため、上記の修正が必要な場合はローカルまたは各リンクからの適用をお願いします。
ブランチ: javakky/ci-php85 |
概要(What / Why)
CI に PHP 8.5 を追加します。
カリー化 (#76) で入るパイプ演算子のテストは
#[RequiresPhp('>= 8.5')]が付いているため、8.4 だけの CI では一度も実行されません。手元の docker では確認していますが、CI で走らないと退行を検出できません。変更点
フォーマット検査は matrix から切り出して独立した
lintjob にしました。 php-cs-fixer 3.89 は PHP 8.5 の構文をまだサポートしておらず、8.5 で走らせると次を表示して検査せずにexit 0で終わります。通すと偽の成功になりますConfig::setUnsupportedPhpVersionAllowed(true)を足す手もありますが、公式が「unstable / コードが誤って書き換えられる可能性がある」と明言しているので採りませんpathsに.github/workflows/check.yml/phpstan.neon/.php-cs-fixer.dist.phpを追加しました。 workflow 自身の変更で CI が走らないと matrix が動くことをこの PR で検証できませんし、level: 10を変えても Check が起動しない状態でしたfail-fast: falseにしたので、片方が落ちてももう一方の結果が見えますcomposer.jsonのrequire.phpは>=8.4のままです。カリー化した関数は callable を返すだけなので 8.4 で動き、パイプ構文を使うかは利用者側の PHP バージョン次第です。動作確認
docker
php:8.5-cli(PHP 8.5.8) で実際に走らせました。composer install --no-progress --prefer-dist --no-interactionvendor/bin/phpstan analyse -c phpstan.neon[OK] No errorsvendor/bin/phpunit testsOK (104 tests, 109 assertions)vendor/bin/php-cs-fixer fix --dry-run --diffdev 依存は phpstan
^2.1/ phpunit^12.4/ php-cs-fixer^3.89で、8.5 で問題があったのは php-cs-fixer だけです。レビューへの対応
if: matrix.php == '8.4'は「matrix に'8.4'という文字列が存在すること」への暗黙依存で、将来 matrix を['8.5', '8.6']に更新すると if が全て偽になりフォーマット検査だけが警告なく消えます (CI は緑のまま)。独立 job なら matrix の中身と無関係に必ず 1 回走りますpathsに設定ファイルが漏れているphpstan.neonと.php-cs-fixer.dist.phpを追加#[RequiresPhp]は 0 件)。インフラを先に入れる順序として変更しません。#76 マージ後にchecks (8.5)のテスト件数がchecks (8.4)より多いことを確認しますifが消えました。コメントには「php-cs-fixer が 8.5 をサポートしたらphp-versionを上げる (この job を消す必要はない)」と書いてあります注意
required status check の名前が変わります。
checks→lint/checks (8.4)/checks (8.5)。ブランチ保護でchecksを必須にしている場合は設定の更新が必要です。lintは matrix から独立しているので、今後 PHP バージョンを増減しても名前が変わりません。🤖 Generated with Claude Code