-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Set trailingSlash: true for @site.url #17859
base: main
Are you sure you want to change the base?
Conversation
Ghost policy is to add a trailing slash to all URLs generated. However, it was not doing that for @site.url. That created a problem for Ghost installations at subdirectories, because requests to the generated URL like example.com/blog would be redirected by Ghost to the trailing-slash version like example.com/blog/. That redirect harmed SEO. This replaces trailingSlash: false with trailingSlash: true in the @site.url generation. Now Ghost complies with it's trailing slash policy and avoids creating an unnecessary, SEO-harming redirect.
@ronaldlangeveld I'm hoping this is an easy win, would you have any time to do the code review? |
We ran into this too - we could really use a site.url with trailing slash option (or default) |
@daniellockyer looks like you're a backend dev (I'm not sure who else to reach out to). Could you review + merge this PR? |
@cmraible could you help me find a reviewer for this PR? I've been trying to contribute this small bugfix for 8 months, and for some reason nobody with Ghost has responded yet. Would really appreciate your help 🙏 |
In an open source project that has 42k stars, someone is begging here to make their PR to be noticed since 8 months. That's really sad. :( |
@@ -9,7 +9,7 @@ function updateLocalTemplateOptions(req, res, next) { | |||
|
|||
// adjust @site.url for http/https based on the incoming request | |||
const siteData = { | |||
url: urlUtils.urlFor('home', {trailingSlash: false}, true) |
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.
Could you test this with storages? I think storages uses this base url to construct uploaded file urls. It will be nice to be sure this doesn't break anything (I'm not sure if we have automated tests cover those cases).
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.
@muratcorlu thanks for reviewing. That's a good question. I think a Ghost backend developer could answer that question pretty quickly – certainly much quicker than me.
I think if storages are simply appending strings to the base URL, there should be a separate PR to make that more intelligent and bug-proof. If there is also missing test coverage, it should probably be added at the same time.
Hi @dan-jensen, thanks for submitting, and apologies for not taking a look at this PR sooner! My main concern here is that adding the trailing slash to Just for my own edification, could you elaborate on how this behavior is harming SEO? It looks like redirecting to the version with a trailing slash is recommended by Google but I'm not an expert in SEO so happy to learn something new here! I realize the issue isn't that we're doing the redirect, but that we're linking to the non-trailing slash version of the URL, but I don't fully understand how that is impacting SEO. |
Also, just in the interest of offering a workaround while we sort this out, I wonder if this couldn't be solved more expediently at the theme level by adding the trailing slash manually to these links? Are you using an official theme or something custom? |
@cmraible fantastic, thanks for taking a look!
I became aware of this problem when an Ahrefs SEO scan flagged these redirects as harming SEO on a site. The Ahrefs blog says Google stopped directly penalizing for redirects in 2019. So I believe the SEO penalty is because of the added delay to page load times caused by the second request.
Yep, that's what we did. But now we can't update the existing theme or change themes without going back through the files and making those changes again. Plus, other people will end up going through this same experience, and they might not know how.
Yes, but for every theme where that's true for domain/subdomain installations, the inverse (no slash instead of double-slash) is currently a problem for subdirectory installations. Also, I'm not sure the core should suffer from an inconsistency because some themes do something they shouldn't. Shouldn't themes use a helper method to generate URLs, or use only paths, to avoid this exact conundrum?
Yes, that is my biggest concern too. Maybe wait for the next major release, where breaking changes are expected? |
Ghost policy is to add a trailing slash to all URLs generated. However, it was not doing that for
@site.url
. That created a problem for Ghost installations at subdirectories, because requests to the generated URL likeexample.com/blog
would be redirected by Ghost to the trailing-slash version likeexample.com/blog/
. That redirect harmed SEO.This replaces
trailingSlash: false
withtrailingSlash: true
in the@site.url
generation. Now Ghost complies with its trailing slash policy and avoids creating an unnecessary, SEO-harming redirect. This helps Ghost installations at subdirectories, and does not affect Ghost installations at domains.Resolves #16712