silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
719 stars 820 forks source link

CanonicalURLMiddleware default to all environments #11132

Closed blueo closed 4 months ago

blueo commented 4 months ago

Description

Having a Live/Prod only feature sets people up for bugs. A common scenario for this one is a javascript feature that requests from an API - its easy to forget a trailing slash in this scenario and it will work on your local an your test environment - failing only once you deploy it to prod. I'd suggest we turn this on everywhere except the CLI by default (or, disable it by default).

Manual testing steps

Issues

Pull request checklist

lekoala commented 4 months ago

+1 for this, I ended up using axllent/silverstripe-trailing-slash anyway because of this to make sure it all behave consistently. I believe the way it's handled in the core should align with whatever that module is doing.

I'm not even sure why it was meaningful in the first place to take env into consideration for this

GuySartorelli commented 4 months ago

Bear in mind that this middleware is responsible for more than just the trailing slash redirection. I'm going to have to reject this PR, because this would break for anyone who sets forceWWW or forceSSL to true and expects it to not affect their development environments.

In a future major release we should probably get rid of enabledEnvs and just expect people to set environment-specific YAML config in the first place - but we can't change the default affected environments in a minor release without breaking functionality for some users.

In the meantime, you can configure this in your own projects via the injector YAML configuration.