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

Inconsistent current_path #1086

Closed
yanghuidong opened this issue Jul 12, 2020 · 10 comments
Closed

Inconsistent current_path #1086

yanghuidong opened this issue Jul 12, 2020 · 10 comments
Labels
bug done in pr Already done in a PR

Comments

@yanghuidong
Copy link
Contributor

Bug Report

Environment

Zola version: 0.11.0

Expected Behavior

The global variable current_path should have a consistent format regarding leading and trailing slashes.

Current Behavior

  • At the site root, e.g. http://127.0.0.1/, current_path equals /, which I suppose is the trailing slash;
  • At a root pager, e.g. http://127.0.0.1/page/2/, it equals page/2/, so again, with a trailing slash, so far so good;
  • At a taxonomy name's "list" page, e.g. http://127.0.0.1/tags/, it equals tags, note the trailing slash is missing;
  • At a taxonomy term's "single" page, e.g. http://127.0.0.1/tags/some-tag/, it equals /tags/some-tag, again missing the trailing slash, but also with an apparent leading slash.

Step to reproduce

To show this, just use this variable somewhere in the base template, e.g. add <link href="{{ current_path | safe }}"> in the head?

Sorry if this is a known issue, I did search the issues beforehand though.

Last but not least, thanks for creating and developing Zola.

@Keats
Copy link
Collaborator

Keats commented Jul 19, 2020

In theory it should consistently have a leading/trailing /, looks like we missed some for taxonomies.

@Keats Keats added the bug label Jul 19, 2020
@yanghuidong
Copy link
Contributor Author

By the way, as a side note from my personal experience, current_path is very useful as the "root relative URL", which as you know always starts with a leading slash, and it is effectively the absolute path from the FS point of view (absolute == relative to the root). I think for internal-facing links of a site, i.e. except for links in feeds, sitemaps, and other files meant for outside agents, root relative URLs are drier, more "portable", yet fully capable replacement of full URLs. Therefore, I find myself using current_path instead of current_url all the time, but just having to always prefix it with a slash. This is no big deal of course, but just in case usage pattern is something of interest to Zola?

I'm aware that the documentation clarifies that current_path "never start[s] with a slash", so if you don't want to made breaking changes for such an inconvenience I can understand. In fact I'm very curious if people have different uses of current_path where a leading slash is not desirable?

BTW, not to nitpick, but the same documentation page defines this variable as "the path (full URL without base_url) of the current page", but in Zola, the canonical base_url doesn't have a trailing slash, so if you strip it from the full URL, you actually do get a leading slash for current_path! :)

@yanghuidong
Copy link
Contributor Author

OK, one more follow-up to the non-nitpick: currently, the site root's current_path equals /, right? And its current_url equals e.g. http://127.0.0.1/, so assuming canonical base_url does not have a trailing slash, then indeed the logic is clear: current_path == current_url | trim_start_matches(pat=base_url), in Tera. Now following this simple rule you indeed get a leading slash for all values of current_path, which I think would be very nice.

On the other hand, if current_path must not have a leading slash, as stated in the current documentation, then we would also have to trim that slash following base_url, which means the current_path of the site root would be an empty string instead of a slash (unless of course you treat site root as a special case).

To summarize, if we follow a simple logic to derive current_path from current_url, then all values of current_path should have a leading slash, otherwise, site root's current_path would have to be an empty string.

@Keats
Copy link
Collaborator

Keats commented Jul 23, 2020

I think current_path is matching page.path and section.path which do not start with a /, so you can do if current_path == page.path. I don't remember why {page,section}.path do not start with a / though.

@Keats
Copy link
Collaborator

Keats commented Jul 24, 2020

Having thought about it for a while, I now think all the paths should start with a / to follow the more common path semantics.
That's a big breaking change but if people are comparing paths from Zola like I use it for, it shouldn't too annoying.

@yanghuidong are you interested in working on path?

@yanghuidong
Copy link
Contributor Author

I'm interested of course :) But I'm also new to Zola, and never had real experience collaborating via git, so it would take me some unknown amount of time to actually start getting to know the code, and even learn the GitHub basics, which means I will likely need some guidance just to get going. So I'm not sure if the communication overhead thus incurred would negatively impact your schedule (if you have any)? Sorry for being clueless, I'd like to help but that would take some learning on my part, so just wanted to check if you think a total noob is the right for the job. What do you suggest @Keats?

@Keats
Copy link
Collaborator

Keats commented Jul 25, 2020

No schedule right now, don't worry about it. Git will probably be more problematic than Rust/Zola :)

To start with, make sure you work on the next branch in git.
What I like to do with a clear defined issue like that is update the docs first and look for any tests that needs to be modified if there are some obvious ones or just do the changes and see what fails after cargo test --all.
In this case, current_path comes from:

  1. {page, section}.path: those are generated in components/library/src/content/page.rs and components/library/src/content/section.rs. Changing the path generation for those 2 should get you 90% there.
  2. The last 10% are taxonomies, paginated content and maybe colocated assets which I don't know if there is good test coverage on (probably not since you ran into the issue). Adding more tests should help there

There are some integration tests in components/site/tests where you can just add current_path in all the templates and add a check for it in each test.

Let me know if you need more details on some stuff or if you are overwhelmed!

@yanghuidong
Copy link
Contributor Author

Regarding colocated assets, the documentation says it's designed such that we get to use relative paths to link to them, and I think that's nice, for obvious reasons. So I wanted to check with you @Keats if you meant in the comment above to switch to absolute paths for colocated assets as well, or did I misunderstand you? Or perhaps it was meant to be discussed?

@Keats
Copy link
Collaborator

Keats commented Jul 27, 2020

For colocated assets there was an interesting comment earlier: #977 (comment)
I need to check what's up before answering. If it doesn't work in RSS feeds then we shouldn't advocate for relative links like that.

@Keats
Copy link
Collaborator

Keats commented Jul 27, 2020

For colocated assets path: I think we want to keep them relative. We can combine them with the page/section permalink easily if we want absolute links.

cc @southerntofu

@Keats Keats added the done in pr Already done in a PR label Aug 18, 2020
@Keats Keats closed this as completed Sep 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug done in pr Already done in a PR
Projects
None yet
Development

No branches or pull requests

2 participants