feat: 店舗設定に構造化データ拡張項目を追加 (sameAs/foundingDate 等) (#6147) - #6957
Draft
ttokoro20240902 wants to merge 8 commits into
Draft
feat: 店舗設定に構造化データ拡張項目を追加 (sameAs/foundingDate 等) (#6147)#6957ttokoro20240902 wants to merge 8 commits into
ttokoro20240902 wants to merge 8 commits into
Conversation
Organization / WebSite の JSON-LD をテンプレートの文字列補間で組み立てていたため、 店名等に " や </script> が含まれると構造化データが壊れる／XSS 経路になりうる問題があった （CodeRabbit 指摘）。#6883 で導入済みの json_ld フィルタ（JSON_HEX_* でエスケープ）に揃える。 - SiteStructuredDataService を新設し WebSite / Organization の連想配列を組み立て - TwigInitializeListener で site_json_ld を front グローバルに注入 - default_frame.twig は {{ site_json_ld|json_ld }} で出力 - 旧 Schema/organization.twig・website.twig を削除 - 値が空の任意プロパティは出力しない（ProductStructuredDataService と同方針） - 都道府県未設定（Pref=null）でもフロントが 500 にならない（元 PR の不具合も解消） - SiteStructuredDataServiceTest を追加 出力される JSON-LD は、値のある項目については従来と同一 （空の任意プロパティが省略される点のみ差分）。 Refs #6136 #6147 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Issue #6147。#6139 で新設した SiteStructuredDataService に、店舗設定 (BaseInfo) から出力する構造化データ項目を拡充する。 - BaseInfo に same_as / founding_date / number_of_employees / copyright_year / site_image を追加 - 店舗設定「基本設定」に入力欄（ShopMasterType / shop_master.twig / 翻訳 ja,en）を追加 - SiteStructuredDataService で Organization の sameAs（配列）/ foundingDate / numberOfEmployees（QuantitativeValue）/ image、WebSite の copyrightYear を出力 - マイグレーション追加（Version20260722000000, TEXT型はMySQL/PostgreSQLで分岐） - SiteStructuredDataServiceTest に各項目のテストを追加 値が空の任意プロパティは出力しない（#6139 の方針を踏襲）。 Refs #6136 #6147 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough店舗基本情報に構造化データ項目と営業時間を追加しました。管理画面で保存した情報からJSON-LDを生成し、フロント画面へ出力します。入力検証、データベース保存、単体テスト、E2Eテストも追加しました。 Changesサイト構造化データと営業時間
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant ShopMasterType
participant SiteStructuredDataService
participant Twig
Admin->>ShopMasterType: 店舗情報と営業時間を保存
ShopMasterType->>ShopMasterType: 入力値と営業時間の重複を検証
Twig->>SiteStructuredDataService: BaseInfoからJSON-LDを生成
SiteStructuredDataService-->>Twig: WebSite・Organization JSON-LDを返却
Twig-->>Admin: JSON-LD scriptをフロント画面へ出力
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 |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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 (2)
src/Eccube/Form/Type/Admin/ShopMasterType.php (1)
160-168: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
number_of_employeesに上限を設ける値域チェックが必要
number_of_employeesは数値のみを許容するAssert\Regexのみで、上限がありません。DBカラムはINT（migration参照）のため、桁数の大きい値を入力すると保存時にDBエラーとなる可能性があります。同ファイル内のcopyright_year／basic_point_rate／point_conversion_rateは Range 制約で値域を制限しているため、同様の対応を推奨します。♻️ 提案する修正
->add('number_of_employees', IntegerType::class, [ 'required' => false, 'constraints' => [ new Assert\Regex([ 'pattern' => "/^\d+$/u", 'message' => 'form_error.numeric_only', ]), + new Assert\Range([ + 'min' => 0, + 'max' => 2147483647, + ]), ], ])🤖 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/Form/Type/Admin/ShopMasterType.php` around lines 160 - 168, Update the number_of_employees constraints in ShopMasterType to include an Assert\Range upper bound matching the database INT column, while preserving the existing numeric-only validation. Follow the established Range constraint pattern used by copyright_year, basic_point_rate, and point_conversion_rate, including an appropriate validation message.src/Eccube/EventListener/TwigInitializeListener.php (1)
181-181: 🚀 Performance & Scalability | 🔵 Trivial構造化データの生成はリクエスト毎に行われる点に留意
createWebSiteJsonLdはフロント全ページのリクエストごとに呼び出され、キャッシュされません。現状は軽量な処理（配列構築とルート生成数回）のため大きな懸念はありませんが、将来的に負荷が問題になる場合は、BaseInfoの更新時のみ再計算するキャッシュ戦略（例: タグ付きキャッシュやBaseInfo更新イベントでの無効化）を検討してください。🤖 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/EventListener/TwigInitializeListener.php` at line 181, Review the site_json_ld initialization in TwigInitializeListener and add a cache for createWebSiteJsonLd keyed to the current BaseInfo state, with invalidation when BaseInfo is updated; ensure subsequent requests reuse the cached structured data until that update occurs.
🤖 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/Version20260722000000.php`:
- Around line 35-73: Remove the Version20260722000000 migration because it only
adds nullable columns already represented by BaseInfo entity attributes and
should be applied through schema:update. Delete both up() and down() handling
for same_as, founding_date, number_of_employees, copyright_year, and site_image;
retain the migration only if a documented production-upgrade requirement makes
it necessary, with that exception explicitly recorded.
---
Nitpick comments:
In `@src/Eccube/EventListener/TwigInitializeListener.php`:
- Line 181: Review the site_json_ld initialization in TwigInitializeListener and
add a cache for createWebSiteJsonLd keyed to the current BaseInfo state, with
invalidation when BaseInfo is updated; ensure subsequent requests reuse the
cached structured data until that update occurs.
In `@src/Eccube/Form/Type/Admin/ShopMasterType.php`:
- Around line 160-168: Update the number_of_employees constraints in
ShopMasterType to include an Assert\Range upper bound matching the database INT
column, while preserving the existing numeric-only validation. Follow the
established Range constraint pattern used by copyright_year, basic_point_rate,
and point_conversion_rate, including an appropriate validation message.
🪄 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: 6a3143e4-2c95-413a-bc02-0809e40c0501
📒 Files selected for processing (10)
app/DoctrineMigrations/Version20260722000000.phpsrc/Eccube/Entity/BaseInfo.phpsrc/Eccube/EventListener/TwigInitializeListener.phpsrc/Eccube/Form/Type/Admin/ShopMasterType.phpsrc/Eccube/Resource/locale/messages.en.yamlsrc/Eccube/Resource/locale/messages.ja.yamlsrc/Eccube/Resource/template/admin/Setting/Shop/shop_master.twigsrc/Eccube/Resource/template/default/default_frame.twigsrc/Eccube/Service/SiteStructuredDataService.phptests/Eccube/Tests/Service/SiteStructuredDataServiceTest.php
ttokoro20240902
force-pushed
the
feature/issue-6147-baseinfo-schema
branch
from
July 22, 2026 05:53
247de17 to
b031ca9
Compare
Issue #6147。営業時間を構造化データ(Organization.openingHoursSpecification) として出力できるようにする。既存 business_hour(自由テキスト・Help表示用)とは 別に、曜日×開店/閉店時刻を構造化して保持する。 - OpeningHours エンティティ(dtb_opening_hours)を新設し BaseInfo に OneToMany を追加 - 店舗設定「基本設定」に CollectionType の営業時間入力欄(動的な行追加/削除UI)を追加 - OpeningHoursType(曜日=複数選択, opens/closes=TimeType)・prototype テンプレート - SiteStructuredDataService で openingHoursSpecification(配列)を出力 (曜日/時刻がいずれも無いエントリは出力しない) - バリデーション: 開店<閉店・曜日必須・開店/閉店ペア必須(OpeningHours)、 同一曜日の時間帯重複禁止(ShopMasterType)。BaseInfo の OneToMany に Assert\Valid でカスケード - 削除ボタンは既存の collection UI に合わせ fa-close アイコン化、時刻欄クリックで showPicker - マイグレーション Version20260722010000(Schema APIで両DB対応) - 翻訳(ja/en, ラベルは messages・エラーは validators)・テスト追加 Refs #6136 #6147 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ttokoro20240902
force-pushed
the
feature/issue-6147-baseinfo-schema
branch
from
July 22, 2026 06:22
b031ca9 to
4e3a5c0
Compare
- same_as: 改行区切り各行を Assert\Url で検証（site_image と対称化）
- founding_date: LessThanOrEqual('today') で未来日を禁止
- number_of_employees: Regex を Assert\PositiveOrZero に置換（非負整数）
- 営業時間の重複エラーを該当行に表示（overlap の atPath を closes に変更）
- ShopMasterTypeTest にスカラー5項目のUT 14件を追加（計32件）
- admin-basicinfo.spec.ts に営業時間の動的行UI・保存往復・JSON-LD反映・重複エラーのE2Eを追加（冪等）
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- number_of_employees に Assert\LessThanOrEqual(2147483647) を追加（INT桁あふれ防止） 併せて上限のUT 2件を追加（境界2147483647は有効・2147483648は無効） - Version20260722000000（BaseInfo単純nullableカラム追加のみ）を削除 Entity属性＋schema:update で反映されるためガイドライン準拠（dtb_opening_hours 作成の Version20260722010000 はテーブル新設のため維持） Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
CodeRabbit のレビュー指摘に対応しました（commit 4e97130 / c4e64cb）。
なお、本 PR には別途、追加項目のバリデーション強化（ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.4 #6957 +/- ##
==========================================
+ Coverage 77.21% 77.34% +0.12%
==========================================
Files 563 567 +4
Lines 28111 28391 +280
==========================================
+ Hits 21705 21958 +253
- Misses 6406 6433 +27
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:
|
validators.{ja,en}.yaml のコンフリクトを解消。
双方とも末尾へのキー追記のみだったため、4.4 側の
form_error.refund_request.* と本ブランチの
admin.setting.shop.* を両方残した。
ttokoro20240902
marked this pull request as ready for review
August 5, 2026 02:14
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/Eccube/Tests/Service/SiteStructuredDataServiceTest.php (1)
202-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win空の営業時間エンティティも検証してください。
このテストはコレクションを空にします。
buildOpeningHours()の「空のOpeningHoursエンティティを除外する」分岐は実行しません。値を設定しないOpeningHoursを追加して、openingHoursSpecificationが出力されないことを検証してください。🤖 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/Service/SiteStructuredDataServiceTest.php` around lines 202 - 208, Update testEmptyOpeningHoursIsOmitted to add an OpeningHours entity with no values after clearing the collection, so buildOpeningHours() exercises its empty-entity exclusion branch. Keep the assertion that openingHoursSpecification is absent from the generated organization JSON-LD.
🤖 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 `@e2e/tests/admin-basicinfo.spec.ts`:
- Around line 1329-1424: Wrap the test body after the initial reset save in a
try/finally so cleanup always runs when persistence or duplicate-validation
assertions fail. Move the existing cleanup logic beginning with the
deleteButtons loop into the finally block, preserving its removal of
opening-hour rows and scalar fields before saving.
In `@src/Eccube/Form/Type/Admin/ShopMasterType.php`:
- Around line 372-399: OpeningHours の元のコレクションキーが失われ、$j
がフォーム上の実際の行を指さなくなっています。array_values() を使わず元キーを保持したリストで比較し、atPath() の
OpeningHours[$j].closes には該当する元キーを使用してください。削除後にキーが飛んだ状態で重複行を検出した場合も、正しい closes
フィールドへ違反が表示されるテストを追加してください。
In `@tests/Eccube/Tests/Form/Type/Admin/ShopMasterTypeTest.php`:
- Around line 173-332: Update each newly added test method in
ShopMasterTypeTest, including testValidOpeningHours and the related
opening-hours, same_as, employee, copyright, founding-date, and site-image
tests, to declare a void return type. Keep their existing test logic unchanged.
---
Nitpick comments:
In `@tests/Eccube/Tests/Service/SiteStructuredDataServiceTest.php`:
- Around line 202-208: Update testEmptyOpeningHoursIsOmitted to add an
OpeningHours entity with no values after clearing the collection, so
buildOpeningHours() exercises its empty-entity exclusion branch. Keep the
assertion that openingHoursSpecification is absent from the generated
organization JSON-LD.
🪄 Autofix
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: e6e2fd26-5e0e-4410-b45e-29fffbc286e5
📒 Files selected for processing (16)
app/DoctrineMigrations/Version20260722010000.phpe2e/tests/admin-basicinfo.spec.tssrc/Eccube/Entity/BaseInfo.phpsrc/Eccube/Entity/OpeningHours.phpsrc/Eccube/Form/Type/Admin/OpeningHoursType.phpsrc/Eccube/Form/Type/Admin/ShopMasterType.phpsrc/Eccube/Repository/OpeningHoursRepository.phpsrc/Eccube/Resource/locale/messages.en.yamlsrc/Eccube/Resource/locale/messages.ja.yamlsrc/Eccube/Resource/locale/validators.en.yamlsrc/Eccube/Resource/locale/validators.ja.yamlsrc/Eccube/Resource/template/admin/Setting/Shop/opening_hours_prototype.twigsrc/Eccube/Resource/template/admin/Setting/Shop/shop_master.twigsrc/Eccube/Service/SiteStructuredDataService.phptests/Eccube/Tests/Form/Type/Admin/ShopMasterTypeTest.phptests/Eccube/Tests/Service/SiteStructuredDataServiceTest.php
Comment on lines
+1329
to
+1424
| test('basicinfo_構造化データ_営業時間とSNS等URL - EA0701-UC01-T18', async ({ page }) => { | ||
| test.setTimeout(120_000); | ||
|
|
||
| const sameAsUrl = 'https://example.com/official-sns'; | ||
|
|
||
| // --- 入力（構造化データのスカラー項目 + 営業時間1行） --- | ||
| await page.goto(`/${adminRoute}/setting/shop`); | ||
| await page.waitForLoadState('load'); | ||
| await ensureAdminLoggedIn(page); | ||
| if (!page.url().includes('/setting/shop')) { | ||
| await page.goto(`/${adminRoute}/setting/shop`); | ||
| await page.waitForLoadState('load'); | ||
| } | ||
| await expect(page.locator('.c-pageTitle')).toContainText('基本設定'); | ||
|
|
||
| // --- クリーンな基準状態にする（過去実行の残存行・値を消して保存） --- | ||
| const resetDeletes = page.locator('#opening-hours-group .delete-opening-hour'); | ||
| while (await resetDeletes.count() > 0) { | ||
| await resetDeletes.first().click(); | ||
| } | ||
| await page.locator('#shop_master_same_as').fill(''); | ||
| await page.locator('#shop_master_founding_date').fill(''); | ||
| await page.locator('#shop_master_number_of_employees').fill(''); | ||
| await page.locator('#shop_master_copyright_year').fill(''); | ||
| await page.locator('#shop_master_site_image').fill(''); | ||
| await page.locator('button.ladda-button[type="submit"]').click(); | ||
| await page.waitForLoadState('load'); | ||
| await expect(page.locator('.alert-success')).toContainText('保存しました', { timeout: 30_000 }); | ||
|
|
||
| // --- 入力（構造化データのスカラー項目 + 営業時間1行） --- | ||
| await page.goto(`/${adminRoute}/setting/shop`); | ||
| await page.waitForLoadState('load'); | ||
| await expect(page.locator('#opening-hours-group .opening-hours-item')).toHaveCount(0); | ||
|
|
||
| await page.locator('#shop_master_same_as').fill(sameAsUrl); | ||
| await page.locator('#shop_master_founding_date').fill('2000-04-01'); | ||
| await page.locator('#shop_master_number_of_employees').fill('42'); | ||
| await page.locator('#shop_master_copyright_year').fill('2020'); | ||
| await page.locator('#shop_master_site_image').fill('https://example.com/site.png'); | ||
|
|
||
| // 営業時間の行を追加（月曜 09:00-18:00） | ||
| await page.locator('#add-opening-hour-button').click(); | ||
| await expect(page.locator('#opening-hours-group .opening-hours-item')).toHaveCount(1); | ||
| await page.locator('#shop_master_OpeningHours_0_day_of_week_0').check(); // Monday | ||
| await page.locator('#shop_master_OpeningHours_0_opens').fill('09:00'); | ||
| await page.locator('#shop_master_OpeningHours_0_closes').fill('18:00'); | ||
|
|
||
| await page.locator('button.ladda-button[type="submit"]').click(); | ||
| await page.waitForLoadState('load'); | ||
| await expect(page.locator('.alert-success')).toContainText('保存しました', { timeout: 30_000 }); | ||
|
|
||
| // --- 保存往復の検証（DB反映→再表示） --- | ||
| await page.goto(`/${adminRoute}/setting/shop`); | ||
| await page.waitForLoadState('load'); | ||
| await expect(page.locator('#shop_master_same_as')).toHaveValue(sameAsUrl); | ||
| await expect(page.locator('#shop_master_number_of_employees')).toHaveValue('42'); | ||
| await expect(page.locator('#shop_master_copyright_year')).toHaveValue('2020'); | ||
| await expect(page.locator('#opening-hours-group .opening-hours-item')).toHaveCount(1); | ||
| await expect(page.locator('#shop_master_OpeningHours_0_day_of_week_0')).toBeChecked(); | ||
|
|
||
| // --- フロントの JSON-LD 反映を検証 --- | ||
| await page.goto('/'); | ||
| await page.waitForLoadState('load'); | ||
| const content = await page.content(); | ||
| expect(content).toContain('"openingHoursSpecification"'); | ||
| expect(content).toContain('"sameAs"'); | ||
| expect(content).toContain(sameAsUrl); | ||
|
|
||
| // --- 重複バリデーション：同一曜日で重なる行を追加すると保存が弾かれ、行にエラーが出る --- | ||
| await page.goto(`/${adminRoute}/setting/shop`); | ||
| await page.waitForLoadState('load'); | ||
| await page.locator('#add-opening-hour-button').click(); | ||
| await expect(page.locator('#opening-hours-group .opening-hours-item')).toHaveCount(2); | ||
| await page.locator('#shop_master_OpeningHours_1_day_of_week_0').check(); // Monday（既存行と重複） | ||
| await page.locator('#shop_master_OpeningHours_1_opens').fill('12:00'); | ||
| await page.locator('#shop_master_OpeningHours_1_closes').fill('20:00'); | ||
| await page.locator('button.ladda-button[type="submit"]').click(); | ||
| await page.waitForLoadState('load'); | ||
| await expect(page.locator('.alert-success')).toHaveCount(0); | ||
| await expect(page.locator('#opening-hours-group')).toContainText('同じ曜日で営業時間が重複しています'); | ||
|
|
||
| // --- クリーンアップ：営業時間の行とスカラー項目を消して保存 --- | ||
| await page.goto(`/${adminRoute}/setting/shop`); | ||
| await page.waitForLoadState('load'); | ||
| const deleteButtons = page.locator('#opening-hours-group .delete-opening-hour'); | ||
| while (await deleteButtons.count() > 0) { | ||
| await deleteButtons.first().click(); | ||
| } | ||
| await page.locator('#shop_master_same_as').fill(''); | ||
| await page.locator('#shop_master_founding_date').fill(''); | ||
| await page.locator('#shop_master_number_of_employees').fill(''); | ||
| await page.locator('#shop_master_copyright_year').fill(''); | ||
| await page.locator('#shop_master_site_image').fill(''); | ||
| await page.locator('button.ladda-button[type="submit"]').click(); | ||
| await page.waitForLoadState('load'); | ||
| await expect(page.locator('.alert-success')).toContainText('保存しました', { timeout: 30_000 }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
失敗時にもテストデータを削除してください。
Line 1329 から Line 1424 では、保存後の検証または重複検証が失敗すると、Line 1410 以降のクリーンアップを実行しません。後続の E2E テストが BaseInfo の値と営業時間を引き継ぐため、結果が実行順序に依存します。初期化保存の直後から try/finally を使用して、失敗時にもクリーンアップを実行してください。
🤖 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/tests/admin-basicinfo.spec.ts` around lines 1329 - 1424, Wrap the test
body after the initial reset save in a try/finally so cleanup always runs when
persistence or duplicate-validation assertions fail. Move the existing cleanup
logic beginning with the deleteButtons loop into the finally block, preserving
its removal of opening-hour rows and scalar fields before saving.
Comment on lines
+372
to
+399
| $list = array_values($BaseInfo->getOpeningHours()->toArray()); | ||
| $count = count($list); | ||
| for ($i = 0; $i < $count; ++$i) { | ||
| for ($j = $i + 1; $j < $count; ++$j) { | ||
| $a = $list[$i]; | ||
| $b = $list[$j]; | ||
|
|
||
| $daysA = $a->getDayOfWeek() ?? []; | ||
| $daysB = $b->getDayOfWeek() ?? []; | ||
| if (array_intersect($daysA, $daysB) === []) { | ||
| continue; | ||
| } | ||
|
|
||
| $opensA = $a->getOpens(); | ||
| $closesA = $a->getCloses(); | ||
| $opensB = $b->getOpens(); | ||
| $closesB = $b->getCloses(); | ||
| // 時刻が欠けている行は単体バリデーションに委ねる | ||
| if ($opensA === null || $closesA === null || $opensB === null || $closesB === null) { | ||
| continue; | ||
| } | ||
|
|
||
| // 時間帯が交差する場合はエラー（max(開店) < min(閉店)） | ||
| // 描画済みのリーフ（closes）にエラーを付け、該当行に表示されるようにする | ||
| if (max($opensA, $opensB) < min($closesA, $closesB)) { | ||
| $context->buildViolation('admin.setting.shop.opening_hours.error.overlap') | ||
| ->atPath('OpeningHours['.$j.'].closes') | ||
| ->addViolation(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
営業時間の元のコレクションキーを保持してください。
array_values() は削除後のフォームキーを詰めます。例えば OpeningHours[1] を削除した場合、実際の子フォームは OpeningHours[0] と OpeningHours[2] です。現在の $j は詰めた配列の位置なので、違反が別の行または存在しない closes フィールドに付きます。
元のキーを保持し、そのキーを atPath() に使用してください。削除後に重複した行で、エラーが実際の closes フィールドへ表示されるテストも追加してください。
修正例
- $list = array_values($BaseInfo->getOpeningHours()->toArray());
- $count = count($list);
+ $list = $BaseInfo->getOpeningHours()->toArray();
+ $keys = array_keys($list);
+ $count = count($keys);
for ($i = 0; $i < $count; ++$i) {
for ($j = $i + 1; $j < $count; ++$j) {
- $a = $list[$i];
- $b = $list[$j];
+ $a = $list[$keys[$i]];
+ $b = $list[$keys[$j]];
...
if (max($opensA, $opensB) < min($closesA, $closesB)) {
$context->buildViolation('admin.setting.shop.opening_hours.error.overlap')
- ->atPath('OpeningHours['.$j.'].closes')
+ ->atPath('OpeningHours['.$keys[$j].'].closes')
->addViolation();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| $list = array_values($BaseInfo->getOpeningHours()->toArray()); | |
| $count = count($list); | |
| for ($i = 0; $i < $count; ++$i) { | |
| for ($j = $i + 1; $j < $count; ++$j) { | |
| $a = $list[$i]; | |
| $b = $list[$j]; | |
| $daysA = $a->getDayOfWeek() ?? []; | |
| $daysB = $b->getDayOfWeek() ?? []; | |
| if (array_intersect($daysA, $daysB) === []) { | |
| continue; | |
| } | |
| $opensA = $a->getOpens(); | |
| $closesA = $a->getCloses(); | |
| $opensB = $b->getOpens(); | |
| $closesB = $b->getCloses(); | |
| // 時刻が欠けている行は単体バリデーションに委ねる | |
| if ($opensA === null || $closesA === null || $opensB === null || $closesB === null) { | |
| continue; | |
| } | |
| // 時間帯が交差する場合はエラー（max(開店) < min(閉店)） | |
| // 描画済みのリーフ（closes）にエラーを付け、該当行に表示されるようにする | |
| if (max($opensA, $opensB) < min($closesA, $closesB)) { | |
| $context->buildViolation('admin.setting.shop.opening_hours.error.overlap') | |
| ->atPath('OpeningHours['.$j.'].closes') | |
| ->addViolation(); | |
| $list = $BaseInfo->getOpeningHours()->toArray(); | |
| $keys = array_keys($list); | |
| $count = count($keys); | |
| for ($i = 0; $i < $count; ++$i) { | |
| for ($j = $i + 1; $j < $count; ++$j) { | |
| $a = $list[$keys[$i]]; | |
| $b = $list[$keys[$j]]; | |
| $daysA = $a->getDayOfWeek() ?? []; | |
| $daysB = $b->getDayOfWeek() ?? []; | |
| if (array_intersect($daysA, $daysB) === []) { | |
| continue; | |
| } | |
| $opensA = $a->getOpens(); | |
| $closesA = $a->getCloses(); | |
| $opensB = $b->getOpens(); | |
| $closesB = $b->getCloses(); | |
| // 時刻が欠けている行は単体バリデーションに委ねる | |
| if ($opensA === null || $closesA === null || $opensB === null || $closesB === null) { | |
| continue; | |
| } | |
| // 時間帯が交差する場合はエラー（max(開店) < min(閉店)） | |
| // 描画済みのリーフ（closes）にエラーを付け、該当行に表示されるようにする | |
| if (max($opensA, $opensB) < min($closesA, $closesB)) { | |
| $context->buildViolation('admin.setting.shop.opening_hours.error.overlap') | |
| ->atPath('OpeningHours['.$keys[$j].'].closes') | |
| ->addViolation(); |
🤖 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/Form/Type/Admin/ShopMasterType.php` around lines 372 - 399,
OpeningHours の元のコレクションキーが失われ、$j がフォーム上の実際の行を指さなくなっています。array_values()
を使わず元キーを保持したリストで比較し、atPath() の OpeningHours[$j].closes
には該当する元キーを使用してください。削除後にキーが飛んだ状態で重複行を検出した場合も、正しい closes
フィールドへ違反が表示されるテストを追加してください。
Comment on lines
+173
to
+332
| public function testValidOpeningHours() | ||
| { | ||
| $this->formData['OpeningHours'] = [ | ||
| ['day_of_week' => ['Monday', 'Tuesday'], 'opens' => '09:00', 'closes' => '18:00'], | ||
| ]; | ||
| $this->form->submit($this->formData); | ||
| $this->assertTrue($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testInValidOpeningHoursOpensAfterCloses() | ||
| { | ||
| $this->formData['OpeningHours'] = [ | ||
| ['day_of_week' => ['PublicHolidays'], 'opens' => '20:00', 'closes' => '15:00'], | ||
| ]; | ||
| $this->form->submit($this->formData); | ||
| $this->assertFalse($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testInValidOpeningHoursMissingDay() | ||
| { | ||
| $this->formData['OpeningHours'] = [ | ||
| ['day_of_week' => [], 'opens' => '09:00', 'closes' => '18:00'], | ||
| ]; | ||
| $this->form->submit($this->formData); | ||
| $this->assertFalse($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testInValidOpeningHoursOverlapSameDay() | ||
| { | ||
| $this->formData['OpeningHours'] = [ | ||
| ['day_of_week' => ['Saturday'], 'opens' => '10:00', 'closes' => '15:00'], | ||
| ['day_of_week' => ['Saturday'], 'opens' => '14:00', 'closes' => '18:00'], | ||
| ]; | ||
| $this->form->submit($this->formData); | ||
| $this->assertFalse($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testValidOpeningHoursDifferentDayNoOverlap() | ||
| { | ||
| $this->formData['OpeningHours'] = [ | ||
| ['day_of_week' => ['Saturday'], 'opens' => '10:00', 'closes' => '15:00'], | ||
| ['day_of_week' => ['Sunday'], 'opens' => '10:00', 'closes' => '15:00'], | ||
| ]; | ||
| $this->form->submit($this->formData); | ||
| $this->assertTrue($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testValidSameAsMultipleUrls() | ||
| { | ||
| $this->formData['same_as'] = "https://example.com/a\nhttps://example.com/b"; | ||
| $this->form->submit($this->formData); | ||
| $this->assertTrue($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testInValidSameAsContainsNonUrl() | ||
| { | ||
| $this->formData['same_as'] = "https://example.com/a\nnot-a-url"; | ||
| $this->form->submit($this->formData); | ||
| $this->assertFalse($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testInValidSameAsMaxLength() | ||
| { | ||
| // 形式は有効なURLだが最大長を超えるケース（長さ制約のみを検証） | ||
| $this->formData['same_as'] = 'https://example.com/'.str_repeat('a', $this->eccubeConfig['eccube_ltext_len']); | ||
| $this->form->submit($this->formData); | ||
| $this->assertFalse($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testValidNumberOfEmployeesZero() | ||
| { | ||
| $this->formData['number_of_employees'] = '0'; | ||
| $this->form->submit($this->formData); | ||
| $this->assertTrue($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testInValidNumberOfEmployeesNegative() | ||
| { | ||
| $this->formData['number_of_employees'] = '-1'; | ||
| $this->form->submit($this->formData); | ||
| $this->assertFalse($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testValidNumberOfEmployeesIntMax() | ||
| { | ||
| $this->formData['number_of_employees'] = '2147483647'; | ||
| $this->form->submit($this->formData); | ||
| $this->assertTrue($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testInValidNumberOfEmployeesOverIntMax() | ||
| { | ||
| $this->formData['number_of_employees'] = '2147483648'; | ||
| $this->form->submit($this->formData); | ||
| $this->assertFalse($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testValidCopyrightYearRangeMin() | ||
| { | ||
| $this->formData['copyright_year'] = '1900'; | ||
| $this->form->submit($this->formData); | ||
| $this->assertTrue($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testValidCopyrightYearRangeMax() | ||
| { | ||
| $this->formData['copyright_year'] = '9999'; | ||
| $this->form->submit($this->formData); | ||
| $this->assertTrue($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testInValidCopyrightYearBelowMin() | ||
| { | ||
| $this->formData['copyright_year'] = '1899'; | ||
| $this->form->submit($this->formData); | ||
| $this->assertFalse($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testInValidCopyrightYearAboveMax() | ||
| { | ||
| $this->formData['copyright_year'] = '10000'; | ||
| $this->form->submit($this->formData); | ||
| $this->assertFalse($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testValidFoundingDatePast() | ||
| { | ||
| $this->formData['founding_date'] = '2000-04-01'; | ||
| $this->form->submit($this->formData); | ||
| $this->assertTrue($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testInValidFoundingDateFuture() | ||
| { | ||
| $this->formData['founding_date'] = (new \DateTime('+1 year'))->format('Y-m-d'); | ||
| $this->form->submit($this->formData); | ||
| $this->assertFalse($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testValidSiteImageUrl() | ||
| { | ||
| $this->formData['site_image'] = 'https://example.com/site.png'; | ||
| $this->form->submit($this->formData); | ||
| $this->assertTrue($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testInValidSiteImageNotUrl() | ||
| { | ||
| $this->formData['site_image'] = 'not-a-url'; | ||
| $this->form->submit($this->formData); | ||
| $this->assertFalse($this->form->isValid()); | ||
| } | ||
|
|
||
| public function testInValidSiteImageMaxLength() | ||
| { | ||
| // 形式は有効なURLだが最大長を超えるケース（長さ制約のみを検証） | ||
| $this->formData['site_image'] = 'https://example.com/'.str_repeat('a', $this->eccubeConfig['eccube_stext_len']); | ||
| $this->form->submit($this->formData); | ||
| $this->assertFalse($this->form->isValid()); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
新しいテストメソッドに : void を付けてください。
追加した PHPUnit テストメソッドは戻り値を返しません。各メソッドに : void を宣言してください。
修正例
- public function testValidOpeningHours()
+ public function testValidOpeningHours(): voidAs per coding guidelines, PHP の引数と戻り値には型宣言を付ける。
🤖 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/Form/Type/Admin/ShopMasterTypeTest.php` around lines 173 -
332, Update each newly added test method in ShopMasterTypeTest, including
testValidOpeningHours and the related opening-hours, same_as, employee,
copyright, founding-date, and site-image tests, to declare a void return type.
Keep their existing test logic unchanged.
Source: Coding guidelines
ttokoro20240902
marked this pull request as draft
August 5, 2026 05:26
Contributor
Author
|
@nanasess @dotani1111 レビューをお願いした直後に恐縮ですが、本 PR を一旦 draft に戻します。 先に #6139 を進めるべきと判断しました。まだ着手されていなければ、そのままお待ちいただければ幸いです。 理由本 PR と #6139 が、同じファイルをそれぞれ「新規追加」しています。
4.4 にはどちらも存在しない（あるのは とくに、サイト共通の構造化データについてのレビューは #6139 側に蓄積されています。nanasess さんが 8/3 に出力値そのものを精査してくださっており、未解決スレッドが 11 件残っています。本 PR を先に通すと、そのレビューを迂回して同じ実装を取り込むことになります（ 進め方
|
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要 (Refs #6147, #6136)
店舗設定(BaseInfo)を拡張し、Organization / WebSite の構造化データに以下を出力できるようにします。
スカラー項目
Organization.sameAs（SNS等の公式URL・複数=改行区切り）Organization.foundingDate（稼働開始日）Organization.numberOfEmployees（従業員数, QuantitativeValue）Organization.image（サイト代表画像URL）WebSite.copyrightYear（著作権表示の開始年）営業時間
Organization.openingHoursSpecification（曜日×開店/閉店時刻・複数行）business_hour（自由テキスト・Help表示用）とは別に、構造化データ用の曜日/時刻を保持依存 / 順序（Stacked PR）
SiteStructuredDataServiceに項目を追加する構成）。変更内容
BaseInfoにスカラー5カラム追加 +OpeningHours(dtb_opening_hours) を OneToMany で追加SiteStructuredDataServiceに各項目を反映（値が空の任意プロパティは出力しない）Version20260722000000（BaseInfo列）/Version20260722010000（dtb_opening_hours, Schema APIで両DB対応）SiteStructuredDataServiceTestに各項目のテストを追加テスト
残作業（別軸, #6147）
🤖 Generated with Claude Code
Summary by CodeRabbit
新機能
バグ修正