Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4.1.x用にsitemap.index, sitemap.xml出力機能追加 #4808

Merged
merged 10 commits into from
Jul 7, 2021

Conversation

tao-s
Copy link
Contributor

@tao-s tao-s commented Dec 18, 2020

概要(Overview・Refs Issue)

ref: #4807
sitemap.xml, sitemap.xml出力機能を追加。

  • /sitemap.xml -> sitemap index
    • /sitemap_page.xml -> 商品一覧、詳細以外のページのサイトマップ
    • /sitemap_category.xml -> 商品一覧ページのサイトマップ
    • /sitemap_product.xml -> 商品詳細ページのサイトマップ

方針(Policy)

商品点数やカテゴリが多いと、インデックスが遅くなったりするのと、search console でそれぞれ分けてインデックス状況を見たいので分けました。

実装に関する補足(Appendix)

ページ、商品、カテゴリ、それぞれのupdate_dateをlastmodに入れてます

テスト(Test)

実店舗で稼働しているコードです

相談(Discussion)

毎回入れるの面倒なのと、EC-CUBEはSEO弱くないですって言いたいんで、お願いします。

マイナーバージョン互換性保持のための制限事項チェックリスト

  • 既存機能の仕様変更
  • フックポイントの呼び出しタイミングの変更
  • フックポイントのパラメータの削除・データ型の変更
  • twigファイルに渡しているパラメータの削除・データ型の変更
  • Serviceクラスの公開関数の、引数の削除・データ型の変更
  • 入出力ファイル(CSVなど)のフォーマット変更

レビュワー確認項目

  • 動作確認
  • コードレビュー
  • E2E/Unit テスト確認(テストの追加・変更が必要かどうか)
  • 互換性が保持されているか
  • セキュリティ上の問題がないか

新機能追加に伴う競合確認

  • プラグインとの競合がないか
  • プラグインからのマイグレーションが必要か

@tao-s
Copy link
Contributor Author

tao-s commented Dec 19, 2020

CI走らすために一旦close

@tao-s tao-s closed this Dec 19, 2020
@tao-s tao-s reopened this Dec 19, 2020
@okazy okazy added the enhancement 機能追加 label Dec 19, 2020
@okazy okazy added this to the 4.1 milestone Dec 19, 2020
@tao-s
Copy link
Contributor Author

tao-s commented Dec 23, 2020

user_data/ に管理画面からページ作ってるとエラーになる

@okazy
Copy link
Contributor

okazy commented Mar 26, 2021

商品数が多くなると処理が重たくなるのとページサイズも膨大になってしまうので、ある程度で分割する必要があるかと思います。

少し調べてみました。

サイト 1 つにつき最大 500 個のサイトマップ インデックス ファイルを送信可
指定するURLは50,000個以下
ファイルサイズは未圧縮状態で50MB以下

@okazy okazy changed the base branch from 4.1 to 4.1-feature March 26, 2021 09:01
@okazy okazy changed the base branch from 4.1-feature to 4.1 March 26, 2021 09:02
@okazy okazy changed the base branch from 4.1 to 4.1-feature March 26, 2021 09:07
@okazy
Copy link
Contributor

okazy commented Mar 26, 2021

プルリク先のブランチを 4.1-feature へ変更しました。
Controller を少しリファクタリングしました。

@okazy
Copy link
Contributor

okazy commented Apr 1, 2021

ページのサイトマップでパラメータ付きのURLでエラーとなってしまっていたので対象から除外しました。
entry_activatenoindex の設定ができていなかったので追加しました。

@matsuoshi
Copy link
Contributor

商品数が多くなると処理が重たくなるのとページサイズも膨大になってしまうので、ある程度で分割する必要があるかと思います。

@tao-s @okazy 商品 1000件単位でページ分割する処理を入れまして、プルリクエストお送りしました
ご確認いただけましたらと思います
tao-s#3

sitemap.xml 商品が大量の場合の対応
@tao-s
Copy link
Contributor Author

tao-s commented Apr 16, 2021

@matsuoshi 一応、仕様上はひとつのサイトマップで50,000URLまでとのことなので、1,000件じゃなくてもっと多くても良いかなと思いました

