daattali / beautiful-jekyll

✨ Build a beautiful and simple website in literally minutes. Demo at https://beautifuljekyll.com
https://beautifuljekyll.com
MIT License
5.39k stars 16.27k forks source link

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

Closed abelcheung closed 5 years ago

abelcheung commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@abelcheung is this a sub-issue of https://github.com/daattali/beautiful-jekyll/issues/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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

PR #506 submitted.