Opened on 12/17/2015 at 05:02:50 PM
Closed on 12/18/2015 at 06:30:55 PM
#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/ |
||
Description (last modified by sebastian)
How to reproduce
- Go to https://eyeo.com/en/jobs
- 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.
Attachments (0)
Change History (23)
comment:1 Changed on 12/17/2015 at 05:09:25 PM by trev
comment:3 follow-up: ↓ 7 Changed on 12/17/2015 at 05:13:24 PM 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 on 12/17/2015 at 05:39:13 PM by saroyanm
- Priority changed from Unknown to P3
- Ready set
comment:5 Changed on 12/18/2015 at 09:41:15 AM by saroyanm
- Owner set to saroyanm
comment:6 Changed on 12/18/2015 at 10:31:17 AM by saroyanm
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:8 Changed on 12/18/2015 at 12:57:32 PM by saroyanm
- Resolution set to fixed
- Status changed from reviewing to closed
comment:9 follow-up: ↓ 10 Changed on 12/18/2015 at 01:59:42 PM 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 on 12/18/2015 at 02:11:34 PM 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 on 12/18/2015 at 02:35:47 PM by trev
I guess we can do that if we generalize it ("index" => defaultpage).
comment:12 Changed on 12/18/2015 at 04:52:16 PM 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: ↓ 15 Changed on 12/18/2015 at 05:36:07 PM 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 on 12/18/2015 at 05:38:33 PM by saroyanm
- Review URL(s) modified (diff)
- Status changed from reopened to reviewing
comment:15 in reply to: ↑ 13 ; follow-up: ↓ 17 Changed on 12/18/2015 at 05:44:13 PM 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 on 12/18/2015 at 05:48:09 PM 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: ↓ 18 Changed on 12/18/2015 at 05:51:40 PM 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 on 12/18/2015 at 05:54:41 PM 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:19 Changed on 12/18/2015 at 05:56:43 PM by saroyanm
comment:20 Changed on 12/18/2015 at 06:03:31 PM 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 on 12/18/2015 at 06:19:42 PM by sebastian
- Review URL(s) modified (diff)
- Status changed from reopened to reviewing
comment:22 Changed on 12/18/2015 at 06:30:41 PM by saroyanm
comment:23 Changed on 12/18/2015 at 06:30:55 PM by saroyanm
- Resolution set to fixed
- Status changed from reviewing to closed
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.