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 slash and params sort #99

Closed
wants to merge 4 commits into from

Conversation

webysther
Copy link

@webysther webysther commented Jan 23, 2025

This PR supersede #98

Related to: sissbruecker/linkding#957

@webysther webysther mentioned this pull request Jan 23, 2025
@sissbruecker
Copy link
Owner

I'm not 100% sure what you mean by "fix slash" - I assume this is intended to always add a slash at the end of the URL?

In any case, these changes are not a valid option:

  • Whether the URL ends with a slash or not is relevant, you can not assume that both will work. For example https://demo.linkding.link/bookmarks works without a slash, but does not work with a slash. OTOH the linkding REST API URLs currently require a trailing slash. It's the same for a lot of other websites or apps.
  • As for query params, ?a=1&b=2 does not necessarily equal ?b=2&a=1. While this might be more of an edge case, a web application might very well have custom logic to process the query string and have an implementation that relies on the order of query params, whether intentional or not.

So changing the bookmarked URL or the query params might end up saving a bookmark that does point at the same page, or a non-working URL. Besides, if sissbruecker/linkding#957 would be implemented, then this change would be pointless anyway. If the check for existing bookmarks would use a canonical URL, which is more reasonable than straight-up changing the bookmarked URL, then it doesn't matter if the bookmark was saved with a canonical URL or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants