improvement: deprecation ゲート有効化＋未使用の非推奨 public API 削除 (#6933) - #6937
Conversation
コアに呼び出し元がなく、CSV 出力にも載らず、DB スキーマ変更も伴わない 未使用の @deprecated public API を削除する。仕様（挙動・出力・画面）は不変。 - OrderItem::getTaxRuleId() / setTaxRuleId()（呼び出し元皆無・tax_rule_id カラムは残置） - Product::isEnable() ＋ ProductClass::isEnable()（計算メソッド・相互呼び出しのみ） - Cart::getLock() / setLock() ＋ ORM 非マッピングの $lock プロパティ - transChoice()（trans() の別名・呼び出し元皆無） - TaxProcessor::getTaxDisplayType()（デッドな protected メソッド） - CartService::$cart（未初期化・未使用プロパティ）＋未使用 import Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
呼び出し元がテストのみの @deprecated public API を削除する。 仕様（挙動・出力・画面）は不変。 削除: - Order::getTotalPrice()（getPaymentTotal() の別名。E_USER_DEPRECATED の self 発火源） - CacheUtil::clear()（代替は clearCache()。専用テスト CacheUtilTest ごと削除） - CustomerStatus::NONACTIVE / ACTIVE（現役定数 PROVISIONAL / REGULAR と同値の別名） テストの扱い: - 専用テストは削除: CacheUtilTest、OrderTest::testGetTotalPrice - 道具として使うテストは等価 API へ付け替え: - EditControllerTest: getTotalPrice() → getPaymentTotal() - EntryControllerTest / CustomerRepository(GetQueryBuilderBySearchData)Test / Generator: CustomerStatus::NONACTIVE/ACTIVE → PROVISIONAL/REGULAR Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Symfony のメジャー追従を楽にするため、自コードが Symfony/Doctrine の 非推奨 API を直接呼んだ場合に CI を落とす。weak（発火してもCIを落とさない） から max[direct]=0 へ引き上げる。 - phpunit.xml.dist: 実 PHPUnit CI（unit-test.yml / coverage.yml の phpunit ジョブ）はこの設定を継承するため、ここが強制化の本体。 - coverage.yml: 無効化済み Codeception ジョブの値も整合のため更新。 max[self]=0 は併用しない（StringUtil の E_USER_DEPRECATED は本 Issue の 対象外で、併用すると CI が落ちるため）。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPHPUnitの非推奨検出を強化し、非推奨APIと顧客ステータス定数を整理しました。PHP 8.4・8.5に対応する入力処理とテスト設定も更新しました。 Changes非推奨検出ゲートと実行手順
エンティティAPIと顧客ステータスの整理
サービス・ユーティリティAPIの削除
PHP互換性と入力処理の更新
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/coverage.yml (1)
259-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win無効化された Codeception ジョブではなく、実行されるジョブに設定してください。
この設定は
if: falseの Codeception ジョブ内にあるため、現在のCIでは評価されません。さらに有効化してもcontinue-on-error: trueにより非推奨検出がCIゲートになりません。CI全体のゲートを意図する場合は、PHPUnitジョブへ移動するか、この重複設定を削除してください。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/coverage.yml around lines 259 - 260, Move the SYMFONY_DEPRECATIONS_HELPER setting from the disabled Codeception job to the active PHPUnit job, and remove continue-on-error there so deprecation failures gate CI; if the setting is already present in PHPUnit, delete this duplicate instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Eccube/Tests/Fixture/Generator.php`:
- Line 912:
createCustomers()のDocBlockに記載されたデフォルトステータスをACTIVEからREGULARへ更新し、実装のCustomerStatus::REGULARと説明を一致させてください。
---
Nitpick comments:
In @.github/workflows/coverage.yml:
- Around line 259-260: Move the SYMFONY_DEPRECATIONS_HELPER setting from the
disabled Codeception job to the active PHPUnit job, and remove continue-on-error
there so deprecation failures gate CI; if the setting is already present in
PHPUnit, delete this duplicate instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28c17751-aec3-494b-90c9-37192139dbb3
📒 Files selected for processing (19)
.github/workflows/coverage.ymlphpunit.xml.distsrc/Eccube/Entity/Cart.phpsrc/Eccube/Entity/Master/CustomerStatus.phpsrc/Eccube/Entity/Order.phpsrc/Eccube/Entity/OrderItem.phpsrc/Eccube/Entity/Product.phpsrc/Eccube/Entity/ProductClass.phpsrc/Eccube/Resource/functions/trans.phpsrc/Eccube/Service/CartService.phpsrc/Eccube/Service/PurchaseFlow/Processor/TaxProcessor.phpsrc/Eccube/Util/CacheUtil.phptests/Eccube/Tests/Entity/OrderTest.phptests/Eccube/Tests/Fixture/Generator.phptests/Eccube/Tests/Repository/CustomerRepositoryGetQueryBuilderBySearchDataTest.phptests/Eccube/Tests/Repository/CustomerRepositoryTest.phptests/Eccube/Tests/Util/CacheUtilTest.phptests/Eccube/Tests/Web/Admin/Order/EditControllerTest.phptests/Eccube/Tests/Web/EntryControllerTest.php
💤 Files with no reviewable changes (12)
- src/Eccube/Entity/ProductClass.php
- tests/Eccube/Tests/Util/CacheUtilTest.php
- src/Eccube/Entity/OrderItem.php
- src/Eccube/Resource/functions/trans.php
- src/Eccube/Entity/Master/CustomerStatus.php
- src/Eccube/Entity/Order.php
- src/Eccube/Entity/Product.php
- src/Eccube/Entity/Cart.php
- tests/Eccube/Tests/Entity/OrderTest.php
- src/Eccube/Service/CartService.php
- src/Eccube/Util/CacheUtil.php
- src/Eccube/Service/PurchaseFlow/Processor/TaxProcessor.php
nanasess
left a comment
nanasess
left a comment
There was a problem hiding this comment.
Part 1（deprecation ゲート）と、削除された public API のうち以下は src/・app/・PHPUnit に呼び出し元が無いことを確認できました。妥当な削除です。
OrderItem::getTaxRuleId/setTaxRuleId/Product::isEnable/ProductClass::isEnable/Cart::getLock/setLock/transChoice()/TaxProcessor::getTaxDisplayType()(protected) /CartService::$cart/Order::getTotalPrice()/CacheUtil::clear()
一方で CustomerStatus::NONACTIVE / ACTIVE の削除は「呼び出し元皆無」ではありません。削除監査が src/ と PHPUnit のみを対象にしており、現行アクティブな Playwright E2E ハーネスを見落としています。修正が必要なため change request とします（詳細はインラインコメント参照）。
🔴 要修正: e2e/setup-fixtures.php が削除された定数を参照
e2e/setup-fixtures.php:50, 56, 140がCustomerStatus::ACTIVE/NONACTIVEを使用。- このファイルは
e2e/global-setup.ts:16がphp e2e/setup-fixtures.phpとして 全 E2E 実行前の globalSetup で必ず実行します（e2e-test.yml/e2e-test-throttling.yml）。
影響:
- 定数削除後、line 50 で
Error: Undefined constant Eccube\Entity\Master\CustomerStatus::ACTIVEが fatal になり、フィクスチャ生成が中断。 global-setup.tsの try/catch が例外を握り潰すため E2E setup は即死しませんが、line 50 以降のテスト会員・仮会員・商品・受注が生成されず、これらに依存する spec（例:front-throttling.spec.tsは globalSetup 生成のテスト用顧客に依存）が失敗します。
修正案（本 PR の Generator.php で既に適用済みの置換と同一）:
- $Status = $entityManager->getRepository(CustomerStatus::class)->find(CustomerStatus::ACTIVE);
+ $Status = $entityManager->getRepository(CustomerStatus::class)->find(CustomerStatus::REGULAR);
...
- $nonActiveStatus = $entityManager->getRepository(CustomerStatus::class)->find(CustomerStatus::NONACTIVE);
+ $nonActiveStatus = $entityManager->getRepository(CustomerStatus::class)->find(CustomerStatus::PROVISIONAL);（ACTIVE=2=REGULAR、NONACTIVE=1=PROVISIONAL で同値です）
補足: codeception/acceptance/_bootstrap.php:158,160 も同定数を参照
CI 無効（coverage.yml if: false）のレガシーですが、削除後は壊れた dead reference として残ります。ついでに同置換するか、少なくとも本 PR の削除判定の対象外である旨を明記すると安全です。
補足: CI（PHPUnit / Playwright）が本 PR で未実行
本 PR は base 4.4 と CONFLICTING 状態のため、main.yml（pull_request トリガ）が一時マージコミットを生成できず起動していません。結果、max[direct]=0 ゲート・E2E ともに CI 上で未検証です。4.4 へ rebase してコンフリクト解消 → main.yml 起動 → 上記 E2E 修正込みで緑を確認する流れを推奨します。
| @@ -28,20 +28,6 @@ | |||
| #[ORM\Cache(usage: 'NONSTRICT_READ_WRITE')] | |||
| class CustomerStatus extends AbstractMasterEntity | |||
There was a problem hiding this comment.
この定数削除（NONACTIVE/ACTIVE）は e2e/setup-fixtures.php:50,56,140 で参照されており、現行の Playwright E2E globalSetup（e2e/global-setup.ts:16 が実行）が Undefined constant で fatal になります。e2e/setup-fixtures.php を ACTIVE→REGULAR / NONACTIVE→PROVISIONAL に修正してください（Generator.php と同一の置換）。レガシーの codeception/acceptance/_bootstrap.php:158,160 も同定数を参照しています。
There was a problem hiding this comment.
ご指摘ありがとうございます。対応済みです。
cb7b6a0d50…e2e/setup-fixtures.phpとcodeception/acceptance/_bootstrap.phpの参照を同値の現行定数へ置換しました（ACTIVE(2) →REGULAR、NONACTIVE(1) →PROVISIONAL）。削除定数の監査対象をsrc/と PHPUnit に絞っていたのが漏れの原因で、ご指摘の通り Playwright の globalSetup（e2e/global-setup.ts）と VAddy スキャンが通る現役の経路でした。3ee103e5f6… 上記で 1 箇所（返品申請テスト会員の生成、e2e/setup-fixtures.php:223）に置換漏れが残っており、そこで Fatal になって以降のフィクスチャが未生成となり admin-order / admin-refund-request / front-refund-request の 3 スイートが落ちていたため追加で修正しました。df658f2fb9… 同種の見落としを早く検知できるよう、フィクスチャ生成の失敗理由をログへ出すようにしました。
現時点で CustomerStatus::ACTIVE / NONACTIVE の参照は残っていません（git grep で確認済み）。E2E を含む CI も全て green です。
…on-gate # Conflicts: # .github/workflows/coverage.yml # src/Eccube/Entity/Cart.php # src/Eccube/Entity/Master/CustomerStatus.php # src/Eccube/Entity/Order.php # src/Eccube/Entity/OrderItem.php # src/Eccube/Entity/Product.php # src/Eccube/Entity/ProductClass.php # src/Eccube/Util/CacheUtil.php
レビュー指摘対応。CustomerStatus::NONACTIVE/ACTIVE の削除監査が src/ と PHPUnit のみを対象にしており、以下の現役 E2E ハーネスを見落としていた。 - e2e/setup-fixtures.php … Playwright の globalSetup (e2e/global-setup.ts) が 全 E2E 実行前に必ず実行する。定数削除後は Undefined constant で フィクスチャ生成が中断し、テスト会員・商品・受注が生成されない。 - codeception/acceptance/_bootstrap.php … VAddy スキャン (.github/workflows/vaddy/scan/action.yml) が codecept run -g vaddy を 実行し、@group vaddy の Cest が本ファイルの createCustomer() を使う。 いずれも同値の現行定数へ置換する (ACTIVE=2=REGULAR / NONACTIVE=1=PROVISIONAL) ため挙動は変わらない。あわせて Generator::createCustomers() の DocBlock の デフォルト値表記を実装 (REGULAR) と一致させる。 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
deprecation ゲートを有効化する前提として、テスト実行時に発生していた 非推奨警告 11 件を解消する。いずれも挙動は変えない。 src (3 件・null の受け方): - SafeTextmailEscaperExtension: Twig は null の変数もそのまま渡すため エスケーパの引数を ?string で受け、str_replace へ '' を渡す - PluginService::deleteDirs: install()/update() は例外発生時点で未設定の null を渡す仕様のため、null をスキップし phpdoc を string|null に修正 - InvalidItemException: parent::__construct() へ null を渡さない tests (7 件・動的プロパティ生成): - MasterdataTypeTest::$form / LoginHistoryRepository…AdminTest の $Member1・$LoginHistory1-3 / MailControllerTest の $Member・$MailHistories を宣言する。EccubeTestCase::cleanUpProperties() が tearDown で全プロパティに null を代入するため、いずれも nullable で宣言する vendor 由来 (1 件): - HTML パートを持たないメールに対する assertEmailHtmlBodyNotContains() を assertNull($Message->getHtmlBody()) へ変更（MailServiceTest 3 箇所・ WithdrawControllerTest 1 箇所）。元の書き方は symfony/mime 内で str_contains(null, …) の非推奨警告を出すうえ、HTML パートが存在しないため 常に成立する空振りアサーションだった Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…換える (#6933) 当初 SYMFONY_DEPRECATIONS_HELPER を weak から max[direct]=0 に引き上げたが、 この設定は PHPUnit 11 環境では機能しないことが判明したため、PHPUnit 11 が 持つネイティブのゲートへ置き換える。 SYMFONY_DEPRECATIONS_HELPER が効かない理由: - 値を読んで DeprecationErrorHandler を登録するのは symfony/phpunit-bridge の bootstrap.php だが、同ファイルは冒頭で `if (class_exists(PHPUnit\Metadata\Metadata::class)) { return; }` により PHPUnit 10 以上では早期 return する - PHPUnit 10+ 用の SymfonyExtension は ClockMock / DnsMock のみを登録し、 DeprecationErrorHandler は登録しない - CI (unit-test.yml / coverage.yml) は vendor/bin/phpunit を実行するため、 bridge 経由の simple-phpunit も通らない - 実測: 非推奨警告を発生させるテストを vendor/bin/phpunit で実行しても bridge のサマリは出力されず、警告も集計されない 置き換え内容: - SYMFONY_DEPRECATIONS_HELPER を削除（読まれない設定のため） - failOnDeprecation="true" … 非推奨 API の呼び出しで CI を失敗させる - displayDetailsOnTestsThatTriggerDeprecations="true" … 発生箇所を CI ログに出す 前提となる既存の非推奨警告 11 件は先行コミットで解消済みで、フルスイート (2905 テスト) で Deprecations 0 を確認している。 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- AGENTS.md / phpunit Skill: 「PHPUnit 11（symfony/phpunit-bridge 経由）」を 「vendor/bin/phpunit を直接実行」に訂正し、bridge の SYMFONY_DEPRECATIONS_HELPER が PHPUnit 10 以上で無効である旨を明記 - AGENTS.md / phpunit Skill / contributing Skill: 実在しない `bin/phpunit` を `vendor/bin/phpunit` に訂正（bin/ には console と template_jp.php のみ） - phpunit Skill「よくある間違い」に 3 項追記 - 未宣言プロパティへの代入は failOnDeprecation で CI red になる - テストのプロパティは nullable 必須（cleanUpProperties が null を代入する） - HTML パートを持たないメールへの assertEmailHtmlBodyNotContains は空振り - contributing Skill「よくある間違い」に 1 項追記 - 非推奨 public API の削除監査は e2e/ と codeception/ も対象に含める Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@nanasess レビューありがとうございます。指摘はすべて実コードで裏取りし対応しました。あわせて Part 1（deprecation ゲート）が実は無効だったことが判明したため、方式を差し替えています。 ① 🔴
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/setup-fixtures.php (1)
20-26: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
boot_env()で読み込んだAPP_ENVを$_SERVER/$_ENVから取得してください。
boot_env()は Symfony Dotenv のままではputenv()を使用せず$_SERVER/$_ENVに値を書き込むため、.envのAPP_ENVをgetenv()だけで参照すると反映されず、Kernel がe2eで起動します。-$appEnv = getenv('APP_ENV') ?: 'e2e'; +$appEnv = $_SERVER['APP_ENV'] + ?? $_ENV['APP_ENV'] + ?? (getenv('APP_ENV') ?: 'e2e');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/setup-fixtures.php` around lines 20 - 26, Update the APP_ENV assignment after boot_env() to read from $_SERVER or $_ENV before falling back to 'e2e', rather than relying only on getenv(). Preserve the existing fallback when neither environment array contains APP_ENV.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@e2e/setup-fixtures.php`:
- Around line 20-26: Update the APP_ENV assignment after boot_env() to read from
$_SERVER or $_ENV before falling back to 'e2e', rather than relying only on
getenv(). Preserve the existing fallback when neither environment array contains
APP_ENV.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a4ff616f-8a1a-493c-a098-576332df8de3
📒 Files selected for processing (16)
.claude/skills/contributing/SKILL.md.claude/skills/phpunit/SKILL.mdAGENTS.mdcodeception/acceptance/_bootstrap.phpe2e/setup-fixtures.phpphpunit.xml.distsrc/Eccube/Entity/Cart.phpsrc/Eccube/Entity/Master/CustomerStatus.phpsrc/Eccube/Entity/Order.phpsrc/Eccube/Entity/OrderItem.phpsrc/Eccube/Entity/Product.phpsrc/Eccube/Entity/ProductClass.phpsrc/Eccube/Service/PluginService.phpsrc/Eccube/Service/PurchaseFlow/InvalidItemException.phpsrc/Eccube/Twig/Extension/SafeTextmailEscaperExtension.phpsrc/Eccube/Util/CacheUtil.php
💤 Files with no reviewable changes (6)
- src/Eccube/Entity/OrderItem.php
- src/Eccube/Entity/Product.php
- src/Eccube/Entity/ProductClass.php
- src/Eccube/Entity/Order.php
- src/Eccube/Util/CacheUtil.php
- src/Eccube/Entity/Master/CustomerStatus.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Eccube/Entity/Cart.php
ローカルで除外していたグループ (plugin-service / cache-clear) に 残っていた非推奨警告を解消する。 - PluginServiceTest::testCreateEntityAndTrait: ダミープラグインの Block エンティティに ORM 非マッピングの public $sample を宣言する。 テストが $Block->sample = true と代入していたため PHP 8.2 の 動的プロパティ生成 (deprecated) になっており、failOnDeprecation で CI が失敗していた（12 マトリクス全滅の唯一の原因） - TemplateControllerTest::testChangeTemplateWithEnvOverride: doc-comment の @group cache-clear を #[Group(name: 'cache-clear')] 属性に統一する。 同ファイルの他メソッドは既に属性を使っており取り残しだった。 doc-comment メタデータは PHPUnit 12 で廃止予定 これによりスイート全体の Deprecations / PHPUnit Deprecations がいずれも 0 になった（cache-clear グループは 21 テストのまま変化なし）。 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI で PHP 8.4 / 8.5 のジョブのみ失敗していた（8.2 / 8.3 は緑）。原因は failOnDeprecation が PHP 8.4 / 8.5 で新たに非推奨となった呼び出しを検出した ためで、うち 1 件は vendor 内で我々には修正できないものだった。 ゲート範囲を direct のみに限定 (phpunit.xml.dist): PHPUnit 11 の <source> は Symfony の SYMFONY_DEPRECATIONS_HELPER と同じ 分類 (self / direct / indirect) を持つため、当初意図した max[direct]=0 と 同じ範囲になるよう設定する。 - ignoreSelfDeprecations="true" … 自コードの @deprecated 予告 (trigger_error) は対象外 - ignoreIndirectDeprecations="true" … vendor 内部で発生し修正できないものは対象外 (mobiledetect/mobiledetectlib の暗黙 nullable。PHP 8.4 で非推奨) direct な非推奨呼び出しの解消（いずれも挙動不変）: - Order::getMergedProductOrderItems(): 未永続明細では ProductClass の ID が null になり、null を配列キー / オフセットに使っていた（PHP 8.5 で非推奨）。 (string) にキャストする。null は '' として扱われるため挙動は同じ - ProductController::index() / OrderController::index(): $searchData['sortkey'] が null のとき COLUMNS[null] を参照していたため ?? '' で受ける - PluginApiService: curl_close() は PHP 8.0 以降なにもせず 8.5 で非推奨に なったため呼び出しを削除する - CsvExportServiceTest / SjisToUtf8EncodingFilterTest: fgetcsv() の $escape を 現行の既定値 '\\' で明示する（PHP 8.4 で明示指定が必須）。とくに fgetcsvDoesntOccur5cProblem は SJIS 2 バイト目の 0x5c を escape 文字として 誤認しないことの確認なので、既定値を変えずに明示することが重要 なお同ジョブのリトライ 2 回目で失敗する RateLimiterListenerTest 3 件は 1 回目が緑であり、レートリミッタの状態が試行間で残る既知の flaky で本変更とは無関係。 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.4 #6937 +/- ##
==========================================
- Coverage 77.18% 77.17% -0.02%
==========================================
Files 564 564
Lines 28123 28082 -41
==========================================
- Hits 21707 21671 -36
+ Misses 6416 6411 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
CI が全 115 チェック成功しました（PHP 8.2 / 8.3 / 8.4 / 8.5 × pgsql / pgsql13 / mysql / sqlite3、Playwright E2E 含む）。前回コメント以降の追加対応を報告します。 CI 1 回目: PHPUnit 全 12 マトリクス失敗 → 原因は 1 件本体スイート（2905 テスト）自体は Failures / Errors 0 で緑でした。ローカルで出ていた 16 件の Failures は 失敗の真因は、ローカルで除外していた
別途ご確認いただきたい点: このテストは あわせて、スイート唯一の残件だった CI 2 回目: PHP 8.4 / 8.5 のみ失敗 → ゲート範囲を direct に限定8.2 / 8.3 は全て緑で、8.4 / 8.5 の 9 ジョブだけが落ちました。 PHPUnit 11 の
そのうえで direct な非推奨呼び出しを解消しました（いずれも挙動不変）。
切り分けた flakyリトライ 2 回目で落ちていた 補足
|
nanasess
left a comment
nanasess
left a comment
There was a problem hiding this comment.
先の change request で指摘した e2e/setup-fixtures.php（および補足の codeception/acceptance/_bootstrap.php）の CustomerStatus::ACTIVE/NONACTIVE 参照が REGULAR/PROVISIONAL へ修正されたことを確認しました。
- ゲートが PHPUnit 11 ネイティブの
failOnDeprecation+ignoreSelfDeprecations/ignoreIndirectDeprecationsに置き換わり、当初意図（direct のみで落とす）を標準機能で表現できています。 - 追加された本体コードの非推奨解消（
OrderController/ProductControllerの??、curl_close()削除、PluginServiceの null ガード、InvalidItemException・SafeTextmailEscaperExtensionの null 対策）はいずれも最小・挙動不変。 - コンフリクト解消により
main.ymlがフル実行され、PHPUnit・dockerbuild(8.2–8.5)・e2e-test 全 suite・e2e-test-throttling が全て pass。ゲートと E2E フィクスチャの両方が実 CI で裏取りされています。
LGTM 👍
| * | ||
| * @deprecated 税率設定は受注作成時に決定するため廃止予定 | ||
| */ | ||
| public function setTaxRuleId(?int $taxRuleId = null): OrderItem |
There was a problem hiding this comment.
この関数は、クーポンプラグインで使用しているようです。
削除する場合は、先にプラグインの修正PRをお願いします。
https://github.com/EC-CUBE/coupon-plugin/blob/112ab3ebc461cf77e2d63af3f59f33273251d392/Service/PurchaseFlow/Processor/CouponProcessor.php#L298
There was a problem hiding this comment.
ご指摘ありがとうございます。プラグイン側の修正 PR を先に用意しています。
- fix: EC-CUBE 4.4 で削除される OrderItem::setTaxRuleId() の呼び出しを除去 coupon-plugin#199
fix: EC-CUBE 4.4 で削除される OrderItem::setTaxRuleId() の呼び出しを除去
内容は次の通りです。
Service/PurchaseFlow/Processor/CouponProcessor.php… ご指摘の箇所は$taxRuleId = null;を組み立ててsetTaxRuleId(null)を呼んでいるだけで、書き込んだ値の読み手が本体・プラグインともに存在しません。よって変数ごと除去しました（挙動は変わりません）。Tests/Repository/CouponCouponOrderRepositoryTest.php/Tests/Service/CouponServiceTest.php… テストのsetTaxRuleId()呼び出しも同様に除去しました。
base は 4.4 対応 PR（EC-CUBE/coupon-plugin#197、head feature/eccube-4.4-symfony7）に積んだスタック PR です。OrderItem::setTaxRuleId() の削除は 4.4 のみで、4.3 で動く coupon-plugin の master は 4.3 コアの現行 API をそのまま使うため変更不要と判断しています。
取り込み順は coupon-plugin#197 → coupon-plugin#199 の順にマージいただければ、4.4 対応版のプラグインは本 PR の削除後も動作します。プラグイン側のレビューもあわせてお願いできますでしょうか。
|
追加で見たところ、定期購入プラグインも影響があるかと思います。
|
…on-gate # Conflicts: # tests/Eccube/Tests/Web/Admin/Store/TemplateControllerTest.php
deprecation ゲート有効化により、PHP 8.4/8.5 の unit-test が全 DB で red に なっていた。返品申請の CSV 出力が $escape を省略しており、PHP 8.4 で 「the $escape parameter must be provided as its default value will change」 の非推奨が 3 件発生していたため。 出力を変えないよう、コアの既定値 '\' を明示する（AbstractCsvImportController・ CsvExportService と同じ挙動）。本 PR で既に対応済みの CsvExportServiceTest・ SjisToUtf8EncodingFilterTest に合わせ、名前付き引数で指定する。 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
削除した非推奨定数 CustomerStatus::ACTIVE の置換が 1 箇所漏れており、 setup-fixtures.php が返品申請テスト用会員の作成時に Fatal で停止していた。 その結果、以降のフィクスチャ（発送済み注文・返品申請・受注メモ用受注）が 未作成となり、admin-order / admin-refund-request / front-refund-request の 3 スイートが落ちていた。 REGULAR は ACTIVE と同値（2）のため挙動は変わらない。 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
globalSetup は失敗時に error.stderr しか出力していなかったが、PHP CLI は Fatal error を stdout に書くため、setup-fixtures.php が落ちても 「Command failed」しか残らず原因を追えなかった（前コミットの置換漏れの調査で CI ログから原因を特定できなかった）。stdout も併せて出力する。 フィクスチャ失敗でもテストを継続する既存の方針は変更しない。 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
ご指摘ありがとうございます。 他のプラグインも確認しましたが、影響がありそうなのはご指摘の2プラグインのみでした。 別途 Issue を立てたので、そちらで対応します。 |
EC-CUBE 本体 4.4 で非推奨 public API を撤去する EC-CUBE/ec-cube#6937 により Eccube\Entity\OrderItem::setTaxRuleId() / getTaxRuleId() が削除されるため、 本プラグインからの呼び出しを削除する。 - Service/PurchaseFlow/Processor/CouponProcessor.php: クーポン値引き明細生成時の setTaxRuleId(null) と未使用になる $taxRuleId 変数を削除（意図が失われないよう コメントを残す） - Tests/Service/CouponServiceTest.php / Tests/Repository/CouponCouponOrderRepositoryTest.php: テストフィクスチャの setTaxRuleId() 呼び出しを削除 tax_rule_id カラムは OrderItem の private プロパティとして残るため既存データに影響はなく、 書き込んだ値を読み出す実装が本体にもプラグインにも存在しないため、代替処理は不要。 Refs #198 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
未対応だったレビュー指摘 3 件へ個別に返信しました。現状をまとめます。
コアの削除とプラグイン修正の取り込み順は、EC-CUBE/coupon-plugin#197 → EC-CUBE/coupon-plugin#199 をマージいただいてから本 PR、という形を想定しています。あらためてご確認をお願いします。 |
AGENTS.md の技術スタックが衝突していたため、両者の変更を統合した。 - フロントエンド: 4.4 の esbuild 移行を採用 - テスト: 本ブランチの phpunit-bridge 無効化・failOnDeprecation の記述を維持
先行 PR (#6937 / #7013) のマージを取り込む。 .claude/skills/eccube-phpunit/SKILL.md の「よくある間違い」で衝突したため、 両側の項目を残したうえで AGENTS.md の歯止め（1 Skill 10 項・1 項 120 字程度、 超過分は追記ではなく統合）に沿って統合した。 - 4.4 側 3 項（未宣言プロパティ / 非 nullable プロパティ / HTML パート無しメール）と 本ブランチ側 1 項（ローカルだけ 500 になるテスト）はいずれも情報を保持している。 - 未宣言プロパティと非 nullable プロパティは原因が同じ「プロパティ宣言」なので 1 項に統合。 - HTTP クライアント / URL / Entity の自前生成、ID・ステータス値の直書き、 回帰テストのゲート未確認と PHP Warning も同種のため統合した。 - 結果として 13 項・最大 411 字 → 8 項・最大 143 字。 AGENTS.md では「既存の超過は次にその節へ手を入れるときに統合・短縮する」と 定めており、衝突解消でこの節に触るため、あわせて実施した。
3 participants
概要
Issue #6933 のうち、仕様（実行時の挙動・CSV等の出力・管理/店頭画面・DBスキーマ）を一切変えない範囲で対応する。
@deprecatedpublic API の削除（4.4 の BC 破壊に相乗り）Fixes #6933（の安全に実施できる範囲。据え置き分は後述）
方針
Part 1 — deprecation ゲートを PHPUnit 11 ネイティブの
failOnDeprecationで有効化自コードが非推奨 API を呼んだら CI が失敗するようにし、次期 Symfony（7.4→8）追従を「負債の発掘」から「非推奨を消すだけ」に変えるための早期警告を入れる。
当初は
SYMFONY_DEPRECATIONS_HELPERをweak→max[direct]=0に引き上げる方針だったが、この設定は PHPUnit 11 環境では機能しないことがレビュー中の検証で判明したため、PHPUnit 11 が持つネイティブのゲートへ置き換えた。SYMFONY_DEPRECATIONS_HELPERが効かない理由DeprecationErrorHandlerを登録するのはsymfony/phpunit-bridgeのbootstrap.php（composer のautoload.filesで常時ロードされる）だが、同ファイルは冒頭でif (class_exists(PHPUnit\Metadata\Metadata::class)) { return; }により PHPUnit 10 以上では早期 return するSymfonyExtensionは ClockMock / DnsMock のみを登録し、DeprecationErrorHandlerは登録しない（DeprecationErrorHandlerを参照するのはbootstrap.phpとbin/simple-phpunit.phpだけ）unit-test.yml/coverage.yml）はvendor/bin/phpunitを実行するため、bridge 経由のsimple-phpunitも通らないvendor/bin/phpunitで実行しても bridge のサマリは出力されず、警告も集計されない（SymfonyExtensionを登録しても同じ）置き換え内容（
phpunit.xml.dist）SYMFONY_DEPRECATIONS_HELPERfailOnDeprecation="true"displayDetailsOnTestsThatTriggerDeprecations="true"<source ignoreSelfDeprecations="true" ignoreIndirectDeprecations="true">ゲート範囲を direct に限定した理由
PHPUnit 11 の
<source>は Symfony のSYMFONY_DEPRECATIONS_HELPERと同じ分類を持ちます（IssueTrigger::isDirect()は「first-party/test コードが third-party/PHP の非推奨を呼ぶ」＝Symfony の direct と同義）。当初意図したmax[direct]=0と同じ範囲にするため、次の 2 つを除外しています。trigger_errorする@deprecated予告）max[self]=0を併用しなかった判断と同じmobiledetect/mobiledetectlibの暗黙 nullable 引数。PHP 8.4 で非推奨）これを入れないと PHP 8.4 / 8.5 のジョブが vendor 由来の非推奨で必ず落ちます（8.2 / 8.3 では発生しません）。
coverage.ymlは base の4.4で Codeception カバレッジジョブ自体が削除済み（#6905）のため、本 PR での変更は無くなった。SYMFONY_DEPRECATIONS_HELPERはリポジトリから完全に撤去されている。前提として既存の非推奨警告 19 件を解消（いずれも挙動不変）
PHP 8.2 で検出できた 11 件に加え、CI で PHP 8.4 / 8.5・除外グループ（
plugin-service/cache-clear）で表面化した 8 件も解消している。SafeTextmailEscaperExtension（Twig は null の変数もそのまま渡すためエスケーパ引数を?stringに）/PluginService::deleteDirs（install()/update()が例外発生時点で未設定の null を渡す仕様のため null をスキップ・phpdoc をstring|nullに）/InvalidItemException（parent::__construct()へ null を渡さない）MasterdataTypeTest::$form/LoginHistoryRepository…AdminTestの$Member1・$LoginHistory1-3/MailControllerTestの$Member・$MailHistoriesを宣言。EccubeTestCase::cleanUpProperties()が tearDown で全プロパティに null を代入するため nullable で宣言assertEmailHtmlBodyNotContains()をassertNull($Message->getHtmlBody())に変更（MailServiceTest3 箇所・WithdrawControllerTest1 箇所）。元の書き方はsymfony/mime内でstr_contains(null, …)の警告を出すうえ、HTML パートが存在しないため常に成立する空振りアサーションだったPluginServiceTest::testCreateEntityAndTraitのダミープラグインBlockに ORM 非マッピングのpublic $sampleを宣言。テストが$Block->sample = trueと代入していたため動的プロパティ生成になっていたTemplateControllerTest::testChangeTemplateWithEnvOverride()の@group cache-clearを#[Group]属性へ（同ファイルの他メソッドは既に属性を使用。PHPUnit 12 で廃止予定）Order::getMergedProductOrderItems()（未永続明細でProductClassの ID が null になり null を配列キー / オフセットに使用 →(string)キャスト）/ProductController::index()・OrderController::index()（$searchData['sortkey']が null のときCOLUMNS[null]参照 →?? ''）/PluginApiService（curl_close()は PHP 8.0 以降なにもせず 8.5 で非推奨 → 削除）/fgetcsv()の$escape5 箇所を現行既定値'\\'で明示（PHP 8.4 で明示指定が必須。とくにfgetcsvDoesntOccur5cProblemは SJIS 2 バイト目の 0x5c を escape 文字と誤認しないことの検証なので既定値を変えないことが重要）Part 2 — 削除できるのは「呼び出し元なし／CSV非出力／スキーマ不変」のものだけ
各
@deprecatedpublic API を「①コアに呼び出し元があるか ②CSV 出力に載る保存カラムのアクセサか ③カラム削除マイグレーションを伴うか」で判定し、すべて No のもののみ削除した。削除
OrderItem::getTaxRuleId()/setTaxRuleId()tax_rule_idカラムは残置）Product::isEnable()＋ProductClass::isEnable()Cart::getLock()/setLock()＋$locktransChoice()trans()の別名・呼び出し元皆無TaxProcessor::getTaxDisplayType()CartService::$cartOrder::getTotalPrice()getPaymentTotal()の別名CacheUtil::clear()clearCache()（専用テストごと削除）CustomerStatus::NONACTIVE/ACTIVEPROVISIONAL/REGULARと同値の別名据え置き（削除すると仕様が変わるため本 PR では触らない）
Order::getTax/setTax・ItemHolderInterface::setTax・Order::getDiscount＋tax/discountカラム … CSV 出力「税金」「値引き」列を支える現役。@deprecated予告は残置。BaseInfo::getPhpPath/setPhpPath＋php_pathカラム … 削除にマイグレーション（スキーマ変更）を伴う。併せて訂正したドキュメント
SYMFONY_DEPRECATIONS_HELPERが効かない原因調査の過程で、AGENTS.mdと Skill の記述に誤りがあることが判明したため訂正した。symfony/phpunit-bridge経由）」→「vendor/bin/phpunitを直接実行」（bridge の非推奨検出が PHPUnit 10 以上で無効である旨も明記）bin/phpunit→vendor/bin/phpunit（bin/にはconsoleとtemplate_jp.phpのみ）テスト
CacheUtilTest、OrderTest::testGetTotalPriceEditControllerTest:getTotalPrice()→getPaymentTotal()EntryControllerTest/CustomerRepository(GetQueryBuilderBySearchData)Test/Generator:CustomerStatus::NONACTIVE/ACTIVE→PROVISIONAL/REGULARe2e/setup-fixtures.php（Playwright のglobal-setup.tsが全 E2E 前に実行）codeception/acceptance/_bootstrap.php（VAddy スキャンがcodecept run -g vaddyで実行し、@group vaddyの Cest がcreateCustomer()を使う）RateLimiterListenerTest3 件は 1 回目が緑で、レートリミッタの状態が試行間に残る既知の flaky。failOnPhpunitDeprecationは有効化していない（PHPUnit 自身の非推奨は現時点 0 件だが、マトリクス差で表面化するリスクを避けた）。互換性
failOnDeprecation="true"の有効化により、プラグインのテストが非推奨 API を呼んでいる場合は CI が失敗するようになる。🤖 Generated with Claude Code
Summary by CodeRabbit
破壊的変更
バグ修正
テスト