@tao-s
Copy link
Contributor Author

tao-s commented Apr 18, 2021

@okazy 今の状態ってuser_dataにページ作ってもエラーになりませんか?

@okazy
Copy link
Contributor

okazy commented Apr 19, 2021

@tao-s
user_dataにページ作成とは、user_data配下にphpファイルを設置する想定でしょうか?
DBに保存されないのでsitemapには追加されませんが、エラーは発生しないと思います。。。

1,000件じゃなくてもっと多くても

どれくらいがいいでしょうか?
ローカルだと10,000件だとメモリのエラーになったので、貧弱な環境を考えると1000くらいでもいいかと思いました。

@tao-s
Copy link
Contributor Author

tao-s commented Apr 21, 2021

ページのサイトマップでパラメータ付きのURLでエラーとなってしまっていたので対象から除外しました。

@okazy これって、user_data配下とかの事ですか?

@okazy
Copy link
Contributor

okazy commented Apr 21, 2021

@tao-s

ページのサイトマップでパラメータ付きのURLでエラーとなってしまっていたので対象から除外しました。
@okazy これって、user_data配下とかの事ですか?

違います。
例えば entry_activate のルーティングはURLの生成で secret_key の引数が必要です。

* @Route("/entry/activate/{secret_key}/{qtyInCart}", name="entry_activate")

このようなルーティングは URL が確定できず twig でエラーとなってしまっていたため除外しました。

実装は以下です。

// URL に変数が含まれる場合は URL の生成ができないためここで除外する
$Pages = array_filter($Pages, function (Page $Page) {
$route = $this->router->getRouteCollection()->get($Page->getUrl());
if (is_null($route)) {
return false;
}
return count($route->compile()->getPathVariables()) < 1;
});

@okazy
Copy link
Contributor

okazy commented Apr 21, 2021

管理画面のページ管理からページを追加した場合のサイトマップ生成については考慮できていませんでした。
実装確認して対応が必要そうです。

@okazy
Copy link
Contributor

okazy commented Apr 22, 2021

@tao-s 3点対応しました。ご確認と取り込みをお願いできますでしょうか。

tao-s#6

  • 管理画面から作成したページに対応しました。
  • 取得を QueryBuilder のカスタマイズを考慮した実装に変更
  • ページあたりの商品数の設定を eccube.yaml に移動しました

image

image

サイトマップの改善をしました。
@codecov-commenter
Copy link

codecov-commenter commented May 28, 2021

Codecov Report

Merging #4808 (42679be) into 4.1 (8c3ae69) will increase coverage by 0.05%.
The diff coverage is 25.80%.

Impacted file tree graph

@@             Coverage Diff              @@
##                4.1    #4808      +/-   ##
============================================
+ Coverage     75.96%   76.01%   +0.05%     
- Complexity     5946     5948       +2     
============================================
  Files           449      449              
  Lines         20922    20945      +23     
============================================
+ Hits          15893    15921      +28     
+ Misses         5029     5024       -5     
Flag Coverage Δ
tests 76.01% <25.80%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Eccube/Controller/SitemapController.php 40.62% <25.80%> (+40.62%) ⬆️
...be/Service/PurchaseFlow/Processor/TaxProcessor.php 79.62% <0.00%> (-3.71%) ⬇️
src/Eccube/Entity/Page.php 77.90% <0.00%> (+2.32%) ⬆️
src/Eccube/Repository/PageRepository.php 97.77% <0.00%> (+4.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c3ae69...42679be. Read the comment docs.

@matsuoshi
Copy link
Contributor

CI実行のため close/reopen します

@matsuoshi matsuoshi closed this Jul 5, 2021
@matsuoshi matsuoshi reopened this Jul 5, 2021
@matsuoshi matsuoshi changed the base branch from 4.1-feature to 4.1 July 6, 2021 07:09
@matsuoshi matsuoshi merged commit e937c6d into EC-CUBE:4.1 Jul 7, 2021
@tao-s tao-s deleted the 4.1/seo branch November 26, 2021 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants