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

filter relative_url should keep absolute urls with scheme/authority #6490

Merged
merged 4 commits into from Nov 5, 2017

Conversation

Projects
None yet
5 participants
@straight-shoota
Contributor

straight-shoota commented Oct 27, 2017

Currently, relative_url converts all uris to an absolute path even absolute uris with authority.
This can lead to unexpected results. For example {{ "https://example.com/" | relative_url }} becomes /https://example.com/ (with baseurl = "/").

This is an issue when you want to define for example a navigation structure with internal and external links.

{% for link in nav %}
  <a href="{{ link.url | relative_url }}">{{ link.label }}</a>
{% endfor %}

The relative_url converts all links to absolute paths, even external links. Leaving it out would mean that internal links don't get the baseurl prepended.

This PR adds a simple check if the input is an absolute url with authority and doesn't modify it (except normalization).

As a minor issue, https://example.com/ could also be a valid relative path and /https://example.com/ the expected result. But this is not typically desired and can easily be mitigated by escaping this path as https%3A//example.com/ to avoid misinterpration.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Oct 27, 2017

Member

/https://example.com/ the expected result.

When is that the expected result?

Member

ashmaroli commented Oct 27, 2017

/https://example.com/ the expected result.

When is that the expected result?

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Oct 27, 2017

Member

I’m in the fence about this.

On the one hand, it makes sense for the filter to have a pass through mechanism rather than returning something nonsensical.

On the other hand, even with these changes the filter returns garbage if called twice:

{{ page.url | relative_url | relative_url }}

There’s no way to prevent this from returning garbage.

What is the use case here? Shouldn’t the site know if a URL will be absolute already?

Member

pathawks commented Oct 27, 2017

I’m in the fence about this.

On the one hand, it makes sense for the filter to have a pass through mechanism rather than returning something nonsensical.

On the other hand, even with these changes the filter returns garbage if called twice:

{{ page.url | relative_url | relative_url }}

There’s no way to prevent this from returning garbage.

What is the use case here? Shouldn’t the site know if a URL will be absolute already?

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Oct 27, 2017

Member

@ashmaroli Stating that /http://example.com is a perfectly valid URL and could be the expected result.

Member

pathawks commented Oct 27, 2017

@ashmaroli Stating that /http://example.com is a perfectly valid URL and could be the expected result.

@straight-shoota

This comment has been minimized.

Show comment
Hide comment
@straight-shoota

straight-shoota Oct 27, 2017

Contributor

@pathawks Yes, calling it twice makes rubbish, but that's also the status quo. That shouldn't matter too much I think, because this filter is typically used directly in a print expression, so no further modifications.

A usecase for this feature is a navigation list including internal and external URLs. The internal ones need to be prefixed with baseurl the external should be unmodified.

nav:
- label: home
   url: /home
- label: jekyll
   url: https://jekyllrb.com
{% for link in site.nav %}
  <a href="{{ link.url | relative_url }}">{{ link.label }}</a>
{% endfor %}
Contributor

straight-shoota commented Oct 27, 2017

@pathawks Yes, calling it twice makes rubbish, but that's also the status quo. That shouldn't matter too much I think, because this filter is typically used directly in a print expression, so no further modifications.

A usecase for this feature is a navigation list including internal and external URLs. The internal ones need to be prefixed with baseurl the external should be unmodified.

nav:
- label: home
   url: /home
- label: jekyll
   url: https://jekyllrb.com
{% for link in site.nav %}
  <a href="{{ link.url | relative_url }}">{{ link.label }}</a>
{% endfor %}
@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Oct 27, 2017

Member

Ok, I’m on board conceptually.

Member

pathawks commented Oct 27, 2017

Ok, I’m on board conceptually.

Show outdated Hide outdated lib/jekyll/filters/url_filters.rb Outdated
@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Oct 27, 2017

Member

But protocol-relative URIs are also valid and even with https-only becoming more and more standard they might still prove useful in some situations.

Protocol-relative URLs are an antipattern we do not support. The two URL filters should behave consistently.

Member

pathawks commented Oct 27, 2017

But protocol-relative URIs are also valid and even with https-only becoming more and more standard they might still prove useful in some situations.

Protocol-relative URLs are an antipattern we do not support. The two URL filters should behave consistently.

@straight-shoota

This comment has been minimized.

Show comment
Hide comment
@straight-shoota

straight-shoota Oct 27, 2017

Contributor

@pathawks What do you think about normalizing absolute URIs? In both methods.

Contributor

straight-shoota commented Oct 27, 2017

@pathawks What do you think about normalizing absolute URIs? In both methods.

@@ -20,13 +20,17 @@ def absolute_url(input)
).normalize.to_s
end
# Produces a URL relative to the domain root based on site.baseurl.
# Produces a URL relative to the domain root based on site.baseurl
# unless it is already an absolute url with an authority (host).

This comment has been minimized.

@ashmaroli

ashmaroli Oct 28, 2017

Member

small correction here. authority != host

def uri(input = "http://localhost:4000")
  Addressable::URI.parse(input.to_s)
end

print uri.absolute? # => true
print uri.host      # => "localhost"
print uri.authority # => "localhost:4000"
@ashmaroli

ashmaroli Oct 28, 2017

Member

small correction here. authority != host

def uri(input = "http://localhost:4000")
  Addressable::URI.parse(input.to_s)
end

print uri.absolute? # => true
print uri.host      # => "localhost"
print uri.authority # => "localhost:4000"

This comment has been minimized.

@straight-shoota

straight-shoota Oct 28, 2017

Contributor

It's not supposed to mean authority == host but host is the integral component of an authority. No host, no authority. It may also have userinfo and port, but they're not key.

@straight-shoota

straight-shoota Oct 28, 2017

Contributor

It's not supposed to mean authority == host but host is the integral component of an authority. No host, no authority. It may also have userinfo and port, but they're not key.

This comment has been minimized.

@ashmaroli

ashmaroli Oct 28, 2017

Member

okay then..

@ashmaroli

ashmaroli Oct 28, 2017

Member

okay then..

DirtyF and others added some commits Oct 30, 2017

@straight-shoota

This comment has been minimized.

Show comment
Hide comment
@straight-shoota

straight-shoota Nov 4, 2017

Contributor

Does this need any more work?

Contributor

straight-shoota commented Nov 4, 2017

Does this need any more work?

@pathawks

Love it! Thanks for all the work addressing all of the feedback 👍🏼

@DirtyF DirtyF added this to the v3.7.0 milestone Nov 5, 2017

@DirtyF

DirtyF approved these changes Nov 5, 2017

Great first-contrib, thank you! 😍

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 5, 2017

Member

@jekyllbot: merge +minor

Member

pathawks commented Nov 5, 2017

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit a66c478 into jekyll:master Nov 5, 2017

1 of 2 checks passed

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

@straight-shoota straight-shoota deleted the straight-shoota:jm-relative_url branch Nov 5, 2017

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