chore: Fixed an issue with canonical URLs containing query strings #328
Conversation
colinmurphy
requested review from
ahuseyn,
Copilot and
moonmeister
and removed request for
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue with canonical URLs by ensuring that query strings are removed from the URL before rendering the canonical tag.
- Replaces the ternary operator initialization with an if block to remove query strings.
- Ensures the canonical URL is generated without any query parameters.
Co-authored-by: Copilot <[email protected]>
|
Check out the recent updates to your Headless Platform preview environment:
Learn more about preview environments in our documentation. |
|
Check out the recent updates to your Headless Platform preview environment:
Learn more about preview environments in our documentation. |
|
Check out the recent updates to your Headless Platform preview environment:
Learn more about preview environments in our documentation. |
|
Nice @colinmurphy, I've tested too! |
moonmeister
left a comment
moonmeister
left a comment
There was a problem hiding this comment.
See comment, I'm wondering if this is the correct fix
| // eslint-disable-next-line no-restricted-globals, n/prefer-global/process, n/prefer-global/url | ||
| const urlObject = new URL(url, process.env.NEXT_PUBLIC_SITE_URL); | ||
| // Remove any query string as search is writable so we remove any potential query string | ||
| urlObject.search = ""; |
There was a problem hiding this comment.
This fixes the search string issues but I could imagine others potentially. I'm wondering if this is the correct fix or if the issue is the URL we are passing in is from the router and not from WP.
There was a problem hiding this comment.
@moonmeister Ah I see your point. Let me fix this. Thanks for spotting this ❤️
There was a problem hiding this comment.
@moonmeister Sorry I might be missing something here.
But if you have a look at the blog post slug template we have
const { title, date, author, uri, excerpt, editorBlocks } = post;
and that uri is used for the SEO component
<Seo
title={title}
url={uri}
description={excerpt.replaceAll(/<\/?\S+>/gm, "")}
/>
So it looks to me we are using the WP URL and sorry if I am totally missing something.
There was a problem hiding this comment.
@colinmurphy Interesting. Well, the bad URL is getting in there somehow. Maybe we need to dig deeper.
There was a problem hiding this comment.
FWIW, this scenario cleans things:
There was a problem hiding this comment.
I just updated the original ticket with a video. TL;DR: This only affects doc pages as supported by my testing and the analytics page I linked to there.
There was a problem hiding this comment.
Thanks @moonmeister 🚀
@alexwpengine is going to have a look. (Thanks @alexwpengine )
There was a problem hiding this comment.
Fix for doc pages not in SEO component as discussed in previously
|
Not the correct fix. @alexwpengine working on new PR |
fixes #325