jekyll / jekyll-redirect-from

:twisted_rightwards_arrows: Seamlessly specify multiple redirections URLs for your pages and posts.
MIT License
782 stars 114 forks source link

Preserve search and hash in redirect #127

Closed FokkeZB closed 7 years ago

FokkeZB commented 7 years ago

Closes #123

FokkeZB commented 7 years ago

One job still fails but that seems to be a different issue: https://travis-ci.org/jekyll/jekyll-redirect-from/jobs/176961793

pathawks commented 7 years ago

What if the redirect URL already has a query?

I don't understand the motivation behind trying to modify <meta> tags with JS.

FokkeZB commented 7 years ago

@pathawks we're talking a static generator right? I can't think of a situation where #{item_url} would have a query?

I agree modifying the refresh meta tag has not a ton of value, neither has modifying the link. If you have JavaScript, the location.replace() will do the job anyway. So I can remove that part.

But.. it do think it would should let the meta refresh at 1s to make sure JavaScript can handle te redirect before the browser does.

pathawks commented 7 years ago

we're talking a static generator right?

Either it is important or it isn't.

A page could have been integrated into a section of another page. Perhaps the URL contains a hash of the section in the new page that contains the old content.

I feel like this greatly increases the complexity of the template, and I worry that there are some edge cases where problems could creep up.

FokkeZB commented 7 years ago

What I meant is that the afaik #{item_url}always refers to the URL of a generated page as Jekyll constructs it and I don't think these ever come with a search or hash?

Sure, the it could be that the search and hash no longer serve a purpose on the new page, but in that case it wouldn't hearth anything either. It just wouldn't do anything.

In the meanwhile, if would still do anything... without this fix it doesn't let you.

pathawks commented 7 years ago

What I mean is, if I had a page location.html and I remove it, and redirect it to about.html#location

With these new changes, if somebody lands on location.html?q=p#hash, it looks like they will be redirected to about.html#location?q=p#hash, which does not seem appropriate.

FokkeZB commented 7 years ago

Ah, you mean when using redirect_to? Although afaik redirect_from is preferred when redirecting within the site and redirect_to is mostly used to redirect to external websites - I guess indeed you could use it internally as well and then potentially run into this issue.

I will make a change to accommodate.

FokkeZB commented 7 years ago

All checks green, except still this odd one: https://travis-ci.org/jekyll/jekyll-redirect-from/jobs/177239657

pathawks commented 7 years ago

Not your fault. Looks like it's time for us to stop testing against Ruby 1.9

FokkeZB commented 7 years ago

@pathawks did you have a chance to have another look at this? @jekyllbot just staled #123 so I'd love to see this (updated and) merged.

haniawni commented 7 years ago

Hi! Random First-Time User here. Ran into the exact problem by @pathawks here where redirect-to with Hashes wont connect.

Example is visible here:

Is there an existing workaround?

mpchadwick commented 7 years ago

Not sure what happened here, but I think the ability to preserve the hash and query string when redirecting is pretty important.

For example, I have a page at /projects which I'm thinking about changing to /work. I can link directly to a certain section of /projects with a hash /projects/#open-source-contributions. If I have backlinks directly to that section, I don't want them to lose the hash when redirecting.

That being said, I understand the concerns outline here.

Perhaps there could be an additional flag in the frontmatter to preserve these?

redirect_from:
   - /projects/
redirect_preserve:
   - hash
   - query
pathawks commented 7 years ago

No, adding options would increase complexity, not reduce it.

Any solution would require JavaScript which is far more complex than our existing code. I’m not convinced it’s worth the trade.

mpchadwick commented 7 years ago

@pathawks i'm not arguing that it makes this less simple. all I'm saying is that it's a useful feature and something i would expect to be able to do with a tool that facilitates redirects. as you can see from the issues on the repo i'm not the only one who thinks this.

using a flag to enable the feature would prevent some of the potential drawbacks and edge-cases you've outlined as it would require users to specifically opt-in.

pathawks commented 7 years ago

Thank you for the Pull Request. Code looks solid, but I don't think this feature is worth the complexity it adds to the plugin.