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

multi-segment slugs return 404 when ending in a / #1531

Closed
eleith opened this issue Aug 31, 2022 · 4 comments · Fixed by #1532
Closed

multi-segment slugs return 404 when ending in a / #1531

eleith opened this issue Aug 31, 2022 · 4 comments · Fixed by #1532
Labels
Milestone

Comments

@eleith
Copy link

eleith commented Aug 31, 2022

How Shlink is set-up

  • Shlink Version: 3.2.1
  • PHP Version: x.y.z
  • How do you serve Shlink: docker / pg

Summary

https://shlink.io/documentation/some-features/#multi-segment-custom-slugs

in the considerations section, it might be worth documenting that you get a 404 on slugs that end with a /

this happens already to any slug, but with multi-segement slugs, if you have the following

http://example.com/store/donut and http://example.com/store

it becomes a bit more common to accidentally leave the / on the shorter slug

Current behavior

you get a 404 on any short URL that ends with a forward slash

Expected behavior

either this is documented, particularly in the multi-segment docs or a feature is introduced to assume when a request ends in a /, the / can safely be ignored

@eleith eleith added the bug label Aug 31, 2022
@eleith
Copy link
Author

eleith commented Aug 31, 2022

related: #1090

perhaps it's already documented elsewhere and i missed it.

@acelaya acelaya added troubleshooting Tickets in which an issue was happening which was not obvious to solve and required support docs and removed bug labels Sep 1, 2022
@acelaya
Copy link
Member

acelaya commented Sep 1, 2022

I will try to document it better

@acelaya acelaya added feature and removed docs troubleshooting Tickets in which an issue was happening which was not obvious to solve and required support labels Sep 5, 2022
@acelaya acelaya added this to the 3.3.0 milestone Sep 5, 2022
@acelaya acelaya added this to Shlink Sep 5, 2022
@acelaya acelaya moved this from Todo 🗒️ to In Progress 📝 in Shlink Sep 5, 2022
@acelaya acelaya moved this to Todo 🗒️ in Shlink Sep 5, 2022
@acelaya
Copy link
Member

acelaya commented Sep 5, 2022

I have actually realized that supporting trailing slashes is quite easy. I don't remember what was the challenge back in the day when I investigated it. Could be that something has changed in the codebase that makes it easier now.

I will add a new env var/config option that will allow enabling support for trailing slashes, and document the considerations, being:

  • If you enable multi-segment slugs and explicitly create a URL with the slug whatever/, Shlink will try to match only whatever as the slug, resulting in a 404 (considering you enable support for triling slashes, of course).
  • If you create both whatever and whatever/, Shlink will always match the former.

These are quite edge cases though, so I think its ok.

@eleith
Copy link
Author

eleith commented Sep 5, 2022

lovely!

i'm glad that as shlink has evolved with the support of multi-segment slugs, handling of trailing forward slash may be facilitated.

enabling as an opt-in config feels like a good match, since that is the approach used for multi-segment as well.

looking forward to it!

@acelaya acelaya moved this from In Progress 📝 to In review 👀 in Shlink Sep 5, 2022
Repository owner moved this from In review 👀 to Done ✅ in Shlink Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants