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

removed cgi unescape in page destination #1215

Merged
merged 2 commits into from Jun 25, 2013

Conversation

Projects
None yet
5 participants
@sanandnarayan
Contributor

sanandnarayan commented Jun 15, 2013

Address issue #1211

@parkr

This comment has been minimized.

Member

parkr commented Jun 15, 2013

The comment was added here: a428ace

@sanandnarayan

This comment has been minimized.

Contributor

sanandnarayan commented Jun 15, 2013

@parkr the comment was last modified on that commit... tried checking into the history, not able to find a failing spec for that line.

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Jun 15, 2013

I think it came from ee0167d

@sanandnarayan

This comment has been minimized.

Contributor

sanandnarayan commented Jun 15, 2013

okay, how should i proceed on this?

@parkr

This comment has been minimized.

Member

parkr commented Jun 15, 2013

I think we should write a test for a page which has a permalink which is non-ASCII and to expect the escaped output. If we don't have a test already, then it wasn't important enough to keep, I'd say.

@sanandnarayan

This comment has been minimized.

Contributor

sanandnarayan commented Jun 15, 2013

@parkr why would we write a test that deals with encoding of content.. This issue is more to do with the path/filename than its content, or am i missing something?

@parkr

This comment has been minimized.

Member

parkr commented Jun 15, 2013

@sanandnarayan I didn't say non-ASCII content - I said a non-ASCII permalink. That's what this code affects :)

@sanandnarayan

This comment has been minimized.

Contributor

sanandnarayan commented Jun 15, 2013

@parkr oops, got it... 👍

@sanandnarayan

This comment has been minimized.

Contributor

sanandnarayan commented Jun 15, 2013

hmm, i feel like i have got the correct spec to remove that line...

permalink: plus+in+url

if passed to cgi escape will be plus in url and that cant be a filename ...

P.S: if i added a non ascii character in the permalink, then liquid throws up a Invalide Byte Sequence

ArgumentError: invalid byte sequence in US-ASCII
    /Users/abinand/.rvm/gems/ruby-1.9.3-p194/gems/liquid-2.5.0/lib/liquid/template.rb:141:in `split'
    /Users/abinand/.rvm/gems/ruby-1.9.3-p194/gems/liquid-2.5.0/lib/liquid/template.rb:141:in `tokenize'
@parkr

This comment has been minimized.

Member

parkr commented Jun 22, 2013

Ok, this should be OK. We really should allow non-ASCII paths, but it seems like a very difficult issue to solve, particularly as it seems that Liquid doesn't really support non-ASCII characters.

@parkr

This comment has been minimized.

Member

parkr commented Jun 25, 2013

@mattr- this LGTM. What do you think?

mattr- added a commit that referenced this pull request Jun 25, 2013

Merge pull request #1215 from sanandnarayan/folder_plus
removed cgi unescape in page destination

@mattr- mattr- merged commit c1850a3 into jekyll:master Jun 25, 2013

1 check passed

default The Travis CI build passed
Details

mattr- added a commit that referenced this pull request Jun 25, 2013

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.