bagisto-code-review

コードレビュー

Bagisto の変更をレビューする際に確認する項目を、重要な指摘が先に届く順に並べています。Pint とテストスイートですでに判定できる機械的な項目は任せ、レビューではそれらでは見抜けない点に注力してください。

ツールで確認できる項目

これらのレビューに時間を使わず、ツールを実行してください。

確認するツール	対象
vendor/bin/pint --test	フォーマット、スペース、インポート順、末尾のカンマ
vendor/bin/pest	テストスイートで検証する動作
php artisan bagisto:translations:check	23 のロケールのいずれかでキーが欠落していないか
Playwright	ブラウザー層

レビューコメントが終了コードで表せる内容なら、コメントを書くのではなく、ツールを実行してください。

マージを妨げる項目

これらが未解決のままでは、変更をマージしないでください。

正確性

• ループ内のクエリ、または Eager Load の不足による N+1 問題。シードデータではコストが見えず、実際のカタログで表面化します。
• 増加し続けるテーブル（商品、注文、顧客）に対する、上限のない ->get()。ページネーションまたはチャンク処理を使用してください。
• 販売者または顧客が所有する行を、所有者で絞り込まずに扱うリポジトリメソッド。
• リクエスト値を埋め込んで組み立てた Raw フラグメント（whereRaw、selectRaw、DB::raw、orderByRaw）。

セキュリティ

bagisto-coding-standards スキルが担当します。差分が認可、レンダリング出力、入力、アップロード、Raw SQL、シークレット、または決済に触れる場合はこのスキルを読み込み、記憶しているチェックリストではなく、記載されたチェックリストに従ってください。実際の指摘の大半を占めるのは、次の 2 点です。

• 認証済み顧客で絞り込まずに、リクエストの ID を使って商品ストアのクエリを実行している。
• DataGrid のクロージャーが e() を使わずに値を HTML 属性へ埋め込んでいる。タグは自動で除去されますが、引用符は除去されません。

アーキテクチャ

• リポジトリの外で DB:: またはモデルクエリを使用している。ただし、DataGrid の prepareQueryBuilder() は許可された例外です。
• Contract、Model、Proxy、Repository を揃えずに新しいモデルを追加している。
• パッケージを bootstrap/providers.php に登録しているが、config/concord.php には登録していない、またはその逆。
• 拡張機能に対応するためにコアファイルを編集している。Marketplace と B2B Suite では、コアの編集はいずれも禁止されています。変更は、バインドされたサブクラス、view_render_event リスナー、または Concord のモデル差し替えで実現してください。
• ユーザー向け文字列を trans() に渡していない。

ブロッカーではないが、指摘する価値のある項目

• 3 回目に現れる重複。 2 回なら偶然ですが、3 回ならヘルパーにまとめるべきです。
• 本体を理解するのにコメントが必要なメソッド。 コードベースではメソッド本体内のコメントは禁止されているため、これは説明を追加するのではなく、名前を付けたメソッドに切り出すサインです。
• 変更対象ファイルにおける Docblock またはメンバーの順序違反。 変更対象ファイルに既存の違反がある場合は、作成者が修正してください。
• 件数またはリスト内の位置を検証するテスト。 共有データベースが増えるとどちらも変動するため、名前で特定したレコードを検証してください。
• 単一レコードの処理ではイベントが発火するが、一括操作の処理では発火しない場合、またはその逆。

レビューの進め方

1. まずテストを読みます。 テストには、作成者が変更の動作として想定している内容が示されています。修正するバグを対象とするテストがない場合は、それが最初の確認事項です。
2. 何が問題を引き起こすか考えます。 主張ごとに、それを覆す入力を探してください。空のコレクション、2 つ目のロケール、ゲスト、デフォルト以外のチャネルなどです。
3. 逆のケースも確認します。 修正を元に戻してもパスする回帰テストでは、何も保証できません。修正が分かりにくい場合は、失敗する様子を作成者に示してもらいましょう。
4. リクエスト、フォームリクエスト、コントローラー、リポジトリ、モデル、ビューを通して、処理の流れを 1 つ最初から最後まで追います。 差分の各ハンクを個別に読むのではなく、流れを追ってください。実際の不具合の多くは、それぞれ単独では問題なく見える 2 つのファイルの境界に潜んでいます。
5. ゲートが実行されたことと、スキップされたゲートを確認します。

指摘の書き方

まず不具合を述べ、次にそれを引き起こす入力を示し、その後に修正方法を述べます。具体的な障害が伴わない指摘は好みの問題であり、そのように明記してください。

CategoryRepository::saveMediaAltText() は translateOrNew($locale) に書き込みますが、$locale には文字列リテラル all が指定されることがあります。カテゴリ作成フォームは locale=all を送信します。MySQL では name と slug が空の行がエラーにならずに作成され、PostgreSQL では NOT NULL 制約違反になります。書き込み前に、このセンチネル値をすべてのロケールに展開してください。

確認できたことと、推測にとどまることを分けてください。「これは N+1 です」と「N+1 かもしれませんが、リレーションは確認していません」は別の主張です。両者を混同すると、そのことを明言するよりも作者に余計な時間を取らせます。

必須のサブスキル： 変更を完了と判断する前に、bagisto-change-verification を使用してください。