Skip to content
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

Use relative_url to generate feed path link. #317

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fabacab
Copy link

@fabacab fabacab commented Jul 13, 2020

Using the relative URL allows the plugin to be used without modification
when generating sites that may be deployed across more than one domain
name without needing to have the site regenerated for each domain name.

This commit also aligns the code with the code's comments (added in
commit bf728c3) which say that the built-in Jekyll URLFilters are being
used to provide the relative_url functionality, but then the code goes
on to use the absolute_url filter.

Using the relative_url filter does not impact feed reader use (at
least, in my testing), so this should be a backward-compatible change
that improves the versatility of the rendered HTML code.

Using the relative URL allows the plugin to be used without modification
when generating sites that may be deployed across more than one domain
name without needing to have the site regenerated for each domain name.

This commit also aligns the code with the code's comments (added in
commit bf728c3) which say that the built-in Jekyll URLFilters are being
used to provide the `relative_url` functionality, but then the code goes
on to use the `absolute_url` filter.

Using the `relative_url` filter does not impact feed reader use (at
least, in my testing), so this should be a backward-compatible change
that improves the versatility of the rendered HTML code.
@ashmaroli
Copy link
Member

also aligns the code with the code's comments

Perhaps the comment is incorrect. The previous Ruby code clearly takes config["url"] into account. Therefore, absolute_url is the correct choice.

@fabacab
Copy link
Author

fabacab commented Jul 14, 2020

also aligns the code with the code's comments

Perhaps the comment is incorrect. The previous Ruby code clearly takes config["url"] into account. Therefore, absolute_url is the correct choice.

Yeah, that's possibly the original author's intent, but I think what matters more for correctness is whether or not a relative or an absolute URL in this specific context "works better" for the majority of situations jekyll-feed users use it in.

More specifically, is there a situation in which a relative URL does not work but an absolute_url does work? Put another way: assuming a relative URL produces the same behavior that an absolute URL does in all the situations where an absolute URL was intended, then given the fact that a relative URL also works in situations where an absolute URL does not work as I note in the commit message, I think the relative URL is actually the correct choice.

@ashmaroli
Copy link
Member

At first glance, switching to Jekyll's relative_url seems like a safe move to me.
(Prior to the released commit bf728c3, the plugin had also taken config["github"]["url"] into consideration)

That said, /cc @benbalter and @pathawks for further inputs.

@pathawks
Copy link
Member

I see the value here. I wonder if this could be a breaking change: are we worried about sites that were capturing the <head> and replacing the feed link with a FeedBurner/CDN link? Maybe that is not a supported use case?

@torrocus
Copy link
Contributor

Some older browsers have a problem with relative url in RSS/Atom feed.
https://stackoverflow.com/questions/4438794/link-to-rss-atom-feed-relative-doesnt-work-in-firefox
Can anyone confirm or deny this? Is the problem still current?

I also suggest looking at other, similar threads:
gatsbyjs/gatsby#14133
I understand that we do the opposite?

@fabacab
Copy link
Author

fabacab commented Jul 14, 2020

For what it's worth, the W3C's Feed Validator Service correctly follows relative URLs to the location of a feed when you tell it to "Validate by URI" and provide the URL of an HTML page with a LINK element like:

<link type="application/atom+xml" rel="alternate" href="/feed.xml" title="Example Relative Feed" />

This is the expected (and standards-compliant) behavior for HTML4, HTML5, and all XHTML variants as far as I understand. Relative URLs are also explicitly mentioned as allowed on the MDN Wiki. My understanding is that this is also how feed readers that are not Web browsers should auto-discover a site's feed URL when given an HTML home page with such a LINK element in it.

Some older browsers have a problem with relative url in RSS/Atom feed.
https://stackoverflow.com/questions/4438794/link-to-rss-atom-feed-relative-doesnt-work-in-firefox
Can anyone confirm or deny this? Is the problem still current?

Firefox removed its built-in RSS/Atom feed support by 2018 so I'm not sure that Stackoverflow issue is relevant any longer.

@torrocus
Copy link
Contributor

torrocus commented Jul 15, 2020

Firefox removed its built-in RSS/Atom feed support by 2018 so I'm not sure that Stackoverflow issue is relevant any longer.

It's good to know that Firefox no longer supports this.

I have another important question. Should this change not affect tests?
If I understand correctly, different feed.xml files should be generated before and after the change.

@ashmaroli
Copy link
Member

different feed.xml files should be generated before and after the change

Why would the feed be any different? The proposed change involves just the rendered <link ... /> tag..!!

@torrocus
Copy link
Contributor

Ok, in this case, there is probably nothing to worry about.

@DirtyF
Copy link
Member

DirtyF commented Jul 16, 2020

Using the relative URL allows the plugin to be used without modification when generating sites that may be deployed across more than one domain

So it's theoretical and does not solve any real-word case scenario?
Any example to provide where absolute is creating an issue?

@fabacab
Copy link
Author

fabacab commented Jul 16, 2020

So it's theoretical

No, it's not theoretical, if you look at the codebase, you'll see it's really happening.

Any example to provide where absolute is creating an issue?

I currently use a patched version of jekyll-feed to cleanly generate HTML for static sites that can be run on the clearnet as well as a Tor Onion site with exactly one fewer modifications than the unpatched version of this code makes possible, but again, if you take a minute to more carefully read the original PR this would be self-evident to you.

@DirtyF
Copy link
Member

DirtyF commented Jul 16, 2020

I did read, please understand as maintainers we have to put things in perspective:

  1. We already generate a valid URL feed (with absolute URL)
  2. On the 567 508 users of the gem, only 1 person is asking for this
  3. You already patched it for your edge case.

I would mark it as won't fix. I'll defer to @pathawks and @ashmaroli on this one.

@fabacab
Copy link
Author

fabacab commented Jul 16, 2020

I did read, please understand as maintainers we have to put things in perspective

I understand maintainers are busy. That's why I upstreamed my patch as a PR, so you wouldn't have to do any extra work.

On a non-technical note, please understand it's a bit irksome and very off-putting to be gruffly approached as though I'm asking for something theoretically when it's plain as day by prior conversation that that isn't the case, especially in the context of having gone out of my way and taken the extra time to offer to contribute to this project. You could have just posted, "Can you give us a specific example where this modification is useful to you?" You'd have ellicited a less obviously irritated response from me, and simultaneously saved yourself some typing.

I would mark it as won't fix. I'll defer to @pathawks and @ashmaroli on this one.

I'll reiterate what I wrote in my original PR, emphasis added:

Using the relative_url filter does not impact feed reader use (at least, in my testing), so this should be a backward-compatible change that improves the versatility of the rendered HTML code.

Generating sites for Tor Onion services are not the only scenario in which relative URLs are helpful, and I don't even expect it to be the most common one, it just happens to be the reason I patched the Gem for myself. If there is a situation in which an absolute URL "works" but a relative URL does not, it hasn't been brought up on this thread yet. But the inverse, cases where a relative URL "works" and an absolute one does not, have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants