Skip to content

feat: EC-CUBE 4.4への対応 - #197

Merged
ttokoro20240902 merged 5 commits into
4.4from
feature/eccube-4.4-symfony7
Aug 7, 2026
Merged

feat: EC-CUBE 4.4への対応#197
ttokoro20240902 merged 5 commits into
4.4from
feature/eccube-4.4-symfony7

Conversation

@ttokoro20240902

@ttokoro20240902 ttokoro20240902 commented Jul 3, 2026

Copy link
Copy Markdown

概要

クーポンプラグインを EC-CUBE 4.4(Symfony 7.4 / Doctrine ORM 3.0 / PHP 8.2+) に対応させ、コードを Coupon42Coupon44 に改名します。4.3 とは非互換(属性必須・ORM 3・PHP 8.2+)のため、新規 4.4 ブランチへの取り込みです。

参考: 4.3→4.4 マイグレーション手順 (doc #346) / 先行対応 related-product-plugin #91

変更内容

1. EC-CUBE 4.4 対応(コア移行)

  • Annotations → PHP 属性: Entity の @ORM\*/@UniqueEntity#[ORM\*]/#[UniqueEntity]、Controller の @Route/@Template#[Route]/#[Template]Sensio 依存除去)、PurchaseFlow の @ShoppingFlow/@OrderFlowEccube\Attribute\* 属性へ
  • @ParamConverter の除去(本プラグイン固有): CouponController::enable/delete#[MapEntity(id: 'id')]Symfony\Bridge\Doctrine\Attribute\MapEntity)へ移行
  • Doctrine ORM 3.0: Entity を型付きプロパティ化(typeDoctrine\DBAL\Types\Types::*)、flush($entity)flush()AbstractRepository::save(AbstractEntity $entity): void とのシグネチャ互換化、Repository に @extends AbstractRepository<Entity> を付与
  • Symfony 7: PurchaseFlow インターフェース(process/validate/prepare/commit/rollback)の : void 戻り値、Assert\* の名前付き引数化、コンストラクタプロモーション・戻り値/引数の型明示
  • Form: getName()getBlockPrefix()buildForm(): void / configureOptions(): void
  • PHPUnit 11: phpunit.xml.dist<source>/<extensions> 形式へ
  • Tests: namespace を Plugin\Coupon44\... に統一、データプロバイダの static 化、flush()・戻り値型・decimal の文字列比較(assertEquals)へ追従、重複していた Tests/Service/PurchaseFlow/CouponProcessorTest.php(旧版)を整理
  • コード名: namespace / composer code / version: 4.4.0 / Twig 名前空間 @Coupon44 を統一

2. Docker Compose によるテスト環境

  • docker-compose.yml + dev/mysql/pgsql オーバーレイ、dockerbuild/
  • EC-CUBE 4.4 イメージ(ghcr.io/ec-cube/ec-cube-php:8.2-apache-4.4)でプラグインを自動導入・有効化。APP_ENV=dev 起動+dev 用 Cookie 設定(cookie_secure:false/cookie_samesite:lax)で HTTP ログインに対応

3. 静的解析・整形ツール

  • phpstan.neon.dist(level 6・baseline なし)/ Resource/rector.php / Resource/.php-cs-fixer.dist.php を追加(.php 設定は本体の Plugin\: サービス検出で 500 を避けるため Resource/ 配下に配置)
  • decimal の型付きプロパティ(?string)と not-null カラムの columnType 差分は本体コアと同様 ignoreErrors で許容(理由をコメント明記)
  • CLAUDE.md に開発・テスト手順、アーキテクチャ、移行・配置の注意を記載

4. CI(.github/workflows/

  • ci.yml: マトリクスを EC-CUBE 4.4 / PHP 8.2-8.5 / MySQL8・PostgreSQL に更新、checkout@v4$GITHUB_OUTPUT 化、--ignore-platform-req=ext-redis、phpunit 前に cache:warmupstatic-analysis ジョブ(php-cs-fixer / rector / phpstan)追加
  • main.yml(release): 配布から開発・テスト用ファイル(docker-compose / dockerbuild / CLAUDE.md / phpstan / rector / php-cs-fixer)を除外

5. レビュー指摘対応(commit ae08221

  • 複数配送 + クーポンで購入フローが 500 になる不具合を修正: CouponServiceresolveTaxValues() を切り出し、recalcOrder() / isLowerLimitCoupon() の双方で税率・丸め規則を解決。ShippingMultipleControllerRoundingType 未設定で OrderItem を作り直し、TaxProcessorItemHolderPreprocessor)より先に itemHolderValidator が走るため rounding_type_id が null で渡り、非 nullable になった TaxRuleService::calcTax()TypeError になっていた
  • ページ管理からのテンプレート編集が反映されない不具合を修正: CouponShoppingController#[Template]dtb_page.file_name と一致する名前空間なし表記に戻す(@Coupon44 はプラグイン本体に固定解決され、テーマ側に保存されるページ管理の編集が無視される)。再発防止の理由をコメントで明記
  • CouponOrderRepository::save() を親と同じ persist のみに戻す: ORM 3 で flush が UnitOfWork 全体に波及するため、flush は CouponService::saveCouponOrder で明示(CouponStateProcessor は save 直後に自前 flush があり変更不要)
  • テスト追加: CouponStateProcessorTest(キャンセル/返品の枚数戻し・発行枚数での頭打ち・ステータス復帰時の再使用。supports() 対象外ステータスが process() に到達しないことも明文化)、RoundingType 未設定ケース(CouponServiceTest / CouponProcessorTest)、isDiscount() の検証(移植漏れ)
  • CI: DB イメージを本体 4.4 に合わせる(mysql:8.4 / postgres:18)、extensions: ':xdebug' を復活
  • その他: Event.php の未使用 import 削除、Bootstrap 5 に存在しない mr-*me-*(既存不具合。4.2 向けは fix: Bootstrap 5 に存在しない mr-2 / mr-3 を me-2 / me-3 に修正 #200
  • スキップしている OrderControllerTest::testOrderEditWithCouponCancel の追跡先として本体側に Issue を作成: テスト環境で EntityManager::lock() が TransactionRequiredException になり、キャンセル遷移を含む受注編集の Web テストが書けない ec-cube#7016

テスト

ローカルの docker-compose 4.4 環境(8.2-apache-4.4 / MySQL 8)で以下を確認済み:

  • PHPUnit: 127 tests / 196 assertions パス(Failures/Errors 0、Skipped 1)※ PHPUnit 11.5 / PHP 8.2
  • php-cs-fixer: 差分ゼロ
  • rector: 変更ゼロ
  • phpstan level 6: エラーゼロ(baseline なし)

複数配送・ページ管理の実機確認(レビュー指摘分)

  • 複数配送: ゲスト購入(数量 2)→ クーポン適用 → お届け先の複数指定 → 「選択したお届け先に送る」で、修正前は calcTax(): Argument #3 ($RoundingType) must be of type int, null given で 500 になることを再現したうえで、修正後は正常に注文手続きへ戻り、そのまま注文確定まで到達することを確認(payment_total=6660 / discount=500plg_coupon_order 記録、利用枚数 100→99)
  • ページ管理: ページ管理の保存先であるテーマ側(app/template/default/Coupon44/...)に置いた内容がクーポン入力画面に反映されることを確認

Skipped 1 件について(OrderControllerTest::testOrderEditWithCouponCancel

受注ステータスをキャンセルへ遷移させると本体の StockReduceProcessor が在庫戻しのため EntityManager::lock()(悲観ロック)を実行しますが、これは開いたトランザクションを要求し、DAMA DoctrineTestBundle のテスト用トランザクションでは TransactionRequiredException になります(本体側 4.4 のテストハーネス制約で本番の実リクエストでは発生しない)。CouponStateProcessor を無効化しても同じ例外が出ることを確認しており、本プラグイン起因ではないため当該ケースのみスキップしています。

CI(PR上)でも EC-CUBE 4.4 × PHP 8.2/8.3/8.4/8.5 × MySQL8/PostgreSQL の PHPUnit と static-analysis を確認します。

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • クーポンプラグインを EC-CUBE 4.4 向けに更新し、管理画面・購入画面・マイページ表示の対応範囲を広げました。
    • 開発/検証用の Docker 環境が追加・整理され、MySQL/PostgreSQL でも起動しやすくなりました。
  • Bug Fixes

    • クーポン適用時の表示・ルーティング不整合を改善しました。
    • クーポン利用/取消時の再計算や有効期限・利用回数の扱いを安定化しました。
  • Chores

    • 静的解析やテスト実行の仕組みを強化し、品質確認をしやすくしました。

Coupon42 を EC-CUBE 4.4(Symfony 7.4 / Doctrine ORM 3.0 / PHP 8.2+)へ対応し
コードを Coupon44 へ改名。

- Annotation → PHP 属性(Entity の #[ORM\*] / #[UniqueEntity]、Controller の
  #[Route]/#[Template]、Processor の #[ShoppingFlow]/#[OrderFlow])
- @ParamConverter → #[MapEntity](Symfony\Bridge\Doctrine\Attribute\MapEntity)
- Doctrine ORM 3.0: 型付きプロパティ・flush() 引数廃止対応・AbstractRepository
  ::save(AbstractEntity): void 互換化・@extends 付与
- Symfony 7 / PurchaseFlow インターフェースの : void 戻り値・Assert 名前付き引数
- Form の getName() → getBlockPrefix()
- composer(code/name/version=4.4.0) / phpunit.xml.dist を PHPUnit 11 形式へ
- CI(ci.yml)を 4.4 × PHP8.2-8.5 × MySQL8/PostgreSQL に更新し static-analysis 追加
- docker-compose 開発環境 / phpstan(level6) / rector / php-cs-fixer / CLAUDE.md 追加
- Tests を Coupon44 / PHPUnit 11 / ORM3 に追従、重複テスト整理

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

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7d9abf99-1777-4190-81d5-82ad52c073c0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

クーポンプラグインをCoupon42からCoupon44へ全面移行した。Entity/Repository/Form/Controller/Service/PurchaseFlowプロセッサ/Event等の名前空間をCoupon44に変更し、Doctrineアノテーションを属性へ、メソッドシグネチャに型宣言を追加。CI/Docker開発環境・静的解析ツール(php-cs-fixer/rector/phpstan)を新設・更新した。

Changes

プラグイン本体のCoupon44移行

Layer / File(s) Summary
Entityの型付け・属性化
Entity/Coupon.php, Entity/CouponDetail.php, Entity/CouponOrder.php
名前空間をCoupon44へ変更し、DoctrineアノテーションをPHP属性へ置換、全getter/setterに戻り値型・引数型(nullable/union)を付与。
Repository層の型付け
Repository/CouponRepository.php, Repository/CouponOrderRepository.php, Repository/CouponDetailRepository.php
名前空間更新、メソッド引数・戻り値型付け、flushを引数なし呼び出しへ統一。
Form Typeの属性化
Form/Type/CouponType.php, Form/Type/CouponDetailType.php, Form/Type/CouponSearchCategoryType.php, Form/Type/CouponUseType.php
名前空間更新、buildForm/configureOptionsへ: void付与、getName()からgetBlockPrefix()へ置換、バリデーション制約をnamed argumentsへ変更。
Controllerの属性ルーティング
Controller/Admin/CouponController.php, Controller/Admin/CouponSearchModelController.php, Controller/CouponShoppingController.php
#[Route]/#[Template]属性へ移行、コンストラクタをプロパティプロモーション化、flush呼び出しを引数なしへ変更。
CouponServiceの依存整理
Service/CouponService.php
コンストラクタ依存を縮小し公開メソッドに型宣言を追加、flush呼び出しをまとめる形へ変更。
PurchaseFlowプロセッサの属性化
Service/PurchaseFlow/Processor/CouponProcessor.php, Service/PurchaseFlow/Processor/CouponStateProcessor.php
#[ShoppingFlow]/#[OrderFlow]属性へ移行、戻り値型明示、null値の?? 0ガード追加。
Event/Nav/PluginManager/テンプレート参照付け替え
Event.php, Nav.php, PluginManager.php, Resource/template/*, README.md, composer.json
名前空間とテンプレート参照先をCoupon44へ更新、パッケージ識別情報を更新。
テストスイートのCoupon44移行
Tests/**
全テストの名前空間更新、: void戻り値型付与、flush呼び出しの引数なし化、フィルタ引数型のItemInterface化。

CI/Docker開発環境と静的解析ツール整備

Layer / File(s) Summary
CIワークフロー更新
.github/workflows/ci.yml, .github/workflows/main.yml
対象プラグイン・EC-CUBEバージョン・実行環境を更新、アクションをバージョンアップ、static-analysisジョブを新設。
Docker Compose開発環境追加
docker-compose*.yml, dockerbuild/*, .gitignore
開発用Compose定義とDB/セッション設定を新規追加。
静的解析・テスト実行設定
CLAUDE.md, Resource/.php-cs-fixer.dist.php, Resource/rector.php, phpstan.neon.dist, phpunit.xml.dist, Tests/bootstrap.php
php-cs-fixer/rector/phpstan設定を新規作成、phpunit設定整理。

Estimated code review effort: 4 (Complex) | ~75 minutes

Sequence Diagram(s)

sequenceDiagram
    participant Customer
    participant CouponShoppingController
    participant CouponRepository
    participant CouponService
    participant CouponOrderRepository

    Customer->>CouponShoppingController: shoppingCoupon(couponCd)
    CouponShoppingController->>CouponRepository: findActiveCoupon(couponCd)
    CouponRepository-->>CouponShoppingController: Coupon or null
    CouponShoppingController->>CouponService: existsCouponProduct(Coupon, Order)
    CouponService-->>CouponShoppingController: couponProducts
    CouponShoppingController->>CouponService: recalcOrder(Coupon, couponProducts)
    CouponService-->>CouponShoppingController: discount
    CouponShoppingController->>CouponService: saveCouponOrder(Order, Coupon, couponCd, Customer, discount)
    CouponService->>CouponOrderRepository: save(CouponOrder)
    CouponShoppingController-->>Customer: redirect to shopping
Loading

Poem

ぴょんぴょんコードを跳び越えて 🐇
Coupon42はもう昔話、
属性つけて型も添えて、
Coupon44へ引っ越し完了!
にんじん片手にCIも緑、
みんなでキャッシュもクリアしよう✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed タイトルは、EC-CUBE 4.4対応というプルリクエストの主目的を明確かつ簡潔に示しています。
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/eccube-4.4-symfony7

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

PR #197 のレビュー指摘を反映。

- クーポン利用画面の「利用する/しない」トグルJSが旧ブロックプレフィックス
  (front_plugin_coupon_shopping) を参照しており、実レンダリング名 (coupon_use)
  と不一致で動作していなかったため修正(既存バグ)。
- CouponService::existsDepthCategory の親カテゴリ探索が反転・find(entity) 誤用
  だったため getParent() での再帰に修正。未使用になった CategoryRepository の
  注入を除去(本体が商品保存時に祖先カテゴリを保存するため実害は無いデッドコード整理)。
- ORM3 で nullable になった getter の算術/比較を ?? 0・null ガードで防御
  (CouponRepository::checkCouponUseTime, CouponStateProcessor, CouponProcessor)。
- deleteCoupon / removeCouponOrder / CouponController::edit のループ内 flush を
  ループ外 1 回に集約。
- テスト統合時に欠落していた値引き明細の税区分(NON_TAXABLE)アサーションを
  CouponProcessorTest に復活。

検証: PHPUnit 118 tests OK / php-cs-fixer 差分ゼロ / rector 変更ゼロ /
phpstan level 6 エラーゼロ。フロントのトグル挙動は Playwright で修正前後を確認。

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

Copy link
Copy Markdown
Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai 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.

Actionable comments posted: 3

🧹 Nitpick comments (5)
Tests/Web/Admin/OrderControllerTest.php (1)

188-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

キャンセル時クーポン表示テストは恒久的にスキップ状態。

markTestSkipped() によりキャンセル時のクーポン表示シナリオのカバレッジが失われるが、根本原因(DAMA DoctrineTestBundle と本体 StockReduceProcessor の悲観ロックの非互換性)はプラグイン非依存であり、コメントで十分に説明・検証済み。本体側の制約が解消され次第、@phpstan-ignore 以降のコードで再有効化できるよう、本体側 Issue へのリンクなど追跡手段を残しておくと良い。

Also applies to: 222-222

🤖 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 `@Tests/Web/Admin/OrderControllerTest.php` around lines 188 - 205,
`testOrderEditWithCouponCancel()` is being permanently skipped, so the
coupon-cancel coverage is lost. Keep the skip only as a temporary workaround,
but add a clear tracking reference in the test comment or nearby documentation
to the upstream issue/blocker tied to `StockReduceProcessor` and the `DAMA
DoctrineTestBundle` pessimistic lock conflict, so the `markTestSkipped()` block
can be safely re-enabled later.
.github/workflows/ci.yml (4)

1-20: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

ワークフローに明示的な permissions が定義されていません。

静的解析ツール(zizmor)が指摘する通り、permissions: ブロックが存在しないため GITHUB_TOKEN が既定の(過剰な)権限で実行されます。ワークフロー全体、または各ジョブに最小権限を明示することを推奨します。

♻️ 提案例
+permissions:
+  contents: read
+
 jobs:
   run-on-linux:

Also applies to: 161-163

🤖 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/ci.yml around lines 1 - 20, The CI workflow currently runs
with the default broad GITHUB_TOKEN permissions because there is no explicit
permissions block. Add a minimal permissions declaration for the workflow in
ci.yml, or set job-level permissions on run-on-linux if different scopes are
needed, so the GitHub Actions job uses only the required access.

Source: Linters/SAST tools


62-64: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

actions/checkoutpersist-credentials: false の設定がありません。

静的解析ツール(zizmor)が指摘する通り、actions/checkout@v4 はデフォルトで認証情報(GITHUB_TOKEN)を .git/config に永続化します。後続ステップでの意図しない資格情報の漏洩・悪用を防ぐため、リポジトリの書き込みが不要なチェックアウトには persist-credentials: false を追加することを推奨します(本ジョブ内の他の checkout ステップにも同様に適用できます)。

♻️ 提案例(static-analysis ジョブ)
       - name: Checkout
         uses: actions/checkout@v4
+        with:
+          persist-credentials: false

Also applies to: 174-191

🤖 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/ci.yml around lines 62 - 64, The Checkout step using
actions/checkout@v4 is missing the persist-credentials setting, so the default
GITHUB_TOKEN may be written into .git/config. Update the Checkout invocation in
this workflow to set persist-credentials to false, and apply the same change to
the other checkout steps referenced by the review. Use the Checkout step under
the static-analysis job as the primary place to fix, and keep the change
localized to the actions/checkout usage.

Source: Linters/SAST tools


81-90: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

actions/cache@v4 のキャッシュポイズニング対策を検討してください。

静的解析ツール(zizmor)が actions/cache@v4 の既定挙動(全ブランチでの書き込みキャッシュ有効化)によるキャッシュポイズニングのリスクを指摘しています。フォークからのPRでも本ワークフローが実行される場合、restore/save を分離しデフォルトブランチのみ書き込み許可にする、またはキャッシュキーにブランチ/PR情報を含める等の緩和策を検討してください。

Also applies to: 193-202

Source: Linters/SAST tools


65-68: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

nanasess/setup-php@master はブランチ参照であり、サプライチェーンリスクがあります。

@master はタグや特定コミットではなく可変参照のため、上流での予期しない変更や侵害が即座にCIへ反映される可能性があります。特定のタグ/リリースまたはコミットSHAへの固定を推奨します。

Also applies to: 178-181

🤖 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/ci.yml around lines 65 - 68, The Setup PHP step is using
the mutable nanasess/setup-php@master reference, which should be pinned for
supply-chain safety. Update the workflow to use a fixed tag/release or commit
SHA for the Setup PHP action wherever it appears, including the repeated Setup
PHP entries, so the CI runs against an immutable version of nanasess/setup-php.
🤖 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 `@Controller/CouponShoppingController.php`:
- Around line 60-62: The shoppingCoupon action is using a different template
resolution style than the other coupon screens. Update the #[Template]
annotation on shoppingCoupon to use the same
`@Coupon44/default/shopping_coupon.twig` reference as the rest of the coupon UI,
so CouponShoppingController stays consistent with the shared template lookup
approach.

In `@Repository/CouponOrderRepository.php`:
- Around line 50-55: The CouponOrderRepository::save method signature is
incompatible with AbstractRepository::save, so restore it to match the parent
exactly. Update save(AbstractEntity $entity): mixed in CouponOrderRepository and
remove the custom void return type and any renamed parameter that breaks
inheritance, while keeping the persist/flush behavior inside the method.

In `@Service/CouponService.php`:
- Line 108: `recalcOrder()` が返す `discount_price` は文字列の小数値なので、`saveCouponOrder()`
の `int $discount` と型契約がずれています。`CouponService::recalcOrder()` から
`saveCouponOrder()` を呼ぶ箇所と `saveCouponOrder()` / `CouponOrder::setDiscount()`
の定義を見直し、円単位にするなら呼び出し前に明示的に整数化して渡すか、`discount` を小数対応の型に揃えて
`Controller/CouponShoppingController` 側も含めて一貫させてください。

---

Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 1-20: The CI workflow currently runs with the default broad
GITHUB_TOKEN permissions because there is no explicit permissions block. Add a
minimal permissions declaration for the workflow in ci.yml, or set job-level
permissions on run-on-linux if different scopes are needed, so the GitHub
Actions job uses only the required access.
- Around line 62-64: The Checkout step using actions/checkout@v4 is missing the
persist-credentials setting, so the default GITHUB_TOKEN may be written into
.git/config. Update the Checkout invocation in this workflow to set
persist-credentials to false, and apply the same change to the other checkout
steps referenced by the review. Use the Checkout step under the static-analysis
job as the primary place to fix, and keep the change localized to the
actions/checkout usage.
- Around line 65-68: The Setup PHP step is using the mutable
nanasess/setup-php@master reference, which should be pinned for supply-chain
safety. Update the workflow to use a fixed tag/release or commit SHA for the
Setup PHP action wherever it appears, including the repeated Setup PHP entries,
so the CI runs against an immutable version of nanasess/setup-php.

In `@Tests/Web/Admin/OrderControllerTest.php`:
- Around line 188-205: `testOrderEditWithCouponCancel()` is being permanently
skipped, so the coupon-cancel coverage is lost. Keep the skip only as a
temporary workaround, but add a clear tracking reference in the test comment or
nearby documentation to the upstream issue/blocker tied to
`StockReduceProcessor` and the `DAMA DoctrineTestBundle` pessimistic lock
conflict, so the `markTestSkipped()` block can be safely re-enabled later.
🪄 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: fa274bc8-689c-4d59-8a39-448bd816d98a

📥 Commits

Reviewing files that changed from the base of the PR and between 112ab3e and 8c418ab.

📒 Files selected for processing (51)
  • .github/workflows/ci.yml
  • .github/workflows/main.yml
  • .gitignore
  • CLAUDE.md
  • Controller/Admin/CouponController.php
  • Controller/Admin/CouponSearchModelController.php
  • Controller/CouponShoppingController.php
  • Entity/Coupon.php
  • Entity/CouponDetail.php
  • Entity/CouponOrder.php
  • Event.php
  • Form/Type/CouponDetailType.php
  • Form/Type/CouponSearchCategoryType.php
  • Form/Type/CouponType.php
  • Form/Type/CouponUseType.php
  • Nav.php
  • PluginManager.php
  • README.md
  • Repository/CouponDetailRepository.php
  • Repository/CouponOrderRepository.php
  • Repository/CouponRepository.php
  • Resource/.php-cs-fixer.dist.php
  • Resource/rector.php
  • Resource/template/admin/regist.twig
  • Resource/template/admin/search_category.twig
  • Resource/template/admin/search_product.twig
  • Resource/template/default/shopping_coupon.twig
  • Service/CouponService.php
  • Service/PurchaseFlow/Processor/CouponProcessor.php
  • Service/PurchaseFlow/Processor/CouponStateProcessor.php
  • Tests/Fixtures/CreateCouponTrait.php
  • Tests/Form/Type/CouponTypeTest.php
  • Tests/Repository/CouponCouponOrderRepositoryTest.php
  • Tests/Repository/CouponCouponRepositoryTest.php
  • Tests/Service/CouponServiceTest.php
  • Tests/Service/PurchaseFlow/CouponProcessorTest.php
  • Tests/Service/PurchaseFlow/Processor/CouponProcessorTest.php
  • Tests/Web/Admin/CouponControllerTest.php
  • Tests/Web/Admin/CouponSearchModelControllerTest.php
  • Tests/Web/Admin/OrderControllerTest.php
  • Tests/Web/CouponControllerTest.php
  • Tests/bootstrap.php
  • composer.json
  • docker-compose.dev.yml
  • docker-compose.mysql.yml
  • docker-compose.pgsql.yml
  • docker-compose.yml
  • dockerbuild/dev-framework.yaml
  • dockerbuild/grant_to_dbuser.sql
  • phpstan.neon.dist
  • phpunit.xml.dist
💤 Files with no reviewable changes (1)
  • Tests/Service/PurchaseFlow/CouponProcessorTest.php

Comment thread Controller/CouponShoppingController.php
Comment thread Repository/CouponOrderRepository.php
Comment thread Service/CouponService.php Outdated
PR #197 の CodeRabbit 指摘を裏どりのうえ対応。

- CouponShoppingController の #[Template] を他画面と同じ名前空間表記
  (@Coupon44/default/shopping_coupon.twig) に統一。旧表記でも描画は可能
  (Playwright / web テストで確認済み) だが解決方式を揃える。
- CouponService::saveCouponOrder の $discount を int から string|int|float へ。
  recalcOrder() の戻り値(小数通貨では小数を含む DECIMAL 文字列)を int で受けると
  小数が切り捨てられるため、CouponOrder::setDiscount と型を揃える(JPY では影響なし)。
- CI (ci.yml): GITHUB_TOKEN を最小権限 (contents: read) に。全 actions/checkout に
  persist-credentials: false を付与 (zizmor 指摘のクレデンシャル永続化対策)。
- OrderControllerTest のスキップに本体側追跡用の TODO/参照を追記。

備考: CouponOrderRepository::save(): void の非互換指摘は誤り。本体
AbstractRepository::save(AbstractEntity $entity): void と完全一致(裏どり済み)。

検証: PHPUnit 118 OK / php-cs-fixer 0 / rector 変更なし / phpstan level 6 エラー0。
クーポン利用画面の描画とトグルは Playwright で再確認。

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

Copy link
Copy Markdown
Author

@coderabbitai レビューありがとうございます。裏どりのうえ対応しました(commit bf5a9bf)。

Actionable / インライン(各スレッドに返信済み)

  • CouponShoppingController の #[Template]@Coupon44/default/shopping_coupon.twig に統一(旧表記でも実描画されることは Playwright / web テストで確認済みですが、表記を揃えました)。
  • CouponOrderRepository::save(): void の非互換指摘 → 修正不要。本体 AbstractRepository::save(AbstractEntity $entity): void と完全一致で互換です(前提の : mixed は該当バージョンには当てはまりません)。
  • saveCouponOrder(int $discount)string|int|float に変更し recalcOrder() / CouponOrder::setDiscount と型統一(小数通貨での桁落ち防止)。

Nitpick

  • ci.yml: permissions: contents: read を追加、全 actions/checkoutpersist-credentials: false を付与。
  • OrderControllerTest のスキップに本体側追跡用の TODO/参照を追記。

検証: PHPUnit 118 OK / php-cs-fixer 0 / rector 変更なし / phpstan level 6 エラー0。クーポン利用画面の描画とトグルは Playwright で再確認済みです。

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

@ttokoro20240902 対応ありがとうございます。それぞれ確認しました。

  • #[Template] パスの統一、saveCouponOrder の型統一(string|int|float)については意図通りの修正で問題ありません。
  • CouponOrderRepository::save(): void の件は、AbstractRepository::save(AbstractEntity $entity): void のシグネチャと完全に一致しているとのご説明、承知しました。当方の指摘の前提(: mixed)が該当バージョンに当てはまらなかったとのことで、こちらの認識が誤っていました。修正不要という判断で問題ありません。
  • ci.ymlpermissions: contents: read および persist-credentials: false の追加、OrderControllerTest への TODO 追記も確認しました。良い改善です。
  • PHPUnit / php-cs-fixer / rector / phpstan の検証結果、および Playwright での再確認、ありがとうございます。

インラインの各スレッドについても個別に確認します。何か追加の懸念が出てきた場合は改めてお知らせします。

(=^・ω・^=)

@dotani1111

Copy link
Copy Markdown

@ttokoro20240902
PRありがとうございます!

1点確認です。
CouponService::existsDepthCategory()ですが、
旧コードはバグっていて、修正したという認識であっていますか?

新コード:
return $this->existsDepthCategory($targetCategoryIds, $Category->getParent());  
→ 祖先を正しく再帰。

「カテゴリクーポンが対象カテゴリの子孫商品にも効く」というテストを入れて頂きたいです!

@dotani1111 dotani1111 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.

4.4/Symfony7 対応をレビュー・実機検証しました。Coupon42→44 の移行、および購入フロー関与部分(#[ShoppingFlow]/#[OrderFlow] 登録・値引き明細 OrderItemType::DISCOUNT 税率0%保持・小数通貨対応の型調整)を確認。CI 全 green。実機で「全商品500円割引クーポン作成→フロント会員購入で適用」を通し、値引き -500円・お支払い合計が正しく減額されることを実数で確認しました。Blocker なし。LGTM 👍

@nanasess nanasess left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

4.4 環境(PHP 8.5.4 / Symfony 7.4.13 / Doctrine ORM 3.6.2 / DBAL 4.4.3 / SQLite)へ本 PR のブランチをインストールし、実機で動作確認したうえでレビューしました。

クーポンの登録・一覧・有効/無効切り替え、購入フローでのクーポン適用(値引き・税再計算・注文確定・利用履歴)を確認し、単一配送の購入導線は完全に動作しました。移行の網羅性も高く、テストのデータプロバイダ static 化など PHPUnit 10+ 対応も適切です。

ただし クーポンを適用した状態で「お届け先の複数指定」を確定すると購入フローが 500 になります(下記コメント参照)。本体 4.4 で TaxRuleService::calcTax() に型が付いたことに追随できていないのが原因で、4.4 対応 PR としては対応が必要と考え request-changes とさせてください。

実機動作検証の結果

検証項目 結果
クーポン新規登録 ✅ 「クーポンを登録しました。」
クーポン一覧 ✅ 残発行枚数が 99/100 に減算
有効/無効の切り替え(PUT + CSRF) ✅ 「クーポンの状態を変更しました。」
購入フローへのクーポン欄差し込み ✅ ご注文手続き画面に表示
クーポンコード適用 ✅ 合計 ¥4,080 → 値引 -¥500 → 支払 ¥3,580、消費税も ¥325 に再計算
注文確定 ✅ 受注 payment_total=3580 / discount=500
クーポン利用履歴 plg_coupon_order に記録
複数配送 + クーポン 500
ページ管理からのテンプレート編集 反映されない

良い点

  • 単一配送のクーポン適用が値引き・税再計算・注文確定・利用履歴まで完全に動作する
  • クーポンの有効/無効切り替えが PUT + CSRF トークンで実装されている
  • テストのデータプロバイダが public functionpublic static function に正しく変更されている(PHPUnit 10+ で必須)
  • permissions: contents: read / persist-credentials: false など CI のハードニングが入っている

検証の過程で取り下げた指摘

  • existsDepthCategory() の親カテゴリ遡上を有効化すると割引適用範囲が変わるのでは: 本体は商品保存時に $Category->getPath() で祖先カテゴリまで dtb_product_category に登録します(ProductController.php:544 / CsvImportController.php:1199)。実 DB でも商品がカテゴリ 4(3→1 の子孫)に対し 1,2,3,4 全行を保持していることを確認したため、親遡上は標準データでは冗長で、金額は変わりません。修正自体は docblock の意図どおりで正しいと判断しました
  • PHPStan level 6 で expects string, string|null given が出るのでは: PHPStan 2.1.44 で最小再現を作り level 5/6/7/8 を実測したところ、この argument.typelevel 8 でのみ報告されました。PR の「level 6 エラーゼロ」と整合します

Comment thread Service/CouponService.php
public function isLowerLimitCoupon($productCoupon, $lowerLimitMoney)
public function isLowerLimitCoupon(array $productCoupon, int $lowerLimitMoney): bool
{
$subTotal = 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[major] 複数配送 + クーポン利用で購入フローが 500 になりますisLowerLimitCoupon() / 214 行の calcTax() 呼び出し)

getCouponProducts()(402 行)は rounding_type_id に明示的に null を入れ得ます。

'rounding_type_id' => $orderItem->getRoundingType() ? $orderItem->getRoundingType()->getId() : null,

本体 4.3 → 4.4 で calcTax() に型が付いたことが引き金です。

// upstream/4.3
public function calcTax($price, $taxRate, $RoundingType, $taxAdjust = '0')
// 4.4
public function calcTax(string $price, string $taxRate, int $RoundingType, string $taxAdjust = '0'): string

再現手順(4.4 実機で確認済み)

  1. 商品を数量 2 でカートに入れ、ゲスト購入で「ご注文手続き」まで進む
  2. クーポンコードを適用する(-¥500 が反映される)
  3. 「お届け先を追加する」→ お届け先の複数指定画面
  4. 「選択したお届け先に送る」をクリック

→ 「システムエラーが発生しました。」画面になり、購入を続行できません。

front.ERROR - ["Eccube\\Service\\TaxRuleService::calcTax(): Argument #3 ($RoundingType)
  must be of type int, null given, called in .../Service/CouponService.php on line 214"]

#0 Service/CouponService.php(214)
#1 Service/PurchaseFlow/Processor/CouponProcessor.php(160)
#2 src/Eccube/Service/PurchaseFlow/ItemHolderValidator.php(28)
#3 src/Eccube/Service/PurchaseFlow/PurchaseFlow.php(150)
#4 src/Eccube/Controller/AbstractShoppingController.php(39)
#5 src/Eccube/Controller/ShippingMultipleController.php(266)

到達経路(本体コードで確認)

  1. ShippingMultipleController が既存の Shipping / OrderItem を全削除(162-172 行)
  2. RoundingType を設定せずに OrderItem を作り直す(237-245 行)
  3. その直後に executePurchaseFlow($Order, false)(265 行)
  4. PurchaseFlow::validate()itemHolderValidators(149 行)を itemHolderPreprocessors(164 行)より先に実行する
  5. → RoundingType を設定する TaxProcessorItemHolderPreprocessor)より前に CouponProcessor::validate() が走る

recalcOrder()(178 行)には rounding_type_id === null のフォールバックがありますが、foreach が値渡しのため $couponProducts に書き戻されず、同じ配列を受け取る isLowerLimitCoupon() にはフォールバックがありません。

修正案: フォールバックを private メソッドに切り出し、recalcOrder()isLowerLimitCoupon() の両方で使う。

private function resolveTaxValues(int $productClassId, array $value): array
{
    if ($value['tax_rate'] < 1 || $value['rounding_type_id'] === null) {
        $ProductClass = $this->productClassRepository->find($productClassId);
        $TaxRule = $this->taxRuleRepository->getByRule($ProductClass->getProduct(), $ProductClass);
        $value['tax_rate'] = $TaxRule->getTaxRate();
        $value['rounding_type_id'] = $TaxRule->getRoundingType()->getId();
    }

    return $value;
}

あわせて testValidateWithLowerLimitrounding_type_id = null(RoundingType 未設定の OrderItem)ケースを追加すると退行を防げます。

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ご指摘のとおりでした。裏どりして修正しました(ae08221)。

CouponServiceresolveTaxValues() を切り出し、recalcOrder()isLowerLimitCoupon() の両方から呼ぶようにしました。ご提案の形に加えて、ProductClass が解決できない場合はデフォルト課税規則にフォールバックする null 安全化を入れています。

実機で before / after を確認しました(4.4 / PHP 8.2.31 / SQLite、ゲスト購入 + 数量2 + クーポン適用 → お届け先の複数指定 → 選択したお届け先に送る)。

  • 修正前(isLowerLimitCoupon() のフォールバックのみ外した状態):
    Eccube\Service\TaxRuleService::calcTax(): Argument #3 ($RoundingType) must be of type int, null given, called in .../Service/CouponService.php on line 209500
  • 修正後: 注文手続き画面へ正常に戻り、クーポン(-¥500)も維持。そのまま注文確定まで到達(payment_total=6660 / discount=500plg_coupon_order 記録、利用枚数 100→99)

テストも追加しました。

  • CouponServiceTest::testIsLowerLimitCouponWithoutRoundingType: RoundingType 未設定の OrderItem から existsCouponProduct() 経由で得た配列(rounding_type_id が null)で計算できること
  • CouponProcessorTest::testValidateWithoutRoundingType: 同状態で validate() が失敗しないこと(複数配送導線の回帰ガード)

Comment thread Controller/CouponShoppingController.php Outdated
* @see https://github.com/EC-CUBE/coupon-plugin/issues/128
*/
#[Route(path: '/plugin/coupon/shopping/shopping_coupon', name: 'plugin_coupon_shopping')]
#[Template('@Coupon44/default/shopping_coupon.twig')]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[major] @Coupon44/... へのテンプレート参照変更で、ページ管理からの編集が反映されなくなっています

// 移行前
@Template("Coupon42/Resource/template/default/shopping_coupon.twig")
// 移行後
#[Template('@Coupon44/default/shopping_coupon.twig')]

この 2 つは 解決先が異なります

参照形式 解決先
@Coupon44/default/shopping_coupon.twig app/Plugin/Coupon44/Resource/template/default/shopping_coupon.twig(プラグイン本体)
Coupon44/Resource/template/default/shopping_coupon.twig(移行前の形式) app/template/default/Coupon44/...テーマ側

dtb_page には 名前空間なしのパスで登録されています。

$ sqlite3 var/eccube.db "select id, file_name, url from dtb_page where file_name like '%Coupon44%';"
98|Coupon44/Resource/template/default/shopping_coupon|plugin_coupon_shopping

本体の PageController$templatePath.'/'.$Page->getFileName().'.twig'eccube_theme_front_dir = app/template/default)に dumpFile() するため、ページ管理で編集した内容はテーマ側に保存されるのに、コントローラはプラグイン本体を読みます

実機で確認: 管理画面のページ管理(/admin/content/page/98/edit)でテンプレートに目印を追記して保存したところ、

$ ls -la app/template/default/Coupon44/Resource/template/default/
-rw-r--r-- 1 nanasess nanasess 2987 shopping_coupon.twig   # ← 新規作成された

$ grep -rl "COUPON-PAGE-EDIT-MARKER-98765" app/ app/Plugin/Coupon44/
app/template/default/Coupon44/Resource/template/default/shopping_coupon.twig

保存先はテーマ側のみで、コントローラが読むプラグイン本体側は変わりません。つまり店舗側は「編集したのに反映されない」状態になります

(同じ問題が EC-CUBE/ProductReview-plugin#95 にもあります。)

修正案: いずれかに統一してください。

  • (a) #[Template] の参照を移行前と同じ名前空間なし形式に戻す
  • (b) @Coupon44/... を維持するなら、dtb_page への登録を廃止し、ページ管理の対象外であることを明示する

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ご指摘のとおりでした。名前空間なし表記に戻しました(ae08221)。

もともと f613669 では移行前と同じ名前空間なし表記でしたが、bf5a9bf で CodeRabbit の「表記統一」指摘に対応した際に @Coupon44/... へ変えてしまったのが原因です。当時「旧表記でも描画される」ことは確認していましたが、ページ管理経由の上書きを見落としていました。失礼しました。

修正案 (a) を採用し、再発防止のため理由をコメントで明記しています。

// dtb_page.file_name (PluginManager::createPage で登録) と一致させるため, あえて名前空間
// (@Coupon44) を使わない。名前空間なしの場合はテーマ側 (app/template/default/...) が
// プラグイン本体 (app/Plugin) より優先して解決されるため, 管理画面のページ管理で編集した
// 内容が反映される。@Coupon44 にするとプラグイン本体に固定解決され, ページ管理での編集が
// 無視されてしまうので変更しないこと。
#[Template('Coupon44/Resource/template/default/shopping_coupon.twig')]

実機確認: twig のパス解決順が app/template/default(テーマ)→ src/Eccube/Resource/template/defaultapp/Plugin であることを debug:config twig paths で確認し、テーマ側に app/template/default/Coupon44/Resource/template/default/shopping_coupon.twig(ページ管理の保存先)を置いた状態でクーポン入力画面を開くと、テーマ側の内容が描画されることを確認しました(修正前は @Coupon44 によりプラグイン本体側が描画されていました)。

なお Event.phpaddSnippet('@Coupon44/...') は移行前も @Coupon42/...dtb_page 管理外のため、変更していません。

* @param CouponOrder $CouponOrder
*/
public function save($CouponOrder)
public function save(AbstractEntity $CouponOrder): void

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[minor] ORM 3 の scoped flush 廃止で、フロー途中の flush が UnitOfWork 全体に波及しています

4.3 では $em->flush($CouponOrder) と対象エンティティだけをフラッシュしていましたが、ORM 3 でスコープ付き flush が廃止されたため $em->flush() に置き換わっています。これ自体は避けられない変更です。

ただし影響は save() だけではなく、移行前は以下がすべて scoped flush でした。

ファイル 移行前
Repository/CouponOrderRepository.php:47 $em->flush($CouponOrder)
Service/PurchaseFlow/Processor/CouponStateProcessor.php:112 flush($Coupon)
Service/PurchaseFlow/Processor/CouponStateProcessor.php:128 flush($Coupon)
Service/PurchaseFlow/Processor/CouponProcessor.php:196 flush($CouponOrder)
Service/PurchaseFlow/Processor/CouponProcessor.php:203 flush($Coupon)

とくに CouponStateProcessor::process() は管理画面の受注編集フローの途中(ItemHolderPreprocessor)で呼ばれるため、後続の itemHolderPostValidator が失敗しても中間状態が確定してしまう経路が生まれます。

修正案: save() は親クラス(AbstractRepository::save = persist のみ)どおりに戻し、呼び出し側(CouponService::saveCouponOrder / CouponStateProcessor::process)で明示的に flush する方が副作用の所在が読みやすくなります。戻せない事情がある場合は saveAndFlush() のように親の契約と区別できる名前をご検討ください。

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ご指摘のとおりでした。save() は親 AbstractRepository::save() と同じ persist のみに戻し、flush は呼び出し側で明示するようにしました(ae08221)。

調べたところ、CouponStateProcessor の 2 箇所(103 / 124 行)は save 直後の 112 / 127 行で自前 flush しており、save() 内の flush は冗長でした。そのため実際に save() の flush に依存していたのは CouponService::saveCouponOrder の 1 箇所だけで、同所に $this->entityManager->flush(); を追加しています(CouponStateProcessor は変更なし)。

テスト側は save() 後に検索する 3 ケース(CouponCouponOrderRepositoryTesttestSave / testFindUseCouponNonMember / testGetCouponOrder)に明示的な flush を追加しました。

購入フロー(クーポン適用 → 注文確定)も実機で確認し、plg_coupon_order の記録・利用枚数の減算が正常であることを確認済みです。

// TODO 本体側 (EC-CUBE/ec-cube) で DAMA DoctrineTestBundle と StockReduceProcessor の
// 悲観ロックの非互換が解消され次第、markTestSkipped を外して再有効化する。
// 追跡: https://github.com/EC-CUBE/ec-cube (StockReduceProcessor / EntityManager::lock)
$this->markTestSkipped('本体 StockReduceProcessor の悲観ロックが DAMA テストトランザクションと非互換のためスキップ(プラグイン非依存)');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[minor] CANCEL 時のクーポン返却テストが全面 skip され、追跡 Issue 番号がありません

testOrderEditWithCouponCancelmarkTestSkipped で丸ごと無効化されています。移行前(origin/4.4)には存在し実行されていたテストなので、カバレッジとしては退行です。

skip 理由(StockReduceProcessor の悲観ロックがテストハーネスと非互換)は妥当と考えます(本体の app/config/eccube/services_test.yaml:12-13TransactionListener がテスト時に無効化されており、DBAL 4 のラッパ層でトランザクションが開始されないため UnitOfWork::lock() が失敗します)。

ただし CouponStateProcessor::process() の枚数戻し分岐は $orderStatusId == CANCEL || PROCESSING || RETURNED の or 条件なので、生きている testOrderEditWithCouponReturn(RETURNED)が同じ分岐を通ります。失われるのは CANCEL 固有の遷移経路の検証です。

お願い

  • (a) TODO の追跡先がリポジトリ URL のみで Issue 番号が無く、事実上追跡されない状態です。EC-CUBE/ec-cube 側に Issue を作成して番号を書いていただけますでしょうか
  • (b) Web リクエストを介さず CouponStateProcessor::process() を直接呼ぶユニットテスト(CANCEL の Order を渡し Coupon::getCouponUseTime() が +1 されることを検証)で代替できないでしょうか

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

両方対応しました(ae08221)。

(a) 本体側 Issue を作成しました: EC-CUBE/ec-cube#7016

起票にあたり、コンテナ内で当該テストのスキップを一時解除して実際の例外とスタックトレースを採取しました。

Doctrine\ORM\TransactionRequiredException: An open transaction is required for this operation.
vendor/doctrine/orm/src/UnitOfWork.php:2261
vendor/doctrine/orm/src/EntityManager.php:485
src/Eccube/Service/PurchaseFlow/Processor/StockReduceProcessor.php:82
src/Eccube/Service/OrderStateMachine.php:155 → 48
src/Eccube/Controller/Admin/Order/EditController.php:182

ご指摘の分析どおり、ORM 3 の UnitOfWork::lock() は DBAL Connection::isTransactionActive() を要求しますが、DAMA は StaticDriver::connect()ドライバ層の接続beginTransaction() するため false になり、さらに services_test.yamlTransactionListener が無効化されているため DBAL 層でもトランザクションが開かれない、という流れでした。テスト側の TODO コメントに Issue 番号を記載しています。

(b) 代替ユニットテストを追加しました: Tests/Service/PurchaseFlow/Processor/CouponStateProcessorTest

Web リクエストを介さず CouponStateProcessor::process() を直接呼び、以下を検証しています。

  • キャンセル / 返品で Coupon::getCouponUseTime() が +1、CouponOrderorder_change_status が true・order_date が null になること
  • 戻しが発行枚数で頭打ちになること
  • 戻し済みの受注を対象外ステータスへ戻すと再度使用状態になること(-1)

なお実装を読み進める過程で、process() の分岐条件にある PROCESSINGsupports() が対象ステータスを NEW / PAID / IN_PROGRESS / DELIVERED / CANCEL / RETURNED に限定しているため到達しないことが分かりました(実質デッドコード)。この挙動も testProcessWithUnsupportedOrderStatus(購入処理中 / 決済処理中)で明文化しています。

$this->assertEquals(-1000, $OrderItem->getPrice());
$this->assertEquals($Coupon->getCouponName(), $OrderItem->getProductName());
// クーポン値引き明細は不課税で追加される
$this->assertSame(TaxType::NON_TAXABLE, $OrderItem->getTaxType()->getId());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[minor] 旧 CouponProcessorTest にあった isDiscount() のアサーションが移行先にありません

削除された Tests/Service/PurchaseFlow/CouponProcessorTest.php には self::assertTrue($DiscountItem->isDiscount()) があり、追加される明細の OrderItemType が「値引き」であることを検証していました。

TaxType::NON_TAXABLE の検証は移行先の 3 ケース(153 / 208 / 234 行)へ移植されていますが、isDiscount()OrderItemType::DISCOUNT)の検証は移植されていません。

addCouponDiscountItem()setOrderItemType($DiscountType) を行っているので、ここが壊れると受注金額の集計(値引き明細の合算)が狂いますが、現状のテストは processorName / price / productName / taxType しか見ていないため検知できません。

修正案: self::assertTrue($OrderItem->isDiscount(), '値引き明細として追加されている'); を追加。

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ご指摘のとおり移植漏れでした。3 ケース(153 / 208 / 234 行付近)すべてに追加しました(ae08221)。

// OrderItemType が「値引き」で追加される (受注金額の集計に影響する)
$this->assertTrue($OrderItem->isDiscount(), '値引き明細として追加されている');

Comment thread .github/workflows/ci.yml Outdated
- 3306:3306
options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=3
mysql8:
image: mysql:8

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[minor] CI の DB イメージが 4.4 のサポート範囲とずれています

場所 MySQL PostgreSQL
.github/workflows/ci.yml mysql:8 postgres:14
docker-compose.mysql.yml(本 PR で追加) mysql:8.4
docker-compose.pgsql.yml(本 PR で追加) postgres:18
本体 unit-test.yml 8.4 18 / 13

同一 PR 内で CI とローカル検証環境の DB バージョンが食い違っています。移行前の ci.yml から引き継がれた設定で本 PR が壊したものではありませんが、4.4 対応 PR なので合わせておくのが自然かと思います。

なお database_server_version: 8(MySQL)は本体 4.4 と同値なので変更不要です。ずれているのは image と postgres 側です。

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ご指摘のとおり、同一 PR 内で食い違っていました。本体 4.4 に合わせました(ae08221)。

  • services mysql8.image: mysql:8mysql:8.4
  • services postgres.image: postgres:14postgres:18
  • matrix pgsql database_server_version: 1418
  • matrix mysql8 database_server_version: 8 は本体 4.4 と同値のため変更していません

Comment thread .github/workflows/ci.yml

- name: Setup PHP
uses: shivammathur/setup-php@v2
uses: nanasess/setup-php@master

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[question] extensions: ':xdebug'(xdebug 無効化)の指定が落ちています

Setup PHP ステップから、元々あった extensions: ':xdebug' の指定が落ちています(static-analysis ジョブ側も同様)。xdebug が有効なままだとテスト実行が大幅に遅くなるため、意図的な変更かご確認ください。

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

意図的な変更ではなく、移行時の取りこぼしでした。両ジョブ(run-on-linux / static-analysis)の Setup PHP に復活させました(ae08221)。

      - name: Setup PHP
        uses: nanasess/setup-php@master
        with:
          php-version: ${{ matrix.php }}
          # xdebug が有効だとテスト実行が大幅に遅くなるため無効化する
          extensions: ':xdebug'

Comment thread Event.php Outdated
use Plugin\Coupon42\Entity\Coupon;
use Plugin\Coupon42\Repository\CouponOrderRepository;
use Plugin\Coupon42\Repository\CouponRepository;
use Plugin\Coupon44\Entity\Coupon;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[nit] 未使用の use が残っています

use Plugin\Coupon44\Entity\Coupon;

Event.php 内で Coupon クラスは一切参照されていません(コード・docblock とも)。移行前はコンストラクタで CouponRepository などを受け取っていたので、その整理の際の取り残しかと思います。

php-cs-fixer(no_unused_imports を含む @Symfony)でも検出されなかったため CI は通りますが、不要です。

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ご指摘のとおり未使用でした(Coupon の出現は log_info の文字列のみ)。削除しました(ae08221)。

@@ -451,7 +451,7 @@
</div>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[nit] Bootstrap 5 に存在しない mr-2 / mr-3 が残っています(450 行ほか)

Bootstrap 5.3 では右マージンのユーティリティは me-* にリネームされているため、これらは何のスタイルも当たらず、ボタン/アイコンの余白が詰まって表示されます(html/template/admin/assets/css/bootstrap.css.me-2 は存在し .mr-2 のルールは存在しないことを確認済み。app.css の 1 ヒットは pug サンプルのコメント内でスタイル定義ではありません)。

該当は regist.twig の 411 / 450 / 473 行と、regist_product_list_prototype.twig:17 / regist_category_list_prototype.twig:16 です。同じ regist.twig の 391 行だけが me-2 になっており不統一です。

移行前(origin/4.4)から存在する既存の不具合で本 PR が壊したものではなく、本体側テンプレートにも同種の残骸があるため優先度は低いですが、Bootstrap 5.3 世代への移行 PR なので、ついでに直しておくと良さそうです。(別 PR での対応でも構いません。)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ご指摘のとおりでした。本 PR に同梱して 5 箇所すべて修正しました(ae08221)。

  • regist.twig 411 / 473 行: mr-3me-3
  • regist.twig 450 行: mr-2me-2
  • regist_product_list_prototype.twig:17 / regist_category_list_prototype.twig:16: mr-3me-3

html/template/admin/assets/css/bootstrap.css.mr-2 のルールが 0 件、.me-2 が存在することも確認しました。また ml-* / pr-* / float-left / badge-* / sr-only など他の Bootstrap 4 残骸は grep で 0 件でした。

default ブランチ (4.2) にも同じ 5 箇所があるため、別 PR を出しました: #200

nanasess のレビュー指摘 9 件を実機・コード両面で裏どりのうえ対応。

■ 複数配送 + クーポンで購入フローが 500 になる問題を修正
CouponService に resolveTaxValues() を切り出し、recalcOrder() と
isLowerLimitCoupon() の両方で税率・丸め規則のフォールバックを行う。
ShippingMultipleController は RoundingType を設定せずに OrderItem を作り直し、
RoundingType を設定する TaxProcessor (ItemHolderPreprocessor) より先に
itemHolderValidator が走るため rounding_type_id が null で渡る。本体 4.4 の
TaxRuleService::calcTax() は第3引数が非 nullable な int のため TypeError になっていた。
実機で「修正前は 500 / 修正後は正常」を確認済み。

■ ページ管理からのテンプレート編集が反映されない問題を修正
CouponShoppingController の #[Template] を名前空間なし表記に戻す。dtb_page には
名前空間なしのパスが登録されており、ページ管理の保存先はテーマ側
(app/template/default/...)。@Coupon44 はプラグイン本体に固定解決されるため
編集が無視されていた。再発防止のため理由をコメントで明記。

■ CouponOrderRepository::save() の flush を呼び出し側へ
ORM 3 でスコープ付き flush が廃止され flush() が UnitOfWork 全体に波及するため、
save() は親 AbstractRepository と同じ persist のみに戻し、
CouponService::saveCouponOrder で明示的に flush する。CouponStateProcessor は
save 直後に自前 flush があるため変更不要(内部 flush は冗長だった)。

■ テスト
- CouponStateProcessorTest を新規追加。キャンセル/返品時のクーポン枚数戻し、
  発行枚数での頭打ち、ステータス復帰時の再使用を Web リクエストを介さず検証。
  supports() の対象外ステータス (購入処理中/決済処理中) は process() に到達しない
  ことも明文化(process() 内の PROCESSING 条件は実質デッドコード)
- CouponProcessorTest に isDiscount() の検証を追加(旧テストからの移植漏れ)
- CouponProcessorTest / CouponServiceTest に RoundingType 未設定ケースを追加
- OrderControllerTest のスキップ理由に本体側 Issue 番号 (EC-CUBE/ec-cube#7016) を記載

■ CI
- DB イメージを 4.4 のサポート範囲に合わせる (mysql:8.4 / postgres:18)
- Setup PHP に extensions: ':xdebug' を復活(テスト実行の高速化)

■ その他
- Event.php の未使用 import (Entity\Coupon) を削除
- Bootstrap 5 に存在しない mr-2 / mr-3 を me-2 / me-3 に修正(既存不具合)

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

Copy link
Copy Markdown
Author

@nanasess レビューありがとうございました。実機での再現手順まで添えていただき助かりました。指摘 9 件すべてを裏どりのうえ対応しました(ae08221)。各スレッドに個別に返信していますが、要点は以下です。

指摘 対応
[major] 複数配送 + クーポンで 500 resolveTaxValues() を切り出し recalcOrder() / isLowerLimitCoupon() の双方でフォールバック。修正前の 500 を実機で再現 → 修正後に解消を確認。回帰テスト 2 件追加
[major] ページ管理の編集が反映されない #[Template]dtb_page と一致する名前空間なし表記に戻す。再発防止の理由をコメント明記。テーマ側の内容が描画されることを実機確認
[minor] save() の flush スコープ AbstractRepository::save() と同じ persist のみに戻し、flush は saveCouponOrder で明示。CouponStateProcessor は save 直後に自前 flush があり変更不要でした
[minor] スキップテストの追跡 本体側に Issue 起票(EC-CUBE/ec-cube#7016、実際の例外トレース添付)。TODO に番号記載。代替ユニットテスト CouponStateProcessorTest を追加
[minor] isDiscount() の移植漏れ 3 ケースに追加
[minor] CI の DB イメージ mysql:8.4 / postgres:18 に統一(database_server_version も pgsql は 18 に)
[question] extensions: ':xdebug' 移行時の取りこぼしでした。両ジョブに復活
[nit] 未使用 import 削除
[nit] mr-*me-* 5 箇所修正。4.2 向けは別 PR #200

補足: process()PROCESSING 分岐はデッドコードでした

代替ユニットテストを書く過程で、CouponStateProcessor::process() の分岐条件にある OrderStatus::PROCESSING は、supports() が対象ステータスを NEW / PAID / IN_PROGRESS / DELIVERED / CANCEL / RETURNED に限定しているため到達しないことが分かりました。本 PR では挙動を変えず、testProcessWithUnsupportedOrderStatus(購入処理中 / 決済処理中)でこの事実を明文化しています(整理は別途)。

検証結果

  • PHPUnit: 127 tests / 196 assertions パス(Failures/Errors 0、Skipped 1)
  • php-cs-fixer: 差分ゼロ / rector: 変更ゼロ / phpstan level 6: エラーゼロ
  • 実機: 複数配送 + クーポン(before/after)、ページ管理の反映、単一配送での注文確定(payment_total=6660 / discount=500plg_coupon_order 記録、利用枚数 100→99)

再レビューをお願いできますでしょうか。

本体 4.4 の依存が doctrine/dbal 4.4.4 に上がり、SQLite では
SQLitePlatform::getCreateDatabaseSQL が未サポートで例外になるため、
static-analysis ジョブ (SQLite) が Setup EC-CUBE で失敗していた。

  Could not create database var/eccube.db for connection named default
  Operation "Doctrine\DBAL\Platforms\SQLitePlatform::getCreateDatabaseSQL"
  is not supported by platform.

DB ファイルは doctrine:schema:create の接続時に生成されるため、本体
4.4 の unit-test.yml と同様に SQLite では database:create をスキップする。
MySQL / PostgreSQL の PHPUnit ジョブは従来どおり database:create を実行する。

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

@nanasess nanasess left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

再確認しました — LGTM

major 2 件を含む 9 件すべてにご対応いただき、ありがとうございます。major と、挙動が変わる flush の除去は実機で確認しました。

検証環境: EC-CUBE 4.4.0-dev (89dec55c) / PHP 8.5.4 / Symfony 7.4.13 / Doctrine ORM 3.6.2 / DBAL 4.4.3 / SQLite / APP_ENV=prod / symfony server (HTTPS)

実機検証: [major] 複数配送 + クーポンで購入フローが 500

再現手順をそのまま再実行しました。

  1. 商品を数量 2 でカートに入れ、ゲスト購入で「ご注文手続き」へ
  2. クーポン TEST44COUPON を適用(-¥500 が反映)
  3. 「お届け先を追加する」→ お届け先の複数指定画面
  4. 「選択したお届け先に送る」

500 にならず購入手続き画面に戻り、そのまま注文完了まで通りましたdtb_order: total=6660 / discount=500plg_coupon_order にレコード生成)。

resolveTaxValues() への切り出しに加えて、ProductClass が解決できない場合に getByRule() の既定課税規則へフォールバックする null 安全化まで入れていただいた点は、当初の提案より堅い実装だと思います(getByRule() は 4.4 本体で全引数 nullable なので引数なし呼び出しも正当です)。回帰テスト testValidateWithoutRoundingType の追加もありがとうございます。

実機検証: [major] テンプレート参照の名前空間化

テーマ側 app/template/default/Coupon44/Resource/template/default/shopping_coupon.twig に目印を入れて /plugin/coupon/shopping/shopping_coupon を開いたところ、目印が表示されましたdtb_page.file_nameCoupon44/Resource/template/default/shopping_coupon)と一致しています。

管理画面側は @Coupon44/... のまま残し、ページ管理の対象であるフロントだけ名前空間なしにする使い分けも適切です。「変更しないこと」の理由をコメントで明記いただいたので、再発も防げると思います。

実機検証: [minor] save() からの flush 除去

AbstractRepository::save() と同じ persist のみに戻した影響を確認しました。

操作 結果
クーポン利用で注文完了 plg_coupon_order にレコード生成(discount=500 / order_date セット)・coupon_use_time 99 → 98
管理画面で受注を「注文取消し」に変更 coupon_use_time 98 → 99 に返却order_change_status=1 / order_date=null

CouponStateProcessor の 2 箇所は直後に自前 flush() があるため影響がなく、saveCouponOrder() にだけ flush() を追加する対応で正しく動作しています。

そのほかの指摘

指摘 対応
[minor] CANCEL 時のクーポン返却テストの skip 本体 issue #7016 を起票・ユニットテストで代替する旨を明記
[minor] isDiscount() のアサーション欠落 3 ケースすべてに追加
[minor] CI の DB イメージ mysql:8.4 / postgres:18
[question] extensions: ':xdebug' 両ジョブに復活
[nit] 未使用の use 削除
[nit] Bootstrap 4 の mr-2 / mr-3 5 箇所すべて me-* に修正

#7016 の起票にあたって実際にスキップを解除し、TransactionRequiredException のスタックトレースを採取いただいた点は、本体側での調査の助けになると思います。

CI は 10 ジョブ green を確認しました。approve します。

@ttokoro20240902
ttokoro20240902 merged commit 0add16d into 4.4 Aug 7, 2026
10 checks passed
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.

3 participants