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

🐛 Fixed redirects with special characters #15533

Merged
merged 2 commits into from
Oct 13, 2022
Merged

Conversation

Prathamesh010
Copy link
Contributor

refs/closes #15267
Custom redirect URLs with special characters showing 404 instead of redirection
This was because the URLs were not being encoded before adding to the router.

  • There's a clear use-case for this code change, explained below
  • Commit message has a short title & references relevant issues
  • The build will pass (run yarn test:all and yarn lint)

The issue is solved by encoding the URL before its added to the router.

301:
    /joloonii-surgaltuud/b-angilal/: /joloonii-angilal/#в-ангилал
    /joloonii-surgaltuud/а-ангилал/: /joloonii-angilal/#а-ангилал

Here, the first one resolves perfectly as it does not have any special characters but the second one fails to redirect.

I tried adding the encoded version of /joloonii-surgaltuud/а-ангилал/ ie.
/joloonii-surgaltuud/%D0%B0-%D0%B0%D0%BD%D0%B3%D0%B8%D0%BB%D0%B0%D0%BB into the yaml and it resolved perfectly.

So the issue was in encoding as /joloonii-surgaltuud/а-ангилал/ never existed on the router but the encoded one did.

@Prathamesh010 Prathamesh010 changed the title 🐛 Encoded uri for custom redirect bfore adding to router 🐛 Encoded uri for custom redirect before adding to router Oct 5, 2022
Copy link
Member

@ErisDS ErisDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Prathamesh010, thanks for your PR!

I believe that this should be handled inside the redirect manager itself?

I think that will also make it easier to add unit tests that demonstrate the change is correct, which we would need in order to safely merge this please.

refs/closes #
custom redirect urls with special characters showing 404 instead of redirection
this was because of the urls not being encoded before adding to the router
@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

Base: 52.32% // Head: 52.32% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (879cb03) compared to base (0bc31de).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15533   +/-   ##
=======================================
  Coverage   52.32%   52.32%           
=======================================
  Files        1446     1446           
  Lines       93505    93512    +7     
  Branches    10439    10439           
=======================================
+ Hits        48928    48933    +5     
  Misses      43350    43350           
- Partials     1227     1229    +2     
Impacted Files Coverage Δ
...ss-dynamic-redirects/lib/DynamicRedirectManager.js 98.17% <100.00%> (-1.83%) ⬇️
ghost/admin/app/components/gh-file-uploader.js 82.94% <0.00%> (-3.11%) ⬇️
ghost/admin/app/components/gh-dropdown.js 89.79% <0.00%> (-2.05%) ⬇️
ghost/admin/app/controllers/offer.js 42.10% <0.00%> (+0.52%) ⬆️
ghost/admin/app/components/gh-site-iframe.js 45.45% <0.00%> (+2.27%) ⬆️
ghost/admin/app/components/gh-image-uploader.js 87.07% <0.00%> (+2.72%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ErisDS ErisDS changed the title 🐛 Encoded uri for custom redirect before adding to router 🐛 Fixed redirects with special characters Oct 13, 2022
@ErisDS ErisDS merged commit 3424222 into TryGhost:main Oct 13, 2022
@ErisDS
Copy link
Member

ErisDS commented Oct 13, 2022

Hi @Prathamesh010 thanks so much for taking the time to create and update this PR. The updated solution looks great 👌

This has now been merged 🎉 and will appear in the next release of Ghost - usually Fridays.

I'm not sure if you found this through hacktoberfest, but I've added the accepted label to this PR to make sure it counts.

We have plenty of open issues and ongoing projects for hacktoberfest and beyond, including a project around i18n and there was also a recent discussion about adding support for special characters in more key parts of Ghost that I can reopen if it's interesting to you as we're looking for experts to take on this work and help us make sure it doesn't break anything 🙂

sam-lord pushed a commit that referenced this pull request Oct 17, 2022
closes: #15267

- This was because the URLs were not being encoded and matched correctly - it is solved by encoding the URL before adding to the router.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Links with cyrillic characters are not being redirected
2 participants