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

Add absolute_url and relative_url filters. #5399

Merged
merged 3 commits into from Sep 23, 2016

Conversation

Projects
None yet
4 participants
@parkr
Member

parkr commented Sep 22, 2016

When creating sites and themes, there's a lot of confusion around site.url and site.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 with site.baseurl properly prepended. This is useful when you want to be host-agnostic.
  • absolute_url - takes a URL and returns a url with site.baseurl and site.url properly appended.

Examples, assuming site.baseurl = "/project" and site.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.baseurl or site.url values 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

@parkr parkr added the feature label Sep 22, 2016

@parkr parkr added this to the 3.3 milestone Sep 22, 2016

@parkr parkr assigned pathawks and unassigned parkr 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

This comment has been minimized.

@benbalter

benbalter Sep 22, 2016

Contributor

Should this be URI.join here?

@benbalter

benbalter Sep 22, 2016

Contributor

Should this be URI.join here?

This comment has been minimized.

@parkr

parkr Sep 22, 2016

Member

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'
@parkr

parkr Sep 22, 2016

Member

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'
Show outdated Hide outdated lib/jekyll/filters/url_filters.rb
private
def ensure_leading_slash(input)
return input if input.nil? || input.empty?
if input.start_with?("/")

This comment has been minimized.

@benbalter

benbalter Sep 22, 2016

Contributor

Could you move this to the line above to make this a two line method?

@benbalter

benbalter Sep 22, 2016

Contributor

Could you move this to the line above to make this a two line method?

@parkr parkr referenced this pull request Sep 22, 2016

Closed

Jekyll 3.3 Release Gameplan #5400

9 of 9 tasks complete
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 22, 2016

Member

@benbalter Would appreciate another review when you have a minute.

Member

parkr commented Sep 22, 2016

@benbalter Would appreciate another review when you have a minute.

@pathawks

I love the idea of breaking out filters by categories instead of just putting everything in filters.rb.

Just one question about relative_url

@@ -3,8 +3,11 @@
require "date"
require "liquid"
require_all "jekyll/filters"

This comment has been minimized.

@pathawks

pathawks Sep 23, 2016

Member

❤️

@pathawks

pathawks Sep 23, 2016

Member

❤️

Show outdated Hide outdated lib/jekyll/filters/url_filters.rb
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)

This comment has been minimized.

@pathawks

pathawks Sep 23, 2016

Member

Why not

URI(
  ensure_leading_slash(site.config["baseurl"]) + ensure_leading_slash(input.to_s)
).to_s

This seems more clear to me, but I may be missing something.

@pathawks

pathawks Sep 23, 2016

Member

Why not

URI(
  ensure_leading_slash(site.config["baseurl"]) + ensure_leading_slash(input.to_s)
).to_s

This seems more clear to me, but I may be missing something.

This comment has been minimized.

@parkr

parkr Sep 23, 2016

Member

Updated in fa96843.

@parkr

parkr Sep 23, 2016

Member

Updated in fa96843.

This comment has been minimized.

@pathawks

pathawks Sep 23, 2016

Member

❤️

@pathawks

pathawks Sep 23, 2016

Member

❤️

@mmistakes mmistakes referenced this pull request Sep 23, 2016

Closed

Replace base_path include with absolute_url filter #553

1 of 1 task complete
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Sep 23, 2016

Member

@jekyllbot: merge +minor

Member

parkr commented Sep 23, 2016

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 8197e17 into master Sep 23, 2016

1 of 2 checks passed

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

@jekyllbot jekyllbot deleted the relative_url_and_absolute_url branch Sep 23, 2016

jekyllbot added a commit that referenced this pull request Sep 23, 2016

@parkr parkr restored the relative_url_and_absolute_url branch Sep 23, 2016

@pathawks pathawks referenced this pull request Oct 25, 2016

Closed

Filter to get absolute URL #5070

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