Add absolute_url and relative_url filters. #5399
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benbalter
reviewed
Sep 22, 2016
| return if input.nil? | ||
| site = @context.registers[:site] | ||
| return relative_url(input).to_s if site.config["url"].nil? | ||
| URI(site.config["url"] + relative_url(input)).to_s |
There was a problem hiding this comment.
Should this be URI.join here?
There was a problem hiding this comment.
URI.join causes more problems than it solves. relative_url will always start with a / so this is safe. An example of what it breaks is if site.url = "example.com":
Error:
TestFilters#test_: filters absolute_url filter should be ok with a blank but present 'url'. :
URI::BadURIError: both URI are relative
/Users/parkr/.rbenv/versions/2.3.1/lib/ruby/2.3.0/uri/generic.rb:1102:in `rescue in merge'
/Users/parkr/.rbenv/versions/2.3.1/lib/ruby/2.3.0/uri/generic.rb:1099:in `merge'
/Users/parkr/.rbenv/versions/2.3.1/lib/ruby/2.3.0/uri/rfc3986_parser.rb:89:in `each'
/Users/parkr/.rbenv/versions/2.3.1/lib/ruby/2.3.0/uri/rfc3986_parser.rb:89:in `inject'
/Users/parkr/.rbenv/versions/2.3.1/lib/ruby/2.3.0/uri/rfc3986_parser.rb:89:in `join'
/Users/parkr/.rbenv/versions/2.3.1/lib/ruby/2.3.0/uri/common.rb:265:in `join'
/Users/parkr/jekyll/jekyll/lib/jekyll/filters/url_filters.rb:13:in `absolute_url'
/Users/parkr/jekyll/jekyll/test/test_filters.rb:341:in `block (3 levels) in <class:TestFilters>'
/Users/parkr/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/shoulda-context-1.2.1/lib/shoulda/context/context.rb:413:in `instance_exec'
/Users/parkr/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/shoulda-context-1.2.1/lib/shoulda/context/context.rb:413:in `block in create_test_from_should_hash'
benbalter
reviewed
Sep 22, 2016
| private | ||
| def ensure_leading_slash(input) | ||
| return input if input.nil? || input.empty? | ||
| if input.start_with?("/") |
There was a problem hiding this comment.
Could you move this to the line above to make this a two line method?
|
@benbalter Would appreciate another review when you have a minute. |
pathawks
reviewed
Sep 23, 2016
| @@ -3,8 +3,11 @@ | |||
| require "date" | |||
| require "liquid" | |||
|
|
|||
| require_all "jekyll/filters" | |||
| return ensure_leading_slash(input.to_s) if site.config["baseurl"].nil? | ||
| ensure_leading_slash( # in case the baseurl doesn't have a leading slash | ||
| URI( | ||
| site.config["baseurl"] + ensure_leading_slash(input.to_s) |
There was a problem hiding this comment.
Why not
URI(
ensure_leading_slash(site.config["baseurl"]) + ensure_leading_slash(input.to_s)
).to_sThis seems more clear to me, but I may be missing something.
benbalter
approved these changes
Sep 23, 2016
pathawks
approved these changes
Sep 23, 2016
1 task
|
@jekyllbot: merge +minor |
jekyllbot
added a commit
that referenced
this pull request
Sep 23, 2016
This was referenced Sep 24, 2016
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When creating sites and themes, there's a lot of confusion around
site.urlandsite.baseurl. These new filters attempt to unify the way we calculate relative and absolute URL's. The new filters are:relative_url- takes a URL and returns a url withsite.baseurlproperly prepended. This is useful when you want to be host-agnostic.absolute_url- takes a URL and returns a url withsite.baseurlandsite.urlproperly appended.Examples, assuming
site.baseurl = "/project"andsite.url = "http://example.com":{{ "/css/main.scss" | relative_url }} <!-- "/project/css/main.scss" --> {{ "/css/main.scss" | absolute_url }} <!-- "http://example.com/project/css/main.scss" -->If your
site.baseurlorsite.urlvalues aren't set, it will just append what it can dependeing on the desired output of the filter. Read the tests in the diff for more on this./cc @jekyll/ecosystem @jekyll/core @jekyll/stability