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

Don't raise an error if URL contains a colon #5889

Merged
merged 2 commits into from Mar 31, 2017

Conversation

Projects
None yet
4 participants
@DirtyF
Member

DirtyF commented Feb 17, 2017

fix #5284

@DirtyF DirtyF requested a review from parkr Feb 17, 2017

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Feb 17, 2017

Member

I think it's worth taking a look at where this code came from.

In jekyll/jekyll-paginate#8 Jekyll was choking when trying to create a URL from a template that included invalid keys. :path/page:num was not working because :path is not a valid permalink key.

In #2834 (comment) Jekyll dies while trying to write a file that includes a colon in the filename.

#5069 was added to prevent these issues.

workflow_2x

If we get rid of this check, we should find some other way to avoid dying catastrophically if a user tries to specify an invalid permalink.

Perhaps the best solution is that, in edge cases where a colon in the permalink is actually desired, the colon be escaped in the permalink #5284 (comment)

Member

pathawks commented Feb 17, 2017

I think it's worth taking a look at where this code came from.

In jekyll/jekyll-paginate#8 Jekyll was choking when trying to create a URL from a template that included invalid keys. :path/page:num was not working because :path is not a valid permalink key.

In #2834 (comment) Jekyll dies while trying to write a file that includes a colon in the filename.

#5069 was added to prevent these issues.

workflow_2x

If we get rid of this check, we should find some other way to avoid dying catastrophically if a user tries to specify an invalid permalink.

Perhaps the best solution is that, in edge cases where a colon in the permalink is actually desired, the colon be escaped in the permalink #5284 (comment)

Thanks for adding InvalidURLError back.

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Feb 17, 2017

Member

@pathawks thanks fro summing things up, I just followed what @parkr said in #5284 (comment)

Member

DirtyF commented Feb 17, 2017

@pathawks thanks fro summing things up, I just followed what @parkr said in #5284 (comment)

:template => "/:x/:y/:z",
:placeholders => { :x => "foo", :z => "bar" }
)
assert_raises Jekyll::Errors::InvalidURLError do

This comment has been minimized.

@parkr

parkr Feb 17, 2017

Member

What do you and @pathawks think about, if a placeholder doesn't exist, simply removing it?

The real issue we're driving at is: you used an invalid URL placeholder, i.e. :dat instead of :date.

@parkr

parkr Feb 17, 2017

Member

What do you and @pathawks think about, if a placeholder doesn't exist, simply removing it?

The real issue we're driving at is: you used an invalid URL placeholder, i.e. :dat instead of :date.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Feb 18, 2017

Member

I think there are two issues in conflict here:

  1. A user is trying to use an invalid URL placeholder, i.e. :dat instead of :date
  2. A user is trying to use a permalink that includes a colon, i.e. wiki/Category:Cookbook

In scenario 1, the user wants to know that something has gone wrong so that the problem can be fixed.
In scenario 2, the user wants the page to be written in such a way that the permalink does include the colon.

Failing the build serves scenario 1, but breaks scenario 2.
Silently dropping invalid URL placeholders breaks scenario 2, and may cause unexpected results in scenario 1.
Simply building URLs that contain colons serves scenario 2, but will certainly cause unexpected results in scenario 1.

I believe that scenario 1 is far more common than scenario 2.

Apparently, with the way things are currently, scenario 2 can still be achieved by escaping colons in URLs

If we are going to go out of our way to allow URLs to contain colons, we also need to figure out how to write those files to disk in a way that works across the different operating systems we support.

Member

pathawks commented Feb 18, 2017

I think there are two issues in conflict here:

  1. A user is trying to use an invalid URL placeholder, i.e. :dat instead of :date
  2. A user is trying to use a permalink that includes a colon, i.e. wiki/Category:Cookbook

In scenario 1, the user wants to know that something has gone wrong so that the problem can be fixed.
In scenario 2, the user wants the page to be written in such a way that the permalink does include the colon.

Failing the build serves scenario 1, but breaks scenario 2.
Silently dropping invalid URL placeholders breaks scenario 2, and may cause unexpected results in scenario 1.
Simply building URLs that contain colons serves scenario 2, but will certainly cause unexpected results in scenario 1.

I believe that scenario 1 is far more common than scenario 2.

Apparently, with the way things are currently, scenario 2 can still be achieved by escaping colons in URLs

If we are going to go out of our way to allow URLs to contain colons, we also need to figure out how to write those files to disk in a way that works across the different operating systems we support.

@parkr

parkr approved these changes Mar 31, 2017

Let's do this for now and revisit catching bad templates later.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 31, 2017

Member

@jekyllbot: merge +minor

Member

parkr commented Mar 31, 2017

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit a4c4388 into jekyll:master Mar 31, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jekyllbot added a commit that referenced this pull request Mar 31, 2017

@DirtyF DirtyF deleted the DirtyF:pull/permalinks-colon branch Mar 31, 2017

@peterjc peterjc referenced this pull request Apr 5, 2017

Closed

Can no longer use colons in permalinks #5284

7 of 17 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment