Remove querystring from generated canonical links #8261
Conversation
cfgov/v1/jinja2/v1/layouts/base.html
Outdated
| @@ -65,7 +65,7 @@ | |||
| <base target="_blank"> | |||
| {% endif %} | |||
|
|
|||
| <link rel="canonical" href="{{ request.build_absolute_uri() | lower }}"> | |||
| <link rel="canonical" href="{{ request.build_absolute_uri().split('?')[0] | lower }}"> | |||
There was a problem hiding this comment.
Is there ever a distinction here between build_absolute_uri() and something like {{ request.scheme }}://{{ request.get_host }}{{ request.path }}. It looks like probably not from the build_absolute_uri source.
My preference would to do less Python in templates than more, so either using the constituent properties of request or moving the string slicing into a template tag.
There was a problem hiding this comment.
Done in 5bfe0a1. I hard-coded https because that will be true everywhere that canonical links matter
There was a problem hiding this comment.
From my testing, I think the | lower is superfluous as well (as there is a redirect or lower upstream), but I didn't look too hard at it
There was a problem hiding this comment.
I vaguely recall a need to explicitly lower-case some URLs for canonicalization purposes.
There was a problem hiding this comment.
Yeah, happy to leave it without a strong reason to remove
There was a problem hiding this comment.
Could we use request.build_absolute_uri(request.path) to avoid the need to strip the querystring in such an ugly way? See docs.
|
Is there any reason to not to use the same value in the |
Done in b94052. I also squeezed in andy's proposed method, which is prettier than tilde concatenation. |
This PR changes how canonical links are generated in
base.html, specifically it strips the querystring. Across all the pages I audited (perhaps non-exhaustive) the querystring is simply used as a filter or as a way to track specific selections. The whole point of canonical links is to collate these views of the same page into one... canonical link... that google can then reference and give more link equity to, instead of diffusing it across N pages (where N is the number of pages in a filterable list or w/e).Testing: