-
Notifications
You must be signed in to change notification settings - Fork 983
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
Restore trailing slash behaviour in serve command #2482
Conversation
Howdy @Keats! I'm not sure why it doesn't let me request a review, but I think this is good to go. |
let constructed_base_url = construct_url(&base_url, no_port_append, interface_port); | ||
let mut constructed_base_url = construct_url(&base_url, no_port_append, interface_port); | ||
|
||
if !site.config.base_url.ends_with("/") && constructed_base_url != "/" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm what does this do in practice? Do we want it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, to be honest I was hoping you might know better than I.
It restores the logic here: 4bf67af#diff-05643f021a24da1c9475eb37428b799095c0cfbd79f1b79e658c39615746315dL357-L361
This never came up as I was testing #2311 as it doesn't actually seem to have any effect on the generated site. All the permalinks render as one might expect whether you strip that trailing slash or not. EXCEPT, in one case that I've been able to spot so far.
On my site, using the Anpu theme for a base, the config.toml
calls for a few navigation links that are specified like so:
base_url = "https://jamwil.com"
# snip
[extra]
anpu_menu_links = [
{ url = "$BASE_URL/about/", name = "About" },
{ url = "$BASE_URL/categories/", name="Categories" },
{ url = "$BASE_URL/tags/", name = "Tags" },
]
On these links, and only these links it seems, if that logic does not exist to strip the trailing slash to match base_url
in the site config, then the links render with an extra slash, e.g., http://127.0.0.1:1111//about
.
To verify that the logic here matches the prior behaviour, I took the new unit tests that I've begun preparing in #2448 and grafted them onto the parent commit of 4bf67af, and the applicable tests pass.
I admit I find the logic around trailing slashes in zola to be a bit intractable. It seems we expect them throughout, but we do not expect it on the base URL as specified in config.toml
, so this little snippet allows the serve
logic to match the build
logic.
I'm super curious what your thoughts are vis-à-vis trailing slashes in general and how they should be treated. Perhaps my understanding is incorrect, or perhaps this is more indicative of the theme constructing URLs in a non-idiomatic way? There may be some opportunity to clean things up beyond this PR, but this one is merely to match the behaviour on the next
branch to that on master
so there are no surprises in the next release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory every URL in zola should have a trailing slash. I'm guessing we should probably remove one from the base_url when loading the config if there is one to ensure it is uniform and remove a few checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like a logical approach to me. Thanks for merging!
* Restore trailing slash behaviour in serve command. * Restore guard in case where base_url is just a slash.
* Restore trailing slash behaviour in serve command. * Restore guard in case where base_url is just a slash.
* Restore trailing slash behaviour in serve command. * Restore guard in case where base_url is just a slash.
This is a follow on to #2311 which restores the switch that will strip the trailing slash from the base url when calling
zola serve
if there is not trailing slash on the base url as defined in the site configuration.I noticed this discrepancy as I was beginning to prepare tests for #2448. I believe this now aligns with the prior behaviour.
Sanity check:
Code changes
(Delete or ignore this section for documentation changes)
next
branch?If the change is a new feature or adding to/changing an existing one: