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

FIX: remove trailing slashes and query params on meta-tag-updater's canonical url #24445

Merged
merged 5 commits into from Nov 27, 2023

Conversation

renato
Copy link
Contributor

@renato renato commented Nov 18, 2023

This uses the same strategy from the server-side canonical URL generation to avoid inconsistencies.

@renato
Copy link
Contributor Author

renato commented Nov 18, 2023

The test failures are were underlying issues being surfaced: when we use visit("route") without a leading slash, getAbsoluteURL returns something like http://localhost:3000route, which is the invalid URL exception causing these failures.

The simplest fix is just adding these leading slashes in the few places they're missing, which I think is expected.

The alternative is handling this exception before calling getAbsoluteURL, but I'm pretty sure this will never happen in a production environment, because url comes from Ember's router.

We also have a condition on getURL that relies on this leading slash to decide if it's a relative URL:

// if it's a non relative URL, return it.
if (url !== "/" && !/^\/[^\/]/.test(url)) {
return url;
}

CvX added a commit to CvX/discourse-locations that referenced this pull request Nov 21, 2023
Omitting `/` prefix was incorrect and could break in the future (see: discourse/discourse#24445)
CvX added a commit to discourse/discourse-data-explorer that referenced this pull request Nov 21, 2023
Omitting `/` prefix was incorrect and could break in the future (see: discourse/discourse#24445)
@CvX
Copy link
Contributor

CvX commented Nov 21, 2023

The simplest fix is just adding these leading slashes in the few places they're missing, which I think is expected.

I submitted two PRs to plugins (see the linkbacks above) and there are two more instances of this in a single test file in core. Just go ahead and fix them here. No need to support incorrect paths 😃

CvX added a commit to discourse/discourse-data-explorer that referenced this pull request Nov 21, 2023
Omitting `/` prefix was incorrect and could break in the future (see: discourse/discourse#24445)
@renato renato merged commit d93be8c into main Nov 27, 2023
16 checks passed
@renato renato deleted the fix_meta_tag_updater_canonical_url branch November 27, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants