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

absolute_url should not mangle URL if called more than once #5789

Merged
merged 2 commits into from Apr 5, 2017

Conversation

Projects
None yet
4 participants
@jekyllbot
Contributor

jekyllbot commented Jan 17, 2017

PR automatically created for @pathawks.

Failing test: absolute_url with absolute URL

Fixes #5788

@pathawks pathawks self-assigned this Jan 17, 2017

@pathawks pathawks changed the title from Failing test: absolute_url with absolute URL to absolute_url should not mangle URL if called more than once Jan 17, 2017

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 17, 2017

Member

Thinking about this a bit more, perhaps we should issue a debug/warning message instead of / in addition to passing input through unchanged.

Member

pathawks commented Jan 17, 2017

Thinking about this a bit more, perhaps we should issue a debug/warning message instead of / in addition to passing input through unchanged.

@parkr

parkr approved these changes Jan 17, 2017

@parkr parkr added this to the 3.4 milestone Jan 17, 2017

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 17, 2017

Member

Thinking about this a bit more, perhaps we should issue a debug/warning message instead of / in addition to passing input through unchanged.

How come?

Member

parkr commented Jan 17, 2017

Thinking about this a bit more, perhaps we should issue a debug/warning message instead of / in addition to passing input through unchanged.

How come?

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 18, 2017

Member

Why issue a debug message?

I came across this when I was working on a plugin. I assigned an absolute_url to a variable, and then I filtered that variable through absolute_url when I actually output it. These lines were not very close in proximity, so I only noticed my error when the test failed.

I certainly wouldn't want to ship code like this, and I'm sure any plugin or theme author worth her salt would agree. So, it would be nice to get a message that this is happening.

Why allow the URL to pass through?

2.3.0 :001 > require "Addressable"
 => true 
2.3.0 :002 > Addressable::URI.parse("http://example.com/http://example.com").to_s
 => "http://example.com/http://example.com"

Addressable seems to be just fine with http://example.com/http://example.com as a URI, and I don't want to end up on one of those Falsehoods Programmers Believe About… lists.

See also: #5284 #5480

Member

pathawks commented Jan 18, 2017

Why issue a debug message?

I came across this when I was working on a plugin. I assigned an absolute_url to a variable, and then I filtered that variable through absolute_url when I actually output it. These lines were not very close in proximity, so I only noticed my error when the test failed.

I certainly wouldn't want to ship code like this, and I'm sure any plugin or theme author worth her salt would agree. So, it would be nice to get a message that this is happening.

Why allow the URL to pass through?

2.3.0 :001 > require "Addressable"
 => true 
2.3.0 :002 > Addressable::URI.parse("http://example.com/http://example.com").to_s
 => "http://example.com/http://example.com"

Addressable seems to be just fine with http://example.com/http://example.com as a URI, and I don't want to end up on one of those Falsehoods Programmers Believe About… lists.

See also: #5284 #5480

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jan 26, 2017

Member

@pathawks OK, how would you like to proceed?

Member

parkr commented Jan 26, 2017

@pathawks OK, how would you like to proceed?

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Jan 27, 2017

Member

I think it should just output a warning, but I guess it should go head and build the URL as requested.

Am I doing this right? I'm not sure how to write a test to make sure it outputs a warning.

Member

pathawks commented Jan 27, 2017

I think it should just output a warning, but I guess it should go head and build the URL as requested.

Am I doing this right? I'm not sure how to write a test to make sure it outputs a warning.

@parkr parkr modified the milestones: 3.4, 3.5 Jan 27, 2017

Show outdated Hide outdated lib/jekyll/filters/url_filters.rb
Jekyll.logger.warn(
"absolute_url:",
%("#{input}" appears to already be absolute)
)

This comment has been minimized.

@parkr

parkr Feb 11, 2017

Member

From a usability standpoint, the first thing I think about is: how is the user going to have any idea where in their site to look? If you don't think it should be automatically dealt with, then we should raise or something so we can get some context.

@parkr

parkr Feb 11, 2017

Member

From a usability standpoint, the first thing I think about is: how is the user going to have any idea where in their site to look? If you don't think it should be automatically dealt with, then we should raise or something so we can get some context.

@pathawks pathawks assigned parkr and unassigned pathawks Feb 11, 2017

@parkr

@pathawks Instead of warning, let's just do the needful and return the URL if it's already absolute. Warning here without a line number or filename is useless to the user.

@parkr parkr assigned pathawks and unassigned parkr Feb 16, 2017

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 31, 2017

Member

@pathawks You cool with my suggestion above?

Member

parkr commented Mar 31, 2017

@pathawks You cool with my suggestion above?

pathawks added some commits Jan 17, 2017

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Apr 4, 2017

Member

@parkr Yessir, how does it look now?

Member

pathawks commented Apr 4, 2017

@parkr Yessir, how does it look now?

@parkr

parkr approved these changes Apr 5, 2017

💥

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 5, 2017

Member

@jekyllbot: merge +bug

Member

parkr commented Apr 5, 2017

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 768d2bb into master Apr 5, 2017

2 checks passed

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

@jekyllbot jekyllbot added bug fix labels Apr 5, 2017

@jekyllbot jekyllbot deleted the pull/absolute_url_once branch Apr 5, 2017

jekyllbot added a commit that referenced this pull request Apr 5, 2017

DirtyF added a commit to DirtyF/jekyll that referenced this pull request Apr 7, 2017

DirtyF added a commit to DirtyF/jekyll that referenced this pull request Apr 7, 2017

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