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

Some absolute URLs broken when theme used as project page #500

Closed
abelcheung opened this issue May 30, 2019 · 6 comments
Closed

Some absolute URLs broken when theme used as project page #500

abelcheung opened this issue May 30, 2019 · 6 comments

Comments

@abelcheung
Copy link
Contributor

abelcheung commented May 30, 2019

This only concerns the case when theme is used as project page (baseurl non-empty). Although the layout templates and misc files have taken care to only prepend site.url as absolute url, it won't work if absolute_url liquid filter is used. All such links would have project path component inserted twice (like https://user.github.io/proj/proj/aboutme), resulting in invalid links.

This affects at least jekyll-sitemap plugin for now, which uses absolute_url internally. And most likely affects any other user added plugins utilizing absolute_url, as well as user overrided template using such filter.

@abelcheung
Copy link
Contributor Author

IMO the root cause is this theme using site.url property in a way different from Jekyll accepted standard. Jekyll expects site.url to include only up to host name part, and site.baseurl contains the project path prefix. (Reference 1) (Reference 2)

{{ '/path' | absolute_url }} filter works essentially like {{ site.url }}{{ site.baseurl }}/path. But for the theme in current state, project name is included in both site.url and site.baseurl, hence the error.

Given the substantial user base, any change in this area would perhaps make many peoples' life difficult though.

@daattali
Copy link
Owner

daattali commented Jun 7, 2019

@abelcheung is this a sub-issue of #254 ?

I haven't had a chance to look at your recent PR yet, but I assume that PR implements the change you suggest here, is that correct?

@abelcheung
Copy link
Contributor Author

Yes, you can think of it as a subtask of #254, which covers a whole class of generic issues. However, this issue and the PR I submitted cover entirely different parts.

The essential portion of PR #497 is css/main.css, where images would fail to show by default if one follows how it's done in _config.yml. Since I have already used relative_url filter, it might make sense to use it globally across the whole project for coherence (and usage of relative_url won't break anything AFAICT).

And this issue is about absolute links, which need a breaking change for many people (who don't sync changes carefully), and I haven't submitted any PR yet.

@daattali
Copy link
Owner

daattali commented Jun 8, 2019

I'm usually extremely conservative and against breaking changes, but I think this is something that should be done. You seem to have a great grasp of this concept, so if you find the time, it would be greatly appreciated if you helped fix this "path variables mess" that currently exists.

Even though it's a breaking change, we can include a paragraph that clearly explains how to convert all previous paths to the new correct paths. A similar issue happened a few years ago when GitHub Pages updated to a new version of jekyll that had a different behaviour for some of these variables, so I had an FAQ for the hundreds of people that all of a sudden had their sites broken. It's going to be a painful but necessary change.

@abelcheung
Copy link
Contributor Author

OK, in that case I'll try to prepare a PR by weekend, along with more verbose comments in _config.yml. Is extra FAQ needed in this case?

@abelcheung
Copy link
Contributor Author

PR #506 submitted.

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

No branches or pull requests

2 participants