feat: FAQ機能を追加 (#6148) - #6980
Conversation
サイト共通 / 商品ごと / カテゴリごとの3区分でFAQを管理・表示し、 AI・構造化データ向けに FAQPage の JSON-LD を出力する機能を追加する。 - Faq エンティティ(dtb_faq) と Product/Category への OneToMany を追加 - サイト共通FAQ: 管理画面 Content > FAQ の専用CRUD (FaqController) - 商品ごとFAQ: 商品編集画面にコレクションとして埋め込み - カテゴリごとFAQ: カテゴリFAQ専用ページ (CategoryFaqController) - FAQブロック(dtb_block)を追加し use_controller で描画 - FaqStructuredDataService で FAQPage の JSON-LD を生成し、 商品詳細/カテゴリ一覧/FAQブロックで |json_ld フィルタ経由で出力 - フロント表示スタイル(ec-faqRole)を追加 - PHPUnit: Service/Repository/管理CRUD/カテゴリFAQ の各テストを追加 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:
📝 WalkthroughWalkthroughFAQのデータモデル、共通・商品・カテゴリFAQの管理画面、フロント表示、FAQPage JSON-LD、ブロック登録、関連テストとスタイルを追加した。 ChangesFAQ機能
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ProductController
participant FaqRepository
participant FaqStructuredDataService
participant ProductTemplate
ProductController->>FaqRepository: 可視FAQを取得
FaqRepository-->>ProductController: FAQ一覧を返却
ProductController->>FaqStructuredDataService: FAQPage JSON-LDを生成
FaqStructuredDataService-->>ProductController: JSON-LDデータを返却
ProductController->>ProductTemplate: FAQ一覧とJSON-LDを渡す
ProductTemplate-->>ProductTemplate: FAQと構造化データを描画
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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: 6
🧹 Nitpick comments (3)
tests/Eccube/Tests/Web/Admin/Content/FaqControllerTest.php (1)
24-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win共通FAQ以外へのアクセス制御に対するテストカバレッジが手薄
FaqController::edit/deleteはFaq::getFaqType() !== Faq::FAQ_TYPE_COMMONの場合に404を返す仕様（FaqController.phpのedit/delete参照）ですが、その分岐を検証するテストがありません。商品・カテゴリFAQが誤って共通FAQ管理画面から編集・削除できてしまわないことの回帰防止として、そのケースのテスト追加を検討する価値があります。🤖 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/Eccube/Tests/Web/Admin/Content/FaqControllerTest.php` around lines 24 - 89, Extend the FAQ controller tests around testRoutingAdminContentFaqEdit and testRoutingAdminContentFaqDelete to create non-common FAQ records with Faq::getFaqType() different from Faq::FAQ_TYPE_COMMON, then assert both edit and delete requests return 404. Keep the existing common FAQ success-path tests unchanged.src/Eccube/Entity/Faq.php (2)
75-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuecreate_date/update_dateの型宣言がCategory等と不揃い
同PR内の
Category.phpではprivate ?\DateTime $create_date = null;のようにネイティブ型宣言とデフォルト値を付与していますが、本ファイルはphpdocのみで型宣言がありません。動作上は問題ありませんが、スタイルの一貫性のため揃えることを推奨します。♻️ 提案
- /** - * `@var` \DateTime - */ - #[ORM\Column(name: 'create_date', type: Types::DATETIMETZ_MUTABLE)] - private $create_date; - - /** - * `@var` \DateTime - */ - #[ORM\Column(name: 'update_date', type: Types::DATETIMETZ_MUTABLE)] - private $update_date; + #[ORM\Column(name: 'create_date', type: Types::DATETIMETZ_MUTABLE)] + private ?\DateTime $create_date = null; + + #[ORM\Column(name: 'update_date', type: Types::DATETIMETZ_MUTABLE)] + private ?\DateTime $update_date = null;🤖 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 `@src/Eccube/Entity/Faq.php` around lines 75 - 85, Faqエンティティのcreate_dateおよびupdate_dateプロパティに、Category.phpと同じネイティブ型宣言（nullableなDateTime）とnullのデフォルト値を追加し、既存のORMマッピングは維持してください。
212-225: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueProduct/Category両方が設定された場合の排他制御なし
getFaqType()はProductが優先され、Category・Product双方が同時に設定された場合の不整合を防ぐ仕組みがエンティティ側にありません。現状はCategoryFaqType/ProductTypeのfaqsコレクションがそれぞれ別々の関連にのみ追加する実装のため実害は出にくいですが、将来的な拡張やイベントリスナー経由の操作で両方セットされるケースに備え、setProduct/setCategory側での相互排他チェック、またはバリデーション制約の追加を検討する余地があります。🤖 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 `@src/Eccube/Entity/Faq.php` around lines 212 - 225, Update the FAQ entity’s setProduct and setCategory paths to enforce mutual exclusivity, preventing both Product and Category from being assigned simultaneously; alternatively add an entity validation constraint that rejects this state. Keep getFaqType’s existing type resolution unchanged for valid FAQ configurations.
🤖 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 `@app/DoctrineMigrations/Version20260727000000.php`:
- Around line 30-47: Update the down() method to return early when the dtb_block
table is absent, matching the existing up() guard before executing the DELETE
statement. Keep the deletion unchanged for environments where the table exists,
and ensure down() uses its Schema $schema parameter so the PHPMD
unused-parameter warning is resolved.
In `@html/template/default/assets/scss/style.scss`:
- Line 73: Move the project/23.1.faq import from its current position into the
initial contiguous import block in the stylesheet, preserving the intended
import order. Regenerate the compiled style.css so it reflects the updated SCSS
imports.
In `@src/Eccube/Controller/Admin/Content/FaqController.php`:
- Line 49: 型なしのルート引数を修正してください。FaqController の index() は $page_no を int $page_no
= 1 として宣言し、edit() は $id を ?int $id = null に変更してください。edit() 内のID有無の判定は truthiness
ではなく !== null を使用し、FaqType() へ渡す際はルート由来の値を適切に整数として扱ってください。
In `@src/Eccube/Controller/Block/FaqController.php`:
- Around line 39-43: src/Eccube/Controller/Block/FaqController.php#L39-L43 の
FaqController、src/Eccube/Controller/ProductController.php#L166-L175 のカテゴリ
FAQ取得処理、src/Eccube/Controller/ProductController.php#L227-L237 の商品
FAQ取得処理で、表示件数の上限を受け取る Repository メソッドを追加・利用し、全件取得をやめてください。各箇所の JSON-LD
生成にも同じ上限済み配列を渡してください。
In `@src/Eccube/Resource/template/admin/Content/faq.twig`:
- Around line 68-79: Update the delete control around the modal trigger to use a
button type="button" instead of an href-less anchor, preserving its existing
modal target and styling. Change the modal’s aria-labelledby value to reference
the delete modal heading’s unique id, and assign that id to the corresponding h5
title.
In `@src/Eccube/Resource/template/default/Block/faq.twig`:
- Around line 20-24:
FAQ回答を生のHTMLとして出力せず、src/Eccube/Resource/template/default/Block/faq.twigのFaq.answerをpurify経由に更新してください。あわせてsrc/Eccube/Resource/template/default/Product/detail.twigの450-454行およびsrc/Eccube/Resource/template/default/Product/list.twigの243-247行でも同じくFaq.answerをpurify経由で出力し、改行表示が必要な場合は既存の商品説明と同様にnl2brを組み合わせてください。
---
Nitpick comments:
In `@src/Eccube/Entity/Faq.php`:
- Around line 75-85:
Faqエンティティのcreate_dateおよびupdate_dateプロパティに、Category.phpと同じネイティブ型宣言（nullableなDateTime）とnullのデフォルト値を追加し、既存のORMマッピングは維持してください。
- Around line 212-225: Update the FAQ entity’s setProduct and setCategory paths
to enforce mutual exclusivity, preventing both Product and Category from being
assigned simultaneously; alternatively add an entity validation constraint that
rejects this state. Keep getFaqType’s existing type resolution unchanged for
valid FAQ configurations.
In `@tests/Eccube/Tests/Web/Admin/Content/FaqControllerTest.php`:
- Around line 24-89: Extend the FAQ controller tests around
testRoutingAdminContentFaqEdit and testRoutingAdminContentFaqDelete to create
non-common FAQ records with Faq::getFaqType() different from
Faq::FAQ_TYPE_COMMON, then assert both edit and delete requests return 404. Keep
the existing common FAQ success-path tests unchanged.
🪄 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 Plus
Run ID: 462add9c-162f-459f-b40a-832392c4ac6d
⛔ Files ignored due to path filters (3)
html/template/default/assets/css/style.min.cssis excluded by!**/*.min.csssrc/Eccube/Resource/doctrine/import_csv/en/dtb_block.csvis excluded by!**/*.csvsrc/Eccube/Resource/doctrine/import_csv/ja/dtb_block.csvis excluded by!**/*.csv
📒 Files selected for processing (34)
.claude/skills/repository/SKILL.mdapp/DoctrineMigrations/Version20260727000000.phpapp/config/eccube/packages/eccube_nav.yamlhtml/template/default/assets/css/style.csshtml/template/default/assets/scss/project/_23.1.faq.scsshtml/template/default/assets/scss/style.scsssrc/Eccube/Controller/Admin/Content/FaqController.phpsrc/Eccube/Controller/Admin/Product/CategoryFaqController.phpsrc/Eccube/Controller/Block/FaqController.phpsrc/Eccube/Controller/ProductController.phpsrc/Eccube/Entity/Category.phpsrc/Eccube/Entity/Faq.phpsrc/Eccube/Entity/Product.phpsrc/Eccube/Event/EccubeEvents.phpsrc/Eccube/Form/Type/Admin/CategoryFaqType.phpsrc/Eccube/Form/Type/Admin/FaqType.phpsrc/Eccube/Form/Type/Admin/ProductType.phpsrc/Eccube/Repository/FaqRepository.phpsrc/Eccube/Resource/locale/messages.en.yamlsrc/Eccube/Resource/locale/messages.ja.yamlsrc/Eccube/Resource/template/admin/Content/faq.twigsrc/Eccube/Resource/template/admin/Content/faq_collection.twigsrc/Eccube/Resource/template/admin/Content/faq_edit.twigsrc/Eccube/Resource/template/admin/Product/category.twigsrc/Eccube/Resource/template/admin/Product/category_faq.twigsrc/Eccube/Resource/template/admin/Product/product.twigsrc/Eccube/Resource/template/default/Block/faq.twigsrc/Eccube/Resource/template/default/Product/detail.twigsrc/Eccube/Resource/template/default/Product/list.twigsrc/Eccube/Service/FaqStructuredDataService.phptests/Eccube/Tests/Repository/FaqRepositoryTest.phptests/Eccube/Tests/Service/FaqStructuredDataServiceTest.phptests/Eccube/Tests/Web/Admin/Content/FaqControllerTest.phptests/Eccube/Tests/Web/Admin/Product/CategoryFaqControllerTest.php
レビュー指摘対応:
- フロント3テンプレの回答表示を {{ answer|raw }} から
{{ answer|purify|nl2br }} に変更（商品説明・News と同じ表示時サニタイズ）
- Faq::__clone() で id を null 化（商品複製で cascade persist されるため）
- 商品詳細のFAQ表示・非表示・FAQPage(JSON-LD)出力の Web テストを追加
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Eccube/Tests/Web/ProductFaqDisplayTest.php (1)
44-59: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win回答サニタイズの回帰テストを追加してください。
現在は通常の文字列しか検証していないため、
purify|nl2brが壊れても検出できません。<script>や許可対象外の HTML、改行を含む回答を追加し、危険な要素が除去され、改行が期待どおり表示されることを確認してください。🤖 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/Eccube/Tests/Web/ProductFaqDisplayTest.php` around lines 44 - 59, Update the ProductFaqDisplayTest fixture and assertions around createProductFaq to include an answer containing a script tag, disallowed HTML, and newline characters. Assert that the rendered .ec-faqRole output excludes dangerous/disallowed elements and preserves the expected HTML line-break representation, while keeping the existing visible/hidden FAQ checks.
🤖 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/Web/ProductFaqDisplayTest.php`:
- Around line 62-64: Update the callbacks assigned to $jsonLdList in
ProductFaqDisplayTest.php, including the matching callback around the second
referenced location, to type-hint the node parameter as
Symfony\Component\DomCrawler\Crawler; add the corresponding use import and
preserve the existing string return type and behavior.
---
Nitpick comments:
In `@tests/Eccube/Tests/Web/ProductFaqDisplayTest.php`:
- Around line 44-59: Update the ProductFaqDisplayTest fixture and assertions
around createProductFaq to include an answer containing a script tag, disallowed
HTML, and newline characters. Assert that the rendered .ec-faqRole output
excludes dangerous/disallowed elements and preserves the expected HTML
line-break representation, while keeping the existing visible/hidden FAQ checks.
🪄 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 Plus
Run ID: 378fb080-2b6a-476e-a4ec-9f3613c604f4
📒 Files selected for processing (5)
src/Eccube/Entity/Faq.phpsrc/Eccube/Resource/template/default/Block/faq.twigsrc/Eccube/Resource/template/default/Product/detail.twigsrc/Eccube/Resource/template/default/Product/list.twigtests/Eccube/Tests/Web/ProductFaqDisplayTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Eccube/Resource/template/default/Block/faq.twig
- src/Eccube/Resource/template/default/Product/detail.twig
- src/Eccube/Entity/Faq.php
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.4 #6980 +/- ##
==========================================
+ Coverage 77.18% 77.34% +0.15%
==========================================
Files 564 572 +8
Lines 28123 28469 +346
==========================================
+ Hits 21707 22018 +311
- Misses 6416 6451 +35
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:
|
- Faq Entity の if(!class_exists()) ガードを撤去し他 Entity と統一（24fa8523ea の全廃方針に追従） - FAQブロックの secHeading を他ブロックと同じ __en/__line/__ja の3点セットに統一（locale キーを title__en/title__ja に分割） - ProductType の faqs に orphanRemoval による全削除リスクの注意コメントを追記 - 商品編集経由の商品FAQ保存経路テスト（追加・更新・削除の混在）を追加 - FaqController の登録/削除の永続結果検証と、サイト共通以外での404ガードのテストを追加 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Eccube/Resource/template/default/Block/faq.twig (1)
22-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win表示側もJSON-LDと同じ有効FAQ判定に揃えてください。
現在は
Faqsが空でない限り全件を表示しますが、FaqStructuredDataServiceは question または answer が null/空文字のFAQを除外しています。不完全なデータが存在すると、空のFAQ項目だけが画面に表示されます。表示側でも同じ条件で絞り込むか、取得処理側で有効なFAQのみ返してください。根拠:
FaqStructuredDataServiceは空の question/answer をJSON-LD対象外にしています。🤖 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 `@src/Eccube/Resource/template/default/Block/faq.twig` around lines 22 - 25, Update the FAQ rendering loop in the Twig template to display only entries whose question and answer are both non-null and non-empty, matching FaqStructuredDataService’s validity criteria. Apply the filtering before rendering each ec-faqRole__item, while preserving the existing question and answer formatting for valid FAQs.
🧹 Nitpick comments (2)
tests/Eccube/Tests/Web/Admin/Content/FaqControllerTest.php (1)
118-145: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winカテゴリFAQ区分に対する404テストも追加を推奨。
Faq::getFaqType()（src/Eccube/Entity/Faq.php:219-229）はFAQ_TYPE_PRODUCT/FAQ_TYPE_CATEGORY/FAQ_TYPE_COMMONの3分岐を持ちますが、追加された404テストは商品FAQ（FAQ_TYPE_PRODUCT）のみを検証しており、カテゴリFAQ（FAQ_TYPE_CATEGORY）に対する共通FAQ編集/削除エンドポイントへのアクセス制御は未検証です。同様のアクセス制御ロジックに分岐漏れがあった場合、この分岐は現状のテストでは検出できません。🤖 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/Eccube/Tests/Web/Admin/Content/FaqControllerTest.php` around lines 118 - 145, FAQ_TYPE_CATEGORYのカテゴリFAQについても、testFaqEditReturns404ForNonCommonFaqおよびtestFaqDeleteReturns404ForNonCommonFaqと同様に共通FAQ編集・削除エンドポイントが404を返すテストを追加する。カテゴリFAQを作成する既存のヘルパーまたはフィクスチャを再利用し、削除テストでは404後にエンティティが残っていることも検証する。tests/Eccube/Tests/Web/Admin/Product/ProductControllerTest.php (1)
405-405: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
testEditWithProductFaq()に戻り値型宣言: voidが無い。同ファイル内の他の新しめのテストメソッド（例:
testExportWithFilterNoStock(): void,testEditWithOrderMemo(): void）は: voidを付与しています。🔧 修正案
- public function testEditWithProductFaq() + public function testEditWithProductFaq(): voidAs per coding guidelines, "PHP の引数と戻り値には型宣言を付け、PHPStan level 6 を通過させる。"
🤖 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/Eccube/Tests/Web/Admin/Product/ProductControllerTest.php` at line 405, Update the testEditWithProductFaq() method declaration to include the void return type, matching the typed test methods in the same class and the project’s PHP typing guidelines.Source: Coding guidelines
🤖 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 `@src/Eccube/Entity/Faq.php`:
- Around line 57-60: Update the Faq::__clone() method signature to declare the
PHP 8.2-compatible void return type, while preserving its existing behavior of
resetting id to null.
- Around line 180-202: Update setProduct() and setCategory() so assigning a
non-null Product clears Category and assigning a non-null Category clears
Product; preserve null assignments without altering the opposite relation,
ensuring each FAQ remains linked to at most one target type.
---
Outside diff comments:
In `@src/Eccube/Resource/template/default/Block/faq.twig`:
- Around line 22-25: Update the FAQ rendering loop in the Twig template to
display only entries whose question and answer are both non-null and non-empty,
matching FaqStructuredDataService’s validity criteria. Apply the filtering
before rendering each ec-faqRole__item, while preserving the existing question
and answer formatting for valid FAQs.
---
Nitpick comments:
In `@tests/Eccube/Tests/Web/Admin/Content/FaqControllerTest.php`:
- Around line 118-145:
FAQ_TYPE_CATEGORYのカテゴリFAQについても、testFaqEditReturns404ForNonCommonFaqおよびtestFaqDeleteReturns404ForNonCommonFaqと同様に共通FAQ編集・削除エンドポイントが404を返すテストを追加する。カテゴリFAQを作成する既存のヘルパーまたはフィクスチャを再利用し、削除テストでは404後にエンティティが残っていることも検証する。
In `@tests/Eccube/Tests/Web/Admin/Product/ProductControllerTest.php`:
- Line 405: Update the testEditWithProductFaq() method declaration to include
the void return type, matching the typed test methods in the same class and the
project’s PHP typing guidelines.
🪄 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 Plus
Run ID: 6b16edf1-9d7b-404e-af16-3aefbaf6771a
📒 Files selected for processing (9)
src/Eccube/Entity/Faq.phpsrc/Eccube/Form/Type/Admin/ProductType.phpsrc/Eccube/Resource/locale/messages.en.yamlsrc/Eccube/Resource/locale/messages.ja.yamlsrc/Eccube/Resource/template/default/Block/faq.twigsrc/Eccube/Resource/template/default/Product/detail.twigsrc/Eccube/Resource/template/default/Product/list.twigtests/Eccube/Tests/Web/Admin/Content/FaqControllerTest.phptests/Eccube/Tests/Web/Admin/Product/ProductControllerTest.php
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Eccube/Resource/template/default/Product/detail.twig
- src/Eccube/Resource/template/default/Product/list.twig
- src/Eccube/Resource/locale/messages.en.yaml
- src/Eccube/Form/Type/Admin/ProductType.php
- src/Eccube/Resource/locale/messages.ja.yaml
- サイト共通FAQブロックの全件取得に上限を追加（AutoNewItem 同様 config 化: eccube_max_number_faq_get）。FaqRepository::getCommonFaq に $limit を追加し Block\FaqController から設定値を渡す - Version20260727000000 の down() に up() と対称なテーブル存在ガードを追加 - ProductFaqDisplayTest のクロージャ引数に Crawler 型を付与 - FaqRepositoryTest に取得件数上限の検証を追加 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CodeRabbit 指摘対応。setProduct/setCategory で一方に非nullを設定したら もう一方の関連を解除し、getFaqType() の導出と FaqRepository の取得結果が 食い違わないようにする。FaqTest で排他動作と区分導出を検証。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CodeRabbit のフォローアップ対応。getProductFaq / getCategoryFaq にも $limit を 追加し、ProductController から config eccube_max_number_faq_get を渡す。 これで3区分すべてのフロント取得が上限付きになり、JSON-LD も同じ上限済み配列から 生成される。FaqRepositoryTest に上限検証を追加。 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- create_date / update_date を `@var` docblock から native 型宣言 (`?\DateTime ... = null`) へ変更。#6569 で全 Entity が統一済みで、 旧スタイルが残るのは Faq のみだった（PHPStan level6 は docblock でも 通るため CI では検出されない） - setProduct / setCategory の「もう一方を黙って null にする」副作用を撤去し、 コアの他 Entity と同じ素の代入に戻す。区分の導出は getFaqType() に集約 されており、管理画面に両方を設定する経路は無い旨を PHPDoc に明記 - FaqTest を排他検証から getFaqType() の3区分導出検証へ置き換え Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
上限10件は管理者に一切見えないまま FAQ を切り捨てていた（表示ONを12件 登録するとフロントは10件・JSON-LD も10問だけになるが、管理画面の一覧は 12件すべてを「表示」として並べる）。新着商品（eccube_max_number_new_items_get） は本質的に「最新N件」ウィジェットだが、FAQ は店舗が編纂した全件を出す前提の コンテンツのため、黙って落ちる状態は事故になりやすい。 - eccube_max_number_faq_get: 10 -> 50 - admin.content.faq.display_limit_notice を ja/en に追加 - サイト共通FAQ一覧と FAQ 入力コレクション（商品FAQ・カテゴリFAQ が共用）に 上限の注記を表示 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Block/faq.twig・Product/detail.twig・Product/list.twig に約16行が
逐語コピーされており、上書きしている店舗の追従も3箇所必要だった。
- default/_faq_list.twig を追加（secHeading 3点セット＋FAQループ、
空判定もパーシャル側に集約）。配置は _front_mode_alert_row.twig に倣う
- 3ファイルから `{% include '_faq_list.twig' with {'Faqs': Faqs|default([])} only %}`
で呼ぶ。default フィルタで Faqs 未定義時（strict_variables 有効な dev/test）
も安全に空扱いにする
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
カテゴリFAQは ?pageno=2 以降にも同じ FAQPage の JSON-LD と同じ可視FAQを 出力しており、構造化データがページ数ぶん重複していた。 - ProductController::index で pagination->getCurrentPageNumber() === 1 の ときだけカテゴリFAQを取得する - ProductFaqDisplayTest に1ページ目/2ページ目の出し分けテストを追加し、 FAQPage 抽出処理を findFaqPageJsonLd() に共通化 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
FAQ 分は手書きでコンパイル済みルールを追記していたため、ビルド出力と
形が一致していなかった（追記は `@media (min-width: 768px) {`、実ビルドは
media_desktop mixin 由来の `@media only screen and (min-width: 768px){` で
さらに postcss-sort-media-queries が1ブロックに統合する）。次に誰かが
npm run build した時点で消える状態だったため、正規のビルド成果物に戻す。
`npm run build`（gulp: scss -> scss-min -> webpack）の出力のうち、FAQ と
無関係な admin CSS・html/bundle は対象外として差し戻し、フロントの
style.css / style.min.css と各 .map のみを取り込む。Bootstrap 5.3.3 -> 5.3.8
の再生成差分を含む。
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nanasess
left a comment
nanasess
left a comment
There was a problem hiding this comment.
FAQ 機能の追加をレビューしました。実装・テスト・ドキュメント（PR 本文の仕様説明）はよく揃っており、CI も全ジョブ pass、CodeRabbit の指摘も概ね解消済みであることを確認しています。
今回は特に以下 2 点を重点的に見ました。
- 表示順の UI をカテゴリ／規格／タグ等と同じ D&D に統一できないか
- 商品 FAQ・カテゴリ FAQ が「どこに登録されているか」一覧から分からない
どちらもコア側に流用できる既存パターンがあり、追加実装は比較的小さく収まりそうです。該当箇所にインラインでコメントしています。
あわせて、ProductType に追加された faqs（mapped な CollectionType）＋ Product::$Faqs の orphanRemoval の組み合わせについて 1 件、対応をご検討いただきたい点があります（テンプレート上書き店舗での沈黙データ消失）。
確認範囲
- 差分 44 ファイル。
style.css/style.min.css/*.mapはビルド成果物のため内容の逐行確認は省略しました（生成手順の妥当性は PR 本文の説明どおりと理解しています）。 - コア側の既存慣例:
admin/Product/tag.twig・category.twig・class_category.twig・Setting/Shop/delivery_edit.twig・Content/news.twigと各moveSortNo()、DeliveryTimeType。 - Doctrine ORM の
orphanRemoval/copyProperties()経路（vendor/doctrine/orm/src/UnitOfWork.php、Persisters/Collection/OneToManyPersister.php）。 - FAQ 関連テスト 7 ファイルのテストメソッド一覧。
補足（指摘ではありません）
商品コピー（admin_product_product_copy）は、copyProperties() が $Faqs の PersistentCollection を共有したまま persist()＋flush() される経路を通るため orphanRemoval との相性が気になりましたが、ORM のコードを追った限りコピー元の FAQ が消えることはありませんでした。UnitOfWork::commit() の collection deletion は executeInserts() より先に実行され（UnitOfWork.php:387-404）、その DELETE は WHERE product_id = <コピー先の ID>（OneToManyPersister.php:159-188）なので 0 件ヒットに終わります。ただし挙動が ORM 実装の細部に依存しているため、回帰検知用のテストがあると安心です（別途コメントしています）。
| // 未描画（空コレクション）だと既存 FAQ が全削除される。商品編集テンプレート | ||
| // app/template/admin/Product/product.twig を上書きしている店舗では、上書き先に | ||
| // faqs の描画（form_widget(form.faqs)）を必ず含めること。 | ||
| ->add('faqs', CollectionType::class, [ |
There was a problem hiding this comment.
mapped な CollectionType ＋ orphanRemoval は、テンプレート上書き店舗で沈黙データ消失を起こします。
コメントで注意書きをいただいているとおりですが、この組み合わせはコア側の既存方針から外れており、注意書きだけで担保するのは厳しいのではないかと考えています。
[VERIFIED] 根拠:
ProductTypeの他のコレクションは すべて'mapped' => falseで、保存はコントローラ側が明示的に行っています（ProductType.php:143-171のtags/images/add_images/delete_images）。faqsは本 FormType で唯一の mapped なコレクションです。orphanRemoval: trueはコア Entity で本 PR が初出です（rg 'orphanRemoval' src/Eccube/Entity/のヒットはProduct.php:516とCategory.php:193の 2 件のみ。既存のProductCategories/ProductClasses/ProductImage/ProductTagはいずれもcascadeのみでorphanRemovalなし）。
AGENTS.md が「テンプレート上書き」を標準のカスタマイズ手段として挙げているとおり、app/template/admin/Product/product.twig を上書きしている店舗は実運用で少なくありません。上書き先に form.faqs が無いまま商品編集を保存すると、allow_delete により送信データが空コレクション扱いになり、orphanRemoval で既存の商品 FAQ がエラーも警告もなく全削除されます。同時に、その店舗では商品 FAQ の登録手段自体が UI から失われます。
ご検討いただきたい選択肢:
- 他コレクションと同じ
'mapped' => falseに寄せる —tags/imagesと同様、コントローラで明示的に追加・更新・削除する。コア慣例と揃い、未描画時は「何も起きない」に倒れます（最も安全）。 PRE_SUBMITでキー欠落をガードする — 送信データにfaqsキーが存在しない場合のみ既存 FAQ の内容を埋め戻し、削除を発生させない。mapped のまま実装量を抑えられます。- 上記が難しい場合は、少なくとも 「
faqsキーが送信データに存在しない POST で既存 FAQ が保持される（または意図どおり削除される）」ことを固定するテストを追加してください。現在のProductControllerTest::testEditWithProductFaqはfaqsを必ず送っており、キー欠落のケースは通っていません。
いかがでしょうか。
There was a problem hiding this comment.
ご指摘ありがとうございます。案2（PRE_SUBMIT でキー欠落をガードする） で対応します。
事実関係は確認しました。ProductType の他コレクションがすべて 'mapped' => false であること、orphanRemoval がコア Entity で本 PR 初出（Product.php:519 / Category.php:193 の 2 件のみ）であること、テンプレート上書き店舗で既存 FAQ が無警告で全削除されうることは、いずれもそのとおりです。
一方で、案1（'mapped' => false に寄せる）ではこの沈黙削除は塞がらないと考えています。根拠は次のとおりです。
- mapped な CollectionType ＋
allow_delete自体は、DeliveryType.php:85のdelivery_timesにコア側の前例があります（mappedは既定 true）。 - そちらは
orphanRemovalを使わず、DeliveryController.php:110-114, 184-187が保存前のスナップショットとの差分で明示的にremove()する形です。 - つまり
delivery_edit.twigをdelivery_times抜きで上書きすれば、orphanRemovalが無くても同じ沈黙削除が起きます。
危険の実体は orphanRemoval そのものではなく、「未描画＝送信キーなし」を空コレクションとして受け取ってしまう点にあります。ResizeFormListener::preSubmit() は !is_array($data) のとき $data = [] に倒し、allow_delete 側で既存の子をすべて remove() します。この経路は mapped/unmapped を問わないため、案1に寄せてコントローラで差分削除する形にしても穴は残ります。
そこで、送信データに faqs キーが存在しない場合は既存 FAQ の削除を発生させない PRE_SUBMIT ガードを入れます（CategoryFaqType も同様に対応します）。あわせて案3 のテスト（faqs キーを含まない POST で既存 FAQ が保持されること）も追加します。
なお orphanRemoval 自体は、コレクションから外れた行の削除をコントローラに書かずに済ませるために残す想定です。上記ガードでキー欠落のケースを潰したうえで、UI 上で削除された行の反映はこれまでどおり orphanRemoval に任せます。この方針で問題ないでしょうか。
There was a problem hiding this comment.
@ttokoro20240902 案2（PRE_SUBMIT ガード）で問題ありません。 反論いただいた3点はいずれも実コードで裏付けを確認しました。
ResizeFormListener.php:127-138…!is_array($data)で$data = []に倒し、allowDelete側で!isset($data[$name])の子を全てremove()するDeliveryType.php:85…delivery_timesは'mapped' => falseを持たない（＝mapped な CollectionType ＋allow_delete）。隣接するdelivery_fees等と違い、コア側の前例として成立しているDeliveryController.php:110-115, 184-187…$originalDeliveryTimesのスナップショットと比較して明示的にremove()
したがって「危険の実体は orphanRemoval ではなく、未描画＝送信キーなしを空コレクションとして受け取る点にある」「案1に寄せても穴は残る」というご指摘は正しく、私の案1は不適切でした。orphanRemoval を残す方針にも異論ありません。
ただし、ガードの発動条件を「faqs キーが無い」だけにすると成立しません。
faq_collection_javascript.twig:58-62 は .faq-collection__item を DOM ごと remove() します。
$collection.on('click', '.faq-collection__remove', function () {
$(this).tooltip('hide');
$(this).closest('.faq-collection__item').remove();
moveSortNo();
});そのため UI で全行を削除して保存した場合も、admin_product[faqs][...] の input は1つも残らず、faqs キーは POST に現れません（data-prototype は属性であって input ではないため送信されません）。つまり
| ケース | 送信データ | 期待する挙動 |
|---|---|---|
| テンプレート上書きで未描画 | faqs キーなし |
既存FAQを保持 |
| UIで全行削除 | faqs キーなし |
既存FAQを全削除 |
の2つがサーバ側で区別できません。キー欠落だけを条件にガードすると、後者が「削除したのに復活する」挙動になります。現状は前者が壊れていますが、ガードを入れると今度は後者が壊れる、という関係です。
そこで「描画された」ことを示すセンチネルを1つ足すのはいかがでしょうか。
ProductType/CategoryFaqTypeに unmapped のHiddenType（例faqs_rendered、値1）を追加faq_collection.twigの中で必ずform_widget(...)する（＝partial を include した上書きテンプレートには自動的に付いてくる。include していない上書きでは付かない）- PRE_SUBMIT は
empty($data['faqs_rendered'])のときだけ発動
これで「未描画」と「全行削除」が確実に分かれます。
ガードの中身は、既存データの埋め戻しではなく $form->remove('faqs') を推奨します。
$builder->addEventListener(FormEvents::PRE_SUBMIT, function (FormEvent $event): void {
$data = $event->getData();
if (empty($data['faqs_rendered'])) {
// FAQ欄が描画されていない（テンプレート上書き等）。faqs には一切触れない
$event->getForm()->remove('faqs');
}
});理由は2点です。
- DataMapper が
Product::$Faqsに触れないためorphanRemovalが発火しません。 埋め戻し方式だと「既存と同じ値で再送信」という経路を通るため、by_reference => falseの再代入やFaqのupdate_date更新など余計な副作用が乗ります。 app/Customizeが FaqType に足した拡張フィールドを取りこぼしません。 埋め戻しは Faq エンティティを配列へ変換する必要があり、拡張フィールド分を書き漏らすと今度はそちらが消えます。
PRE_SUBMIT 中の $form->remove() は ResizeFormListener.php:132-138 自身が使っているイディオムなので、挙動としても安全側です。
テストは2本に分けていただけると、この区別が固定できます。
faqsキーなし ＋faqs_renderedなし → 既存FAQが保持されるfaqsキーなし ＋faqs_renderedあり → 既存FAQが全削除される（UIでの全行削除）
よろしくお願いします。
| <li class="list-group-item" data-id="{{ Faq.id }}"> | ||
| <div class="row justify-content-around"> | ||
| <div class="col-2 d-flex align-items-center"> | ||
| <span>{{ Faq.sortNo }}</span></div> |
There was a problem hiding this comment.
表示順の UI を、カテゴリ／規格分類／タグと同じ D&D に統一できないでしょうか。
現状、FAQ 一覧の表示順は読み取り専用のテキスト（この行）で、並び替えるには編集画面を開いて数値を打ち込む必要があります。表示順を持つ他のマスタ系画面は、いずれも一覧上での D&D ＋ ↑↓ ボタンに統一されています。
[VERIFIED] コア側の既存パターン（そのまま流用できます）:
| 画面 | テンプレート | 更新エンドポイント |
|---|---|---|
| タグ | admin/Product/tag.twig:66-90 |
TagController.php:151 admin_product_tag_sort_no_move |
| カテゴリ | admin/Product/category.twig |
CategoryController.php:258 admin_product_category_sort_no_move |
| 規格分類 | admin/Product/class_category.twig |
ClassCategoryController.php:244 |
| 規格名 | admin/Product/class_name.twig |
ClassNameController.php:171 |
| 支払方法 / 配送業者 | Setting/Shop/payment.twig / delivery.twig |
PaymentController.php:321 / DeliveryController.php:333 |
必要な変更は小さく、次の 3 点で揃うと思われます。
<ul>にsortable-container、各<li>にsortable-itemとdata-sort-no="{{ Faq.sortNo }}"を追加（category.twig:212と同じ形）tag.twig:26-90の{% block javascript %}を移植（jQuery UI sortable ＋ ↑↓ ＋ ajax POST）FaqControllerにmoveSortNo()を追加（TagController.php:151-167/CategoryController.php:258-282と同形。Faqは#[ORM\Cache]付きなのでCategoryController同様clearDoctrineCache()も呼ぶ）
CSRF は admin/default_frame.twig:26-34 の $.ajaxSetup が ECCUBE-CSRF-TOKEN ヘッダを全 ajax に自動付与するため、isTokenValid() だけで通り、追加実装は不要です。
なお、この画面は news.twig からの派生と見えますが（news.twig:19-25 と同一の z-index: inherit の {% block stylesheet %} が残っています）、News は publish_date 順で表示順を持たないため D&D の対象外です。表示順を持つ FAQ は tag.twig 系に倣うのが自然だと思います。
There was a problem hiding this comment.
ご提案どおり、tag.twig 系のパターンに揃えました。
FaqController::moveSortNo()を追加（admin_content_faq_sort_no_move）。TagController.php:151-167と同形で、Faqは#[ORM\Cache]付きのためご指摘どおりclearDoctrineCache()も呼んでいます。<ul>にsortable-container、各<li>にsortable-item＋data-sort-noを付け、tag.twig:26-90の{% block javascript %}を移植しました（jQuery UI sortable ＋ ↑↓ ＋ ajax POST）。- CSRF はご指摘どおり
$.ajaxSetupの自動付与で通るため、isTokenValid()のみで追加実装はありません。
1点、方針をご相談させてください。 別コメントでご提案いただいた「Content > FAQ の一覧を3区分横断にする」と、この D&D は組み合わせると衝突します。横断表示のまま行をドラッグすると、異なる商品・カテゴリの FAQ をまたいで sort_no を書き換えることになり、並び替えとして意味を持ちません。
そこで D&D は「サイト共通」で絞り込んだときのみ有効にし、他区分では表示順を読み取り専用のテキストに戻して「商品ごと・カテゴリごとの並び替えは各編集画面で行う」旨を画面に注記する形にしました。moveSortNo() 側でもサイト共通FAQ以外は更新をスキップし、テスト（FaqControllerTest::testMoveSortNoUpdatesOnlyCommonFaq）で固定しています。
商品ごと・カテゴリごとの並び替えは、埋め込みコレクション側の D&D が担います。実機で両画面とも動作を確認しました。
| 画面 | 2行追加後の sort_no |
↓ボタン後 | ドラッグ後 |
|---|---|---|---|
| 商品編集 | 1行目=1 / 2行目=2 | 入れ替わって 1 / 2 に再採番 | 戻って 1 / 2 |
| カテゴリFAQ | 1行目=1 / 2行目=2 | 入れ替わって 1 / 2 に再採番 | — |
jquery-ui/ui/widgets/sortable は html/template/admin/assets/js/bundle.js:17 で admin バンドルに含まれ、default_frame.twig:23 が全管理画面で読み込むため、追加の読み込みは不要でした。
この切り分けでよいか、あるいは区分ごとのタブに分けて各タブ内で D&D にするなど別の形がよいか、ご意見をいただけますか。
There was a problem hiding this comment.
@ttokoro20240902 対応ありがとうございます。この切り分けで問題ありません。
横断表示のまま行をドラッグすると異なる紐付け先をまたいで sort_no を書き換えることになる、というご指摘はそのとおりで、区分をまたぐ D&D は意味を持ちません。タブに分けるより、既存の区分フィルタを流用してサイト共通のときだけ有効化するほうが画面構造もシンプルで良いと思います。
実装も確認しました。
faq.twig:16{% set sortable = faqType == 'common' %}faq.twig:128<ul class="list-group ...{% if sortable %} sortable-container{% endif %}">faq.twig:153-154sortable-itemとdata-sort-noも同じ条件FaqController::moveSortNo()がサーバ側でもサイト共通以外をcontinueでスキップ（クライアント側の制御だけに依存していない点が良いと思います）clearDoctrineCache()も入っている
1点だけ、発見性についてご検討ください。
一覧の既定表示が「すべて」なので、そのままでは D&D もハンドルも ↑↓ も現れません。並び替えたい管理者が「表示順は編集画面でしか変えられない」と誤解する可能性があります。
faq.twig には既に admin.content.faq.display_limit_notice を出す <p class="text-muted"> があるので、そこに1文足すのが一番安く済むと思います。
表示順の並び替えは、区分を「サイト共通」で絞り込むと行えます。商品ごと・カテゴリごとの並び替えは、各商品の編集画面／カテゴリFAQ画面で行ってください。
（文言はお任せします。区分フィルタの「サイト共通」ボタン自体がリンクなので、注記からそこへ誘導する形でも構いません。）
商品ごと・カテゴリごとの並び替えを埋め込みコレクション側の D&D が担う、という役割分担はそのとおりで問題ありません。実機での確認結果の表もありがとうございます。jquery-ui/ui/widgets/sortable が admin バンドルに同梱済みで追加読み込み不要、という点も確認しました。
# Conflicts: # .claude/skills/eccube-repository/SKILL.md
nanasess のレビュー指摘に対応する。 - 管理画面のFAQ一覧を3区分横断に変更（区分・紐付け先の列と絞り込み、 区分ごとのリンク先振り分け）。Product/Category を fetch join して N+1 を回避 - 表示順の並び替えをドラッグ＆ドロップに統一。一覧は tag.twig 系、 埋め込みコレクションは delivery_edit.twig 系のパターンを踏襲する。 一覧のD&Dは並び替えが同一区分内でしか意味を持たないためサイト共通FAQのみ有効 - FaqType に sortable オプションを追加し、コレクションでは sort_no を hidden 化。 表示順を空欄で登録した場合は最大値 + 1 を採番する（CategoryRepository と同じ1始まり） - FAQ入力コレクションの公開状態を操作行にまとめ、カードの高さを削減 - カテゴリツリーにFAQ件数バッジを追加（countByCategoryIds で1クエリ） - カテゴリごとFAQを祖先カテゴリから継承。自カテゴリ優先・祖先は近い順で、 取得件数の上限は統合後に適用する - dtb_faq の product_id / category_id にインデックスを追加 - en ロケールの FAQ 見出しが「FAQ FAQ」と重複していたのを修正し、 front.block.* のキー位置を揃える - 商品コピー時にFAQが複製され、かつコピー元のFAQが残ることを固定するテストを追加 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
style.min.css は minify 済みで全 CSS が1行のため、base 側の #6259 （お届け先の操作を文字ボタン化しスタイルを SCSS へ移設）と本 PR の フルビルド再生成がぶつかり、行単位マージが成立せず衝突した。 scss ソース（_17.1.address.scss / _23.1.faq.scss）は衝突なくマージ できているため、成果物は手でマージせず次のとおり解決した。 - style.css / *.map: 3-way の自動マージ結果を採用 - style.min.css: nodejs コンテナで再ビルドした出力を採用 - ローカルのフルビルドは整形差（ルール間の空行）で +1157 行の ノイズが乗ったため、style.css には取り込んでいない FAQ（ec-faqRole）と #6259（ec-addressList--selectable ほか）の スタイルが style.css / style.min.css の双方に入っていることを確認済み。
style.css / style.min.css とその source map のコンフリクトは、 4.4 で gulp から esbuild へビルドツールが移行したため発生した。 マージ後の SCSS から npm run build で再生成して解消している。
nanasess
left a comment
nanasess
left a comment
There was a problem hiding this comment.
更新分（8e4d52e678）を確認しました。前回の指摘 10 件のうち 9 件が実装済みで、いずれも実コードで意図どおりであることを確認しています。CI も 130 SUCCESS / 1 SKIPPED / 失敗 0 でした。
特に以下は、こちらの提案そのままではなく、より妥当な形に組み替えていただいた点が良かったです。
getQueryBuilderAll()の命名は「文字どおり全件＋区分絞り込み」を採用（共通専用メソッドは呼び出し元が無くなるため作らない、という判断も妥当だと思います）- NULL の並び順が DB 実装で異なる件を
COALESCE(..., 0) AS HIDDENで吸収。(0,0) → (0, c.id) → (p.id, 0)の順序になるため「サイト共通 → カテゴリごと → 商品ごと」が意図どおり成立することを確認しました - カテゴリ FAQ の祖先継承で、上限の切り捨てが常に遠い祖先側になるよう
ORDER BY c.hierarchy DESCを先頭に置いた設計 - 一覧の D&D をサーバ側（
FaqController::moveSortNo()）でも共通 FAQ 限定にガードしている点
ProductType の PRE_SUBMIT ガード（未実装分）と一覧 D&D の切り分けについては、それぞれのスレッドに返信しました。ガードは方針に賛成ですが、発動条件について 1 点だけ追加の論点があります。
あわせて、今回の更新で新しく入った変更に対して 2 件コメントしています。いずれも CI では検出されない種類のものです。
| * 表示順. 未採番（null）で保存されたときは FaqRepository::save() が最大値 + 1 を割り当てる。 | ||
| */ | ||
| #[ORM\Column(name: 'sort_no', type: Types::INTEGER, options: ['default' => 0])] | ||
| private ?int $sort_no = null; |
There was a problem hiding this comment.
プロパティを ?int = null にした一方で、カラムは NOT NULL のままです。
[VERIFIED] 1 行上の Faq.php:78 は options: ['default' => 0] のみで nullable: true が付いていないため、dtb_faq.sort_no は NOT NULL のままです。今回の変更でプロパティ既定値が 0 から null になったので、採番処理を通らずに Faq を永続化する経路は NOT NULL 制約違反になります。
採番が効くのは次の 2 経路だけです。
FaqRepository::save()（FaqRepository.php:41-43）… サイト共通 FAQ の CRUDFaqTypeの POST_SUBMIT フォールバック（FaqType.php:79-85）…sortableオプションがtrueのときのみ
つまり、プラグインや app/Customize、データフィクスチャが
$Faq = new Faq();
$Faq->setQuestion('...')->setAnswer('...');
$Product->addFaq($Faq);
$entityManager->persist($Faq);
$entityManager->flush();のように書くと、変更前は sort_no = 0 が入って通っていたものが、今後は INSERT 時に落ちます。コア内の経路はすべて上記いずれかを通るため、PHPUnit でも CI でも表面化しません。エンティティ単体の契約としては後退しており、拡張ポイントとしては踏みやすい形だと思います。
最小の修正はプロパティ既定値だけ 0 に戻す形です。
| private ?int $sort_no = null; | |
| private ?int $sort_no = 0; |
これなら「new Faq() は常に永続化可能」という従来の契約を保ったまま、フォーム経由で明示的に null が入ったときだけ FaqRepository::save() が採番する、という今回の意図はそのまま成立します（Assert\Range は null をスキップするため、単独編集画面の未入力も従来どおり通ります）。
もし「未採番」を型で表現することを優先されるのであれば、代わりに #[ORM\PrePersist]（Faq は既に #[ORM\HasLifecycleCallbacks] 付きです）で null を埋める形も考えられますが、コアに前例が無いので上記の 1 行のほうが素直だと思います。いかがでしょうか。
| /** | ||
| * 登録済みFAQの表示順の最大値を返す（未登録なら 0）. | ||
| */ | ||
| private function getMaxSortNo(): int |
There was a problem hiding this comment.
採番の母集団が全区分横断になっています。
sort_no は区分／紐付け先ごとに独立して意味を持つ値です。フロント取得はいずれもスコープを絞ったうえで ORDER BY f.sort_no しています。
getCommonFaq()…Product IS NULL AND Category IS NULLの中で並べるgetProductFaq()… 特定商品の中で並べるgetCategoryFaq()… 祖先を含むカテゴリ集合の中で並べる
一方この getMaxSortNo() は dtb_faq 全体の MAX を返すため、商品 FAQ を 200 件持つ店舗では、最初のサイト共通 FAQ の表示順が 201 になります。動作としては壊れませんが、管理画面の一覧にそのまま出る数値として不自然ですし、今回 Assert\Range(['min' => 1]) を入れて「1 始まりに揃える」とした意図とも噛み合いません。
[VERIFIED] PHPDoc で参照されている CategoryRepository::save()（CategoryRepository.php:111-124）が全件 MAX で正しいのは、カテゴリの sort_no がツリー全体の通し番号（sortNo > :sort_no を一括で +1 する処理があるとおり）だからで、FAQ とは前提が異なります。
save() は現状サイト共通 FAQ の CRUD からしか呼ばれませんが、AbstractRepository::save() のオーバーライドである以上は汎用 API なので、$entity の区分に応じてスコープを合わせるのが正しいと思います。
private function getMaxSortNo(Faq $Faq): int
{
$qb = $this->createQueryBuilder('f')
->select('COALESCE(MAX(f.sort_no), 0)');
match ($Faq->getFaqType()) {
Faq::FAQ_TYPE_PRODUCT => $qb->where('f.Product = :Product')
->setParameter('Product', $Faq->getProduct()),
Faq::FAQ_TYPE_CATEGORY => $qb->where('f.Category = :Category')
->setParameter('Category', $Faq->getCategory()),
default => $qb->where('f.Product IS NULL')->andWhere('f.Category IS NULL'),
};
return (int) $qb->getQuery()->getSingleScalarResult();
}FaqRepositoryTest::testSaveAssignsNextSortNoWhenNotSpecified も、他区分の FAQ が存在する状態で共通 FAQ が 1 から始まることを確認する形にしていただけると、この性質が固定できます。
nanasess
left a comment
nanasess
left a comment
There was a problem hiding this comment.
@ttokoro20240902 追加コメントがありますので確認お願いします🙇♂️
2 participants
概要
FAQ（よくある質問）を管理・表示できる機能を追加します（Issue #6148）。当初の狙いだった Google の FAQ リッチリザルトは廃止済みですが、購入前不安の解消・問い合わせ削減・AI/構造化データ対策を目的に、本体同梱で実装します（方針は #6148 で合意済み）。
区分は3つ:
方針
Faqエンティティ（dtb_faq）を追加。区分はproduct_id/category_id（いずれも nullable）の設定状態で導出（両方 null=サイト共通）。Content > FAQの専用CRUD（FaqController、News を踏襲）CollectionTypeで埋め込みCategoryFaqController、ツリー各行にFAQボタン）Content > FAQの一覧は3区分横断。「区分」「紐付け先」列と区分の絞り込みを持ち、行のリンク先を区分ごとに振り分けます（サイト共通→FAQ編集、商品ごと→商品編集、カテゴリごと→カテゴリFAQ）。Product/Categoryを fetch join して N+1 を避けています。tag.twig系（moveSortNo()＋ ajax）、商品編集・カテゴリFAQの埋め込みコレクションはdelivery_edit.twig系（hidden のsort_noに JS が連番）のパターンを踏襲します。一覧上の並び替えは同一区分内でしか意味を持たないため、「サイト共通」で絞り込んだときのみ有効です（商品ごと・カテゴリごとは各編集画面で並び替え）。FaqRepository::save()が最大値 + 1 を採番します（CategoryRepositoryと同じ 1 始まり。0 は弾きます）。FaqRepository::countByCategoryIds()で1クエリ）。dtb_blockのfaq）を追加し、use_controllerで描画。FaqStructuredDataServiceがFAQPageの連想配列を生成し、既存の|json_ldフィルタ（EccubeExtension::encodeJsonLd）経由で XSS 安全に出力。商品詳細 / 商品一覧(カテゴリ) / FAQブロックに、可視コンテンツと同居して出力。ec-faqRole）を追加（Q./A. マーカー・字下げ・区切り線）。補足
dtb_faqテーブルはエンティティ属性が源泉のためschema:updateで作成されます。マイグレーション（Version20260727000000）は FAQブロックのdtb_blockINSERT のみ（CSV も同時追記）。style.css/style.min.cssはnpm run build（gulp）による正規のビルド成果物です。Bootstrap 5.3.3 → 5.3.8 の再生成差分を含みます（style.cssで約 500 行）。ビルド出力のうち FAQ と無関係な admin CSS・html/bundleは対象外として差し戻しています。purify_html）に加えて表示時も|purify|nl2br（商品説明・Newsと同じ多層防御）。eccube_max_number_faq_get（既定 50）件までです。上限は管理画面のFAQ一覧・商品FAQ欄・カテゴリFAQ欄に明記しています。default/_faq_list.twigに集約し、FAQブロック・商品詳細・商品一覧の3箇所から include しています。FAQPageの構造化データが重複するため）。Category::getPath()（ルート〜自身）に登録されたFAQをまとめて表示するため、上位カテゴリに共通のFAQを置く運用ができます。並びは自カテゴリが先頭・祖先は近い順に後ろ（カテゴリ階層の降順）で、同一カテゴリ内は表示順・IDの昇順。eccube_max_number_faq_getはこの統合後の並びに適用するため、件数超過で切り捨てられるのは常に遠い祖先側です。FAQPageは統合して1本のまま出力します。FAQPageが2本出力されます。既定のレイアウトでは発生せず、ProductFaqDisplayTestが1本であることを検証しています。テスト
PHPUnit を追加（すべてグリーン）:
FaqStructuredDataServiceTest… FAQPage 構造・空時の抑止FaqRepositoryTest… 3区分の取得・並び順・表示フラグ／祖先カテゴリからの継承（自カテゴリ優先・上限で切れるのは祖先側・継承は祖先方向のみ）／表示順未指定時の採番FaqControllerTest… サイト共通FAQの一覧/新規/編集/削除（登録・削除は永続結果を検証。商品FAQ id での編集/削除が404になるガードも検証）FaqControllerTest（一覧・並び替え） … 3区分横断一覧・区分での絞り込み・表示順の並び替え（サイト共通以外は更新しない）・表示順未入力/0 の扱いCategoryFaqControllerTest… カテゴリFAQページの表示・保存ProductControllerTest（FAQ表示） … 商品詳細で表示FAQのみ描画・非表示は出ない・FAQPageJSON-LD 出力／FAQ無し時は非出力ProductControllerTest::testEditWithProductFaq（FAQ保存経路） … 商品編集経由で商品FAQを追加→更新→（送信から漏らして）orphanRemovalで削除、の混在保存を検証ProductControllerTest::testCopyWithProductFaq（商品コピー） … コピー先にFAQが複製され、かつコピー元のFAQが残ることを検証ローカルで php-cs-fixer / PHPStan(level6) / Rector / PHPUnit を通過。管理画面・フロントでの CRUD（複数行での 削除+追加+更新 の混在保存を含む）と JSON-LD 出力を実機確認済み。
商品/カテゴリ編集の埋め込みCollection保存は、それぞれ
ProductControllerTest::testEditWithProductFaq/CategoryFaqControllerTest::testAddFaqでWebテスト済み。互換性
Product/CategoryエンティティにFaqsの OneToMany を追加、ProductTypeにfaqsフィールドを追加（いずれも追加のみ）。Fixes #6148
🤖 Generated with Claude Code
Summary by CodeRabbit