remove trailing slash from SiteTree links if no action present

[xini]

To my knowledge, SiteTree::RelativeLink() is currently the only place in SS where a trailing slash is added when there is no $action parameter given.

Controller::join_links already adds slashes between sections of the path, this explicit slash is not needed.

This change would help with canonicalisation of URLs.

[kinglozzer]

Hi @xini, there’s been a lot of discussion about this in silverstripe/silverstripe-framework#8882, the general consensus was against making any changes to this in core (or at the very least, making any changes configurable/opt-in).

[kinglozzer]

Test failures would need addressing, and this change would need to be “opt-in”

[xini]

Hi @kinglozzer,

Thanks for your feedback.

I completely agree with @dhensby 's last comment in silverstripe/silverstripe-framework#8882.

In accordance with 3) this PR aligns SiteTree::Link() with all other URL creating methods in SS, removing the trailing slash from page links.

As pointed out in that comment, redirects and canonical tags are left to a site's dev, rather than being considered the framework's responsibility.

What is the best way to make this “opt-in”? Best would be for this to be the default for new installations, but optional for existing ones? How do I do that?

Also, to make URLs more consistent (and the method simpler), `Controller::join_links()' could get an optional update as well. How would I link these PRs together over the two repos?

diff --git a/src/Control/Controller.php b/src/Control/Controller.php
index 09e8e7e1a..2b8eaadbe 100644
--- a/src/Control/Controller.php
+++ b/src/Control/Controller.php
@@ -690,10 +690,10 @@ class Controller extends RequestHandler implements TemplateGlobalProvider
             }
             if ((is_string($arg) && $arg) || is_numeric($arg)) {
                 $arg = (string) $arg;
-                if ($result && substr($result, -1) != '/' && $arg[0] != '/') {
-                    $result .= "/$arg";
+                if (strlen($result) > 0) {
+                    $result .= '/' . trim($arg, '/');
                 } else {
-                    $result .= (substr($result, -1) == '/' && $arg[0] == '/') ? ltrim($arg, '/') : $arg;
+                    $result .= rtrim($arg, '/');
                 }
             }
         }

Thank you very much.

[kinglozzer]

As pointed out in that comment, redirects and canonical tags are left to a site's dev, rather than being considered the framework's responsibility.

Yep that’s good - adjusting link generation is far safer than adding redirects!

What is the best way to make this “opt-in”? Best would be for this to be the default for new installations, but optional for existing ones? How do I do that?

We achieved this with other issues by using config values which could then be added to the config in the recipe.

One possible problem here is that we’re fixing the “remove trailing slashes” use case, but not the “force trailing slashes” one - both are equally valid. I’m going to post some ramblings on the linked issue, bear with me...

[dhensby]

I'm going to close this issue:

1. It's not been touched for 6 months
2. The general feeling from core contributors about this is it should be a separate module / not something we change in core. if it is changed in core it's opt-in.
3. The fact tests have to change indicate a change in behaviour which we'd want to avoid in a minor release