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

ref: #4807 4.1.x用にcanonical url追加 #4809

Merged
merged 7 commits into from
Jan 27, 2022

Conversation

tao-s
Copy link
Contributor

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

概要(Overview・Refs Issue)

ref: #4807

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

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

レビュワー確認項目

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

@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 tao-s closed this Dec 19, 2020
@okazy
Copy link
Contributor

okazy commented Mar 22, 2021

@tao-s
すみません、反応が遅くなりました。
こちら間違ってクローズされたのではないでしょうか?

動作確認しました。
おおよそ動作問題ないかと思います。

検索ページで Category が null で定義されているのでエラーとなってしまっていました。
それぞれで null のチェックも必要かと思います。

    {# canonical url #}
    {% if Category is defined and Category is not null %}
    <link rel="canonical" href="{{url('product_list')}}?category_id={{Category.id}}" />
    {% elseif Product is defined and Product is not null %}
    <link rel="canonical" href="{{url('product_detail',{"id":Product.id})}}" />
    {% elseif Page is defined and Page is not null %}
    <link rel="canonical" href="{{url(Page.url)}}" />
    {% endif %}

一覧ページのページネーションについて、こちらの実装ではカテゴリページの全てのページに同じ canonical が設定されます。
Google の推奨設定では、各ページでコンテンツ(商品)が異なるため別ページとして扱うのが良いようです。

Googleウェブマスター向け公式ブログより
間違い 1: 複数ページにまたがるコンテンツの 1 ページ目を rel=canonical のリンク先とする
https://webmaster-ja.googleblog.com/2013/05/5-common-mistakes-with-relcanonical.html

Google の推奨設定のためには、全商品表示の商品ページを作成するか、rel="canonical" の代わりに rel=”prev” と rel=”next” を利用する必要がありますが、実装が冗長になってしまう懸念があります。

私の意見としては、推奨設定ではありませんが、ひとまずはいただいている実装で問題ないのではと思いました。

@tao-s
Copy link
Contributor Author

tao-s commented Apr 21, 2021

@okazy こちら解決したんで復活させます

@tao-s tao-s reopened this Apr 21, 2021
@chihiro-adachi chihiro-adachi modified the milestones: 4.1, 4.1.x Sep 3, 2021
@tao-s
Copy link
Contributor Author

tao-s commented Nov 26, 2021

c6d4c6a でcanonicalがmeta.twigに移されたので、商品一覧のページング処理などそっちにもってく

@matsuoshi matsuoshi modified the milestones: 4.1.x, 4.1.2 Jan 14, 2022
- Fix  `Unexpected character "&".`
- Category.title は存在しないため Category.name に修正
- ページング等の処理を meta.twig へ移動
@nanasess
Copy link
Contributor

@tao-s 最新の 4.1 ブランチに追従して、修正を以下に PR しましたのでご確認いただけますでしょうか?
tao-s#7

@matsuoshi matsuoshi self-assigned this Jan 24, 2022
@matsuoshi
Copy link
Contributor

ありがとうございます、
@nanasess さんの修正を @tao-s さんのこのPRにマージいたしました

@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.24%. Comparing base (65af79f) to head (31c9e7e).
Report is 313 commits behind head on 4.1.

Additional details and impacted files
@@             Coverage Diff              @@
##                4.1    #4809      +/-   ##
============================================
- Coverage     68.44%   68.24%   -0.21%     
- Complexity     6121     6143      +22     
============================================
  Files           461      461              
  Lines         25166    25237      +71     
============================================
- Hits          17224    17222       -2     
- Misses         7942     8015      +73     
Flag Coverage Δ
E2E 56.46% <ø> (-0.27%) ⬇️
Unit 76.16% <ø> (-0.01%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@matsuoshi
Copy link
Contributor

matsuoshi commented Jan 25, 2022

マイページの注文履歴詳細のところでテストが落ちているので確認中です

  • マイページ注文履歴詳細 URL: /mypage/history/{order_no}
  • 商品購入/お届け先の追加 URL: /shopping/shipping_edit/{id}

など、URLにパラメタが必須のページについて、canonical URLを設定しようとしてエラーになっている模様
https://github.com/EC-CUBE/ec-cube/pull/4809/files#diff-ebd2f65646a5a6ff6780c9e04fbfe24f7725aff31b8113ec82b41041305c8722R25

@matsuoshi
Copy link
Contributor

matsuoshi commented Jan 26, 2022

システムのデフォルトのページについて、先のコメントのように URLにパラメタが必須のページの場合、エラーとなっておりました。今回は

  • 商品一覧・詳細など、主要なページのcanonicalは設定できている
  • システムのデフォルトページについてはcanonicalをつける優先度は低い

と判断して、システムデフォルトページの場合 canonical 生成をスキップするように修正し、4.1.2 に取り込めればと考えています。
@tao-s いかがでしょうか?

@chihiro-adachi
Copy link
Contributor

○ページ目の記述をメッセージファイルに切り出しました。
その他動作確認しましたが問題ないかと思います。

@chihiro-adachi chihiro-adachi merged commit 9aaabf5 into EC-CUBE:4.1 Jan 27, 2022
@chihiro-adachi
Copy link
Contributor

@tao-s
いったんマージさせていただきました。
こちら一部挙動を変更していますので、もう少しこうしたい、などありましたらPRいただければと。
#4809 (comment)

ご対応ありがとうございました。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 機能追加
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants