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

Permanent redirects #5104

Closed
marcelstoer opened this issue Jan 13, 2019 · 9 comments · Fixed by #5727
Closed

Permanent redirects #5104

marcelstoer opened this issue Jan 13, 2019 · 9 comments · Fixed by #5727
Labels
Needed: design decision A core team decision is required

Comments

@marcelstoer
Copy link
Contributor

Details

Expected Result

The exact redirect configured as /en/latest/en/$rest -> /en/latest/ should redirect from https://nodemcu.readthedocs.io/en/latest/en/sdcard/ to https://nodemcu.readthedocs.io/en/latest/sdcard/ using HTTP 301 or 308.

Actual Result

HTTP 302, temporary redirect (ignore the discussion whether to use HTTP 302 or 303 or 307 for temporary redirects for now). I would assume that users rewriting URLs in the RTD context mean to do that with "permanent redirect" in mind.

@humitos
Copy link
Member

humitos commented Jan 14, 2019

I suppose that this is a 302 on purpose, because otherwise we will exposing a redirect that could be dangerous for the user if they make a mistake.

I would not change this and expose the users to this problem.

What's your current problem with the 302 that makes you need a 301?

@humitos humitos added the Needed: more information A reply from issue author is required label Jan 14, 2019
@marcelstoer
Copy link
Contributor Author

No real problem but I argue that it is semantically incorrect.

  • My redirect rule is not temporary as the HTTP status suggests but permanent. I restructured my documentation (which made the rule necessary in the first place) and I don't intend to ever revert that. Should I need to change the structure again in some way a new, different rule would be required.
  • Because of the above user agents will for ever have to make two requests. It's unnecessary to delay the delivery of the documentation to the user and put extra load on your infrastructure.

@no-response no-response bot removed the Needed: more information A reply from issue author is required label Jan 14, 2019
@stsewd stsewd added the Needed: design decision A core team decision is required label Jan 14, 2019
@marcelstoer
Copy link
Contributor Author

Ah, it's obvious but missing on the above list...

If you send temporary redirects to search engine crawlers they won't update their index. Thus, they will forever keep the old URLs (-> more load on your infrastructure).

@davidfischer
Copy link
Contributor

I think a 302 is the best choice. If you make a mistake with a 301 redirect, there isn't a good way to reverse that change. Chrome and Firefox cache 301s until the user manually clears their cache so a mistake is essentially permanent for users who don't manually clear their cache. I don't like to present users with choices where they can make nearly permanent mistakes.

@marcelstoer
Copy link
Contributor Author

I appreciate your feedback and I understand you have a slightly different perspective. If you allowed us to edit a redirect rule (currently I can only add or remove) then you could simply add a "permanent yes/no" checkbox. That way we could tune our rules until everything works as expected - a which point we could enable the flag.

@davidfischer
Copy link
Contributor

That's fair. I would consider it as an option but the default should be 302.

@humitos
Copy link
Member

humitos commented May 23, 2019

We do have a Redirect.http_status field that its default is 301, although we are not using it anywhere.

I think we could accept a PR that:

  • create a migration to update the default to 302
  • use this value when redirecting
  • tests for the different cases

Then, we can handle special cases like this one manually (instead of building a whole UX/UI) initially.

@saadmk11
Copy link
Member

@humitos I'm a bit confused about the second point. As one Project might have many Redirect objects which one should we use for redirecting.

@humitos
Copy link
Member

humitos commented May 23, 2019

@saadmk11 each Redirect object has a http_status value on its own. So, when one instance of a redirect that match the url, we access that field and return that status code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needed: design decision A core team decision is required
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants