Remove trailing slash from SiteTree links if no action present

[emteknetnz]

Silverstripe CMS currently is inconsistent as to whether URLs have a trailing slash or not - in fact there's a whole module dedicated to resolving this.

We should make this consistent.

Acceptance criteria

• CMS5 defaults to never having trailing slashes.
• There's a way to configure the CMS to always included trailing slashes
• Whatever the default is, you get redirected to it when appropriate
• There's a way to turn off the redirection altogether

Installer CI

This CI run includes all of the below PRs - if its phpunit and behat tests are clean, we should be okay to merge.
https://github.com/creative-commoners/silverstripe-installer/actions/runs/3962646464

Note that the unit tests failing are related to graphql and are not related to these PRs at all. These will be dealt with in a separate future "broken builds" card.

PRs

Merge in this specific order - but don't merge any until we're happy that they all seem correct..

• MNT Fix tests to work with no trailing slash on base url silverstripe-assets#537
• ENH Allow base URL to not have trailing slash silverstripe-admin#1438
• ENH Expose lib/urls from silverstripe/admin webpack-config#57
• SS5: add trailing slash config and redirect silverstripe-framework#10538
• SS5: Remove trailing slash from SiteTree links if no action present #2771
• ENH Allow URLs without trailing slashes silverstripe-asset-admin#1319
• ENH Allow URLs without trailing slashes silverstripe-campaign-admin#253
• MNT Update tests to not expect trailing slash silverstripe-versioned#388
• MNT Don't expect trailing slash in tests silverstripe-reports#157
• DOC Document changes to trailing slash in URLs developer-docs#136

[xini]

@GuySartorelli #2771 covers criteria 1, I'll submit individual PRs in framework for the others (Controller::join_links() and redirection middleware). Does that sound ok?

[GuySartorelli]

Please do all of these together in the same PR - it will make it a lot easier to review. As far as we're concerned it's all part of the same change.

[xini]

@GuySartorelli fair enough. But do you agree that the adding/removing of the trailing slash should happen in Controller::join_links() and the redirection in a middleware and that both of these are in framework, not the cms? I can't do one PR spanning two repositories, so it will be one here and one there, right?

[GuySartorelli]

Ahh, good point - for some reason I thought the original PR was in framework 😅
Yes, that sounds like a good way to go about it.

I can't remember what exactly the redirection was tbh - I'll get @maxime-rainville to speak to that part of it since he wrote the criteria.

[GuySartorelli]

I would suppose something similar to what https://github.com/axllent/silverstripe-trailing-slash does would be appropriate for the redirection.

[maxime-rainville]

I can't remember what exactly the redirection was tbh.

• If your site is configured to never use trailing slash, accessing a URL with a trailing slash should redirect you to the none-trailing-slash equivalent.
• If your site is configured to always use trailing slash, accessing a URL without a trailing slash should redirect you to the trailing-slash equivalent.
• If your site is configured not to care about trailing slashes, no redirection should occur.

[xini]

@GuySartorelli any updates on this?

[GuySartorelli]

Sorry @xini, I haven't had time to look at this. It's in our ready column for anyone in the team to pick up and review, but it's not the heighest priority item in the list right now.

[xini]

@GuySartorelli that's ok, thanks.

[GuySartorelli]

@xini I've cleaned up the PRs, and added some for the necessary javascript adjustments. Please give the changes a look-over to make sure you're happy with them, but regardless, once the installer CI run linked in this issue is finished and passing correctly I'll kcik this over to someone else to review (now that I've done some of the work I can't finish the review myself)

[xini]

@GuySartorelli thank you very much! It looks good to me, but I can't really comment on the js side of things. Are the failing tests related or not?

[GuySartorelli]

The PRs are inter-related, so it's basically impossible for the individual PRs to go green. That's what the installer CI run is for, linked in the description of the issue.
Now that some of those tests are finishing I see some failures there I'll have to tidy up, which yes are related.

[xini]

right. ok. thanks!

[emteknetnz]

@GuySartorelli half the PRs are still draft?

[GuySartorelli]

@emteknetnz Yes. This is explained in the PRs but for convenience:
There is a webpack-config PR which needs to be merged, tagged, and released. Once that is done, we need to yarn upgrade @silverstripe/webpack-config on some of the PRs (the draft ones) so that we can have the changes from that PR.

I've included those changes in the built js by building with a local copy of the changes, but CI and subsequent yarn build from other people won't include those changes unless the PR includes an update to yarn.lock which can only happen after webpack-config is released.

So we'll need to get that one out the door, then I'll update the relevant PRs and mark them ready, and then those can be merged. But the work as a whole should be reviewed before anything is merged.

[GuySartorelli]

All PRs are merged.
Thanks @xini for your work on this! So good to have this normalised now.

[xini]

@GuySartorelli thanks so much!