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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 3, 2024
Merged

Restore trailing slash behaviour in serve command #2482

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fee33bc
Restore trailing slash behaviour in serve command.
jamwil Apr 25, 2024
af0d067
Restore guard in case where base_url is just a slash.
jamwil Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/cmd/serve.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,11 @@ fn create_new_site(
|u| u.to_string(),
);

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 != "/" {
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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?

Copy link
Contributor Author

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!

constructed_base_url.truncate(constructed_base_url.len() - 1);
}

site.enable_serve_mode();
site.set_base_url(constructed_base_url.clone());
Expand Down