Opened 4 years ago

Closed 4 years ago

#3435 closed defect (fixed)

<link rel=canonical> is set to ../index

Reported by: sebastian Assignee: saroyanm
Priority: P3 Milestone:
Module: Websites Keywords:
Cc: trev, kzar, saroyanm Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29332874/
https://codereview.adblockplus.org/29332884/
https://codereview.adblockplus.org/29332890/

Description (last modified by sebastian)

How to reproduce

  1. Go to https://eyeo.com/en/jobs
  2. Inspect the page an look for the <link rel=canonical> element

Observed behaviour

The canonical URL is indicated as https://eyeo.com/jobs/index. Also when searching Google for "eyeo jobs" this URL shows up, because of that.

Expected behaviour

The canonical URL should be simply https://eyeo.com/jobs/.

What to change

Modify templates/default.tmpl so that the last part is removed from {{page}} for canonical links if that part is index.

Change History (23)

comment:1 Changed 4 years ago by trev

It's the same with https://eyeo.com/ - canonical link is https://eyeo.com/index. The link is working correctly, so it's really only about nicer URLs.

comment:2 Changed 4 years ago by trev

  • Description modified (diff)

comment:3 follow-up: Changed 4 years ago by trev

Note that adblockbrowser.org would normally have the same issue but it doesn't have <meta name="canonical"> in the template for some reason.

comment:4 Changed 4 years ago by saroyanm

  • Priority changed from Unknown to P3
  • Ready set

comment:5 Changed 4 years ago by saroyanm

  • Owner set to saroyanm

comment:6 Changed 4 years ago by saroyanm

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:7 in reply to: ↑ 3 Changed 4 years ago by saroyanm

Replying to trev:

Note that adblockbrowser.org would normally have the same issue but it doesn't have <meta name="canonical"> in the template for some reason.

Ticket is created(#3438) will handle as well after this is fixed.

comment:8 Changed 4 years ago by saroyanm

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:9 follow-up: Changed 4 years ago by sebastian

How about adding that filter to the CMS, that we don't need to duplicate the logic in several websites?

comment:10 in reply to: ↑ 9 Changed 4 years ago by saroyanm

Replying to sebastian:

How about adding that filter to the CMS, that we don't need to duplicate the logic in several websites?

Sounds good.

comment:11 Changed 4 years ago by trev

I guess we can do that if we generalize it ("index" => defaultpage).

comment:12 Changed 4 years ago by sebastian

  • Resolution fixed deleted
  • Status changed from closed to reopened

One more thing I noticed; now the canonical URL is https://eyeo.com/jobs, this URL however redirects to https://eyeo.com/en/jobs/. So I think the canonical URL should be https://eyeo.com/jobs/ (with trailing slash), which doesn't cause a redirect. As this also is how it was specified in the issue description, I reopen the issue.

comment:13 follow-up: Changed 4 years ago by sebastian

After giving it a second thought, I think we don't need a filter at all. I know that has been discussed before in the code review. However, the solution suggested there was complex enough to justify writing a filter. But after all the same can be achieved with a single template expression:

<link rel="canonical" href="https://eyeo.com/{{ page[:-5] if page.endswith('/index') else page }}">

comment:14 Changed 4 years ago by saroyanm

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing

comment:15 in reply to: ↑ 13 ; follow-up: Changed 4 years ago by saroyanm

Replying to sebastian:

After giving it a second thought, I think we don't need a filter at all. I know that has been discussed before in the code review. However, the solution suggested there was complex enough to justify writing a filter. But after all the same can be achieved with a single template expression:

<link rel="canonical" href="https://eyeo.com/{{ page[:-5] if page.endswith('/index') else page }}">

Yep, updated the patch

comment:16 Changed 4 years ago by sebastian

  • Description modified (diff)
  • Summary changed from <meta name=canonical> is set to ../index to <link rel=canonical> is set to ../index

comment:17 in reply to: ↑ 15 ; follow-up: Changed 4 years ago by sebastian

Replying to saroyanm:

Yep, updated the patch

Thanks, LGTM. The next step, after merging this, is to adapt other websites, in particular https://adblockbrowser.org which doesn't have a <link rel=canonical> yet.

comment:18 in reply to: ↑ 17 Changed 4 years ago by saroyanm

  • Resolution set to fixed
  • Status changed from reviewing to closed

Replying to sebastian:

Replying to saroyanm:

Yep, updated the patch

Thanks, LGTM. The next step, after merging this, is to adapt other websites, in particular https://adblockbrowser.org which doesn't have a <link rel=canonical> yet.

Sure, thanks.

comment:20 Changed 4 years ago by sebastian

  • Resolution fixed deleted
  • Status changed from closed to reopened

Sorry, I found one more issue in the review. Unfortunately, you already merged the patch. My bad.

comment:21 Changed 4 years ago by sebastian

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing

comment:23 Changed 4 years ago by saroyanm

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.