Organization, Websiteのマークアップを追加 - #6139
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.4 #6139 +/- ##
==========================================
+ Coverage 77.21% 77.22% +0.01%
==========================================
Files 563 566 +3
Lines 28111 28193 +82
==========================================
+ Hits 21705 21772 +67
- Misses 6406 6421 +15
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:
|
|
@tao-s |
|
ref: #6147 |
a43cd12 to
b36d6a5
Compare
|
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:
📝 WalkthroughWalkthrough
Changes構造化データ出力
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant TwigInitializeListener
participant SiteStructuredDataService
participant default_frame.twig
participant Browser
Request->>TwigInitializeListener: フロント変数を初期化
TwigInitializeListener->>SiteStructuredDataService: createWebSiteJsonLd()
SiteStructuredDataService-->>TwigInitializeListener: site_json_ld
TwigInitializeListener->>default_frame.twig: Twigグローバルを登録
default_frame.twig-->>Browser: JSON-LD scriptを出力
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
メンテナ作業として、マージに向けて本PRを更新しました( 変更点
ローカル検証(Docker / PostgreSQL)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Eccube/Resource/template/default/Schema/website.twig (1)
8-14: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win廃止済みのSearchAction出力を見直してください。
Googleのサイトリンク検索ボックスは2024年11月21日に終了し、現在はRich Results TestやSearch Consoleでもサポートされていません。
WebSiteのname/urlは残しつつ、Google向けだけであればpotentialActionは削除するのが適切です。 (developers.google.com)🤖 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/Schema/website.twig` around lines 8 - 14, Remove the obsolete potentialAction SearchAction block from the WebSite structured-data markup in the website template, while preserving the existing WebSite name and url fields.src/Eccube/Resource/template/default/default_frame.twig (1)
190-192: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWebSite/Organizationの出力をホームページに限定してください。
default_frame.twigは共通フレームのため、Line 190-192 はこれを継承する全ページへ出力されます。Googleのサイト名用WebSite構造化データはホームページに配置する必要があり、全ページへの出力は不要です。ホームページ専用テンプレートへ移すか、実際のホームページルートで条件分岐してください。 (developers.google.com)🤖 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/default_frame.twig` around lines 190 - 192, default_frame.twigの共通フレームにあるWebSite/Organization構造化データ出力を削除し、ホームページ専用テンプレートへ移すか、ホームページのルートでのみ実行される条件分岐を追加してください。Schema/website.twigの内容がサイト内の全ページへ出力されないようにしてください。
🤖 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/Resource/template/default/Schema/organization.twig`:
- Around line 4-27: Replace the interpolated JSON-LD values in organization.twig
with a single object passed through the existing json_ld filter, preserving the
current Organization properties and structure. Apply the same json_ld-based
output to website.twig, including its author value, so both templates use the
shared escaping path; update
src/Eccube/Resource/template/default/Schema/organization.twig lines 4-27 and
src/Eccube/Resource/template/default/Schema/website.twig lines 4-7 accordingly.
---
Nitpick comments:
In `@src/Eccube/Resource/template/default/default_frame.twig`:
- Around line 190-192:
default_frame.twigの共通フレームにあるWebSite/Organization構造化データ出力を削除し、ホームページ専用テンプレートへ移すか、ホームページのルートでのみ実行される条件分岐を追加してください。Schema/website.twigの内容がサイト内の全ページへ出力されないようにしてください。
In `@src/Eccube/Resource/template/default/Schema/website.twig`:
- Around line 8-14: Remove the obsolete potentialAction SearchAction block from
the WebSite structured-data markup in the website template, while preserving the
existing WebSite name and url fields.
🪄 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: 88556e13-120f-4cb7-af24-011ff4cc43ac
📒 Files selected for processing (3)
src/Eccube/Resource/template/default/Schema/organization.twigsrc/Eccube/Resource/template/default/Schema/website.twigsrc/Eccube/Resource/template/default/default_frame.twig
b36d6a5 to
8efe90a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Service/SiteStructuredDataService.php`:
- Around line 107-109: Update the invoice registration number check in
SiteStructuredDataService so both null and empty-string values are excluded
before assigning data['iso6523Code']; preserve the existing 0221: prefix for
non-empty values and add a regression test covering an empty registration
number.
🪄 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: c2495ce0-3531-40ed-93c6-acdb75b76b27
📒 Files selected for processing (4)
src/Eccube/EventListener/TwigInitializeListener.phpsrc/Eccube/Resource/template/default/default_frame.twigsrc/Eccube/Service/SiteStructuredDataService.phptests/Eccube/Tests/Service/SiteStructuredDataServiceTest.php
Organization / WebSite の JSON-LD をテンプレートの文字列補間で組み立てていたため、 店名等に " や </script> が含まれると構造化データが壊れる/XSS 経路になりうる問題があった (CodeRabbit 指摘)。EC-CUBE#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 EC-CUBE#6136 EC-CUBE#6147 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
8efe90a to
2c79b6b
Compare
|
CodeRabbit の指摘(
ローカル QA: php-cs-fixer 0 / phpstan(level6) No errors / phpunit 6 tests 20 assertions OK。 |
Packages::getUrl() はルート相対パス(/html/user_data/...)を返すため, logo.contentUrl が相対パスのまま JSON-LD に出力されていた. ProductStructuredDataService の画像URLと同様に絶対URLへ揃える. - generateAbsoluteAssetUrl() を追加し, ルーティングコンテキストの スキーム込みホストを前置する(base_urls 設定済みで既に絶対URLならそのまま) - logo.contentUrl が絶対URLであることを検証するテストを追加 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nanasess
left a comment
There was a problem hiding this comment.
レビュー: サイト共通の構造化データ(WebSite / Organization)
差分(4 files / +349 -1)を読み、SiteStructuredDataService が出力する各プロパティを BaseInfo の実際の格納値・フォーム制約と突き合わせて確認しました。あわせて Google の Organization 構造化データのドキュメントと Peppol の ISO 6523 ICD リストを参照しています。
Service への切り出し・json_ld フィルタ経由の出力・空値の省略方針は ProductStructuredDataService と揃っていて、方向性は良いと思います。テストも配列を直接アサートできる形になっていて読みやすいです。
一方で、出力される値そのものに確認したい点がいくつかありました。
マージ前に対応したい点
telephoneに+81-を無条件で前置しているため+81-0312345678になります。PhoneNumberTypeの制約上、電話番号を設定した国内店舗はほぼ全てこの値です。国番号は必須ではないので、保存値をそのまま出す形を提案します。→ [1]Organization.emailにemail01(送信元メールアドレス / 全送信メールの BCC 先)を出力している。公開する連絡先としてはemail02(問い合わせ専用)が妥当だと考えます。→ [2]
相談したい点
logoをfavicon.ico(ICO 形式・既定は EC-CUBE のアイコン)から、ファイル管理でアップロードするuser_data/assets/img/common/logo.pngに変える案。未設定ならlogoを出力しない形です。→ [3]- Organization を全フロントページに出す是非(Google は「every page に入れる必要はない」としています)。→ [4]
WebSite.authorではなくpublisher/@graphにする案。→ [5]
既定インストールの状態について
動作確認の前提として共有です。インストーラが dtb_base_info に設定するのは shop_name と email01〜email04 だけ(src/Eccube/Controller/Install/InstallController.php:853-865)で、phone_number / postal_code / addr01 / addr02 / pref_id / company_name / good_traded / message は NULL のまま です。
そのため素の環境では telephone と address が一切出力されず、CI でもこの経路は通っていません(Codecov の 8 lines missing)。リッチリザルトテストで確認する際は、店舗設定に値を入れた状態でお願いします。
確認して問題がなかった点
json_ldフィルタ(JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT)経由の出力なので、店舗設定の値に</script>や"が入っても JSON 破壊・XSS にはなりません。iso6523Codeの0221:は Peppol の ISO 6523 ICD リスト上「The registered number of the qualified invoice issuer (Japan)」(発行機関: 国税庁)で、T付きの適格請求書発行事業者登録番号をそのまま入れる用途として正しいです。FilemtimeVersionStrategyはファイル不在時に例外を投げず空バージョンを返すため、logo.contentUrlの生成で 500 になることはありません。getPref()?->getName()の null 安全化により、PR コメントで報告されていた「都道府県未設定で全フロントページが 500」は解消されています。preview時はsetFrontVariables()が早期 return するためsite_json_ldグローバルが未定義になりますが、テンプレート側がis definedでガードしているので問題ありません。
CI は全ジョブ green(PHPUnit 8.5/pgsql・PHPStan・php-cs-fixer・E2E・plugin-test)、Codecov patch coverage は 89.74%(8 lines missing)です。
@ttokoro20240902 ご確認をお願いします。
|
@nanasess 詳細なレビューをありがとうございます。全 11 件を実コードと突き合わせて確認しました。対応方針をまとめます。 まず前提の共有です。本 PR と同じ 対応する(8 件)
方針を確認したい(2 件)
[4] と [5] は出力構造そのものが変わるため、テストの書き換えが [1]〜[9] の修正と重なります。先に [4] の方針を確定させてから、まとめて 1 回で修正・テスト更新を行いたいと考えています。[4] についてご意見をいただければ着手します。 |
nanasess のレビュー指摘と dotani1111 の指摘に対応。
- telephone: `+81-` の無条件前置を廃止し保存値をそのまま出力する。
phone_number は PhoneNumberType の TruncateHyphenListener と Assert\Type('digit')
によりハイフンなしの数字列(例 0312345678)で保存されるため、+81 を前置すると
国内トランクプレフィックスの 0 が残った不正な値になっていた。Organization と
ContactPoint の 2 箇所を修正。
- email: email01(送信元 From かつ全送信メールの BCC 先)ではなく
email02(問い合わせ専用)を出力する。送信専用・店舗内部の運用アドレスが
全ページで公開されるのを避ける。
- logo: ファビコン(ICO は Google Images の対応形式外)から、帳票 PDF と共通の
店舗ロゴ assets/pdf/logo.png へ変更。優先順は OrderPdfService に合わせ、
user_data に配置されていればそれを使い、無ければ管理画面テンプレート同梱の
既定ロゴへフォールバックする。
- 構造: WebSite に Organization を author として内包する形をやめ、@graph で並列の
トップレベルノードにし WebSite.publisher から @id 参照する。@context も
トップレベルに 1 つだけ置き重複を解消。
- description: ProductStructuredDataService の正規化(strip_tags → 空白正規化 →
300 文字丸め)を StructuredDataDescriptionTrait に切り出して共通化し、
good_traded と message に適用する。
- ContactPoint: contactType(customer support)を追加。
- アセットURL: プロトコル相対URL(//cdn...)にスキームを補って絶対URLにする。
JSON-LD では @base の無い相対 IRI となり解決先が曖昧になるため。
- 出力ページ: TwigInitializeListener でトップページ(homepage)と
「当サイトについて」(help_about)の 2 ページに限定する。Google の Organization
ドキュメントが全ページへの出力を必要としていないため、対象外では組み立て自体を行わない。
- テスト: @graph 構造・publisher の @id 参照・telephone の書式・description の正規化・
email の出力先を検証。出力ページの限定は Web テストで検証し、ガードを外すと
落ちることも確認済み。
|
@nanasess ご指摘とご提案をすべて反映して push しました( 対応内容
出力例(既定インストール+電話番号設定時){
"@context": "https://schema.org",
"@graph": [
{
"@type": "WebSite",
"@id": "https://example.com/#website",
"name": "EC-CUBE SHOP",
"url": "https://example.com/",
"potentialAction": { "@type": "SearchAction", "...": "..." },
"publisher": { "@id": "https://example.com/#organization" }
},
{
"@type": "Organization",
"@id": "https://example.com/#organization",
"url": "https://example.com/",
"logo": { "@type": "ImageObject", "contentUrl": "https://example.com/html/user_data/assets/pdf/logo.png" },
"name": "EC-CUBE SHOP",
"telephone": "0312345678",
"contactPoint": { "@type": "ContactPoint", "contactType": "customer support", "...": "..." }
}
]
}テスト
1 点ご相談[3] のロゴについて、 |
Rector が追加した use 文の位置が ordered_imports に反しており CI の php-cs-fixer ジョブが落ちていた。ローカルの局所実行は .php-cs-fixer.cache の キャッシュヒットで検出できなかったため --using-cache=no で適用した。
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Service/SiteStructuredDataService.php`:
- Around line 208-216: Update SiteStructuredDataService::buildLogoUrl so it
never falls back to the admin report logo: return the user_data logo only when
it is a configured SEO-compliant logo, otherwise return null, and ensure the
caller omits the Organization.logo property when no valid logo exists. In
tests/Eccube/Tests/Service/SiteStructuredDataServiceTest.php lines 102-107,
remove the assertion accepting the admin fallback and verify omission when unset
or inclusion only for a compliant dedicated logo.
In `@src/Eccube/Service/StructuredDataDescriptionTrait.php`:
- Around line 33-35: Update the description normalization in
StructuredDataDescriptionTrait so br and block-level HTML elements are replaced
with spaces before strip_tags() removes tags, preserving separation between
adjacent content such as neighboring p elements. Keep the existing whitespace
normalization and trimming, and add a regression test covering adjacent block
elements.
🪄 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: 9f7a672e-967d-4a35-a3e6-958f896c519b
📒 Files selected for processing (6)
src/Eccube/EventListener/TwigInitializeListener.phpsrc/Eccube/Service/ProductStructuredDataService.phpsrc/Eccube/Service/SiteStructuredDataService.phpsrc/Eccube/Service/StructuredDataDescriptionTrait.phptests/Eccube/Tests/Service/SiteStructuredDataServiceTest.phptests/Eccube/Tests/Web/SiteStructuredDataOutputTest.php
- logo: 要件を満たす画像が無ければ出力しない。Google の Organization ドキュメントは 幅・高さともに 112px 以上を求めており、既定の帳票ロゴは 301x38 で満たさない。 file_exists だけでは画像サイズを保証できないため getimagesize で判定し、 満たさない場合は logo を省略する(logo は任意プロパティ)。 - 説明文の正規化: strip_tags はタグを削除するだけなので <p>商品A</p><p>商品B</p> が「商品A商品B」と連結してしまう。br とブロック要素の 境界を空白へ置換してからタグを除去する。隣接ブロック要素の回帰テストを追加。 - TwigInitializeListener: onKernelRequest で取得済みの BaseInfo を setFrontVariables へ渡し、同一リクエスト内の二重取得を解消。 引数は後方互換のため任意にして未指定時は従来どおり取得する。
ロゴの参照先を帳票 PDF 共通の assets/pdf/logo.png から、構造化データ専用の html/user_data/assets/img/common/logo.png へ変更する。 帳票ロゴは LoadDataFixturesEccubeCommand がインストール時に管理画面同梱の 301x38 を user_data へコピーするため既定で必ず存在し、共有するとロゴを 差し替えていない店舗でも EC-CUBE 既定のロゴが Organization.logo として 出力される。また OrderPdfService は幅 40mm 固定・高さアスペクト比で描画するため、 Google の要件(112x112 以上)を満たす画像を置くと帳票のレイアウトが崩れる。 あわせて getimagesize() によるサイズ判定を撤去し、ファイルの有無だけで 出力を決める。サイズ・形式の要件はドキュメントで案内する。 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nanasess
left a comment
There was a problem hiding this comment.
LGTM
5985f081 までの対応を確認しました。指摘した 10 点すべてが解消されています。
対応確認
| 指摘 | 対応 |
|---|---|
[1] telephone への +81- 無条件前置 |
前置を廃止し保存値をそのまま出力。Organization / ContactPoint の 2 箇所とも |
[2] Organization.email に email01 |
email02(問い合わせ専用)へ変更 |
[3] logo がファビコン |
構造化データ専用パス user_data/assets/img/common/logo.png へ移動。帳票ロゴとは非共有、実行時のサイズ判定なし |
| [4] 全フロントページへの出力 | homepage / help_about に限定。対象外では組み立て自体を行わない |
[5] WebSite.author へのネスト / @context 重複 |
@graph で並列ノード化し publisher から @id 参照。@context はトップレベルに 1 つ |
[6] telephone のテスト不足 |
testTelephoneIsOutputAsStored を追加 |
[7] description の無加工 |
StructuredDataDescriptionTrait へ共通化。ブロック要素境界の区切り保持まで改善 |
[8] contactPoint.contactType なし |
customer support を追加 |
| [9] プロトコル相対URL | スキームを補って絶対URL化 |
[10] BaseInfo の二重取得 |
setFrontVariables() へ引き渡し(引数は任意で BC 維持) |
こちらで確認した点
- CI は 128 pass(
dockerbuild / mergeのみ matrix 由来の skipping) assets/img/common/logo.pngはeccube_file_uploadable_extensionsにpngがあるためファイル管理から配置可能です(app/config/eccube/packages/eccube.yaml:174-187)。同ディレクトリは favicon の配置先としてLoadDataFixturesEccubeCommandが作成するのでディレクトリ新設も不要で、既存参照との衝突もありませんlogoの出力経路は、帳票ロゴ流用時は既定画像がサイズ要件を満たさず CI で一度も実行されない状態でしたが、testPlacedLogoIsOutputAsAbsoluteUrlが 1x1 PNG を配置して検証するようになったため通るようになっていますStructuredDataDescriptionTraitへの共通化により、マージ済みのProductStructuredDataService(#6883)の出力も<p>A</p><p>B</p>→ 「A B」に変わりますが、改善方向でテストも追加されているため問題ないと判断しました
ドキュメント(doc4)への補足は別途起票いただけるとのことなので、そちらでお願いします。
@ttokoro20240902 丁寧な対応ありがとうございました。
|
LGTM |
#6139 が 4.4 へマージされ、SiteStructuredDataService とそのテストが本体側に 入ったため、本ブランチの「新規追加」を 4.4 版への追記へ組み替える。 - BaseInfo: 4.4 の mcp_enabled と本ブランチの構造化データ項目を両方残す - SiteStructuredDataService: 4.4 版(@graph・publisher 参照・normalizeDescription・ buildLogoUrl・email02)を土台にし、本ブランチ固有の copyrightYear / image / foundingDate / numberOfEmployees / sameAs / openingHours を載せる - WebSite.author に Organization を内包する形は 4.4 の publisher 参照へ寄せた - description は normalizeDescription 経由、email は email01 でなく email02 - テスト: copyrightYear 検証を @graph 構造へ追随させる Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
概要(Overview・Refs Issue)
#6136 店舗情報などの基本的な構造化データの拡充
サイト全体共通のJSON-LDのマークアップを追加
方針(Policy)
Googleが推奨するプロパティを可能な限り追加
実装に関する補足(Appendix)
現状のEC-CUBEからは取得できない情報はコメントアウトしてあります
テスト(Test)
リッチリザルトテストで確認済み

相談(Discussion)
会社情報やショップの稼働開始日、最終更新日時など現状では取得できないプロパティに関して店舗設定などに追加するかどうかを相談したいです。
マイナーバージョン互換性保持のための制限事項チェックリスト
レビュワー確認項目
Summary by CodeRabbit