fix: by default CanonicalURLMiddleware should run in all environments

wilr commented 4 months ago

Description

Currently CanonicalURLMiddleware only runs in production, this leads to the classic example of 'It works on mine(tm)' but more dangerously 'It works on the test site (tm)'. Out of the box, we should have as close to production behaviour as possible.

While the benefits don't apply to dev / test environments the side effects of the behaviour definitely apply.

cc @madmatt

Manual testing steps

Issues

Pull request checklist

[x] The target branch is correct
- See picking the right version
[x] All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
- Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
[x] The commit messages follow our commit message guidelines
[x] The PR follows our contribution guidelines
[x] Code changes follow our coding conventions
[x] This change is covered with tests (or tests aren't necessary for this change)
[x] Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
[x] CI is green

madmatt commented 4 months ago

Pushing this to dev might be a bit over the top (though having TLS on localhost is possible and is much more common in these days of Docker etc), but I definitely think it should apply to test and live because these environments should always be considered identical (in as many ways as possible).

wilr commented 4 months ago

Forcing SSL is still not enabled by default in dev. You'll need to opt in via Injector as you would currently.

madmatt commented 4 months ago

Oh okay I did miss that part, nice _b

mandrew commented 4 months ago

I got caught out on this yesterday :( however it's a big change and might be best to wait to 6? Also any documentation that points to CanonicalURLMiddleware working only in live mode will need to be changed

GuySartorelli commented 4 months ago

Please use the pull request template, which I have added back for you. The following need to be done:

open a new issue so that we have an issue to track. We track issues, not pull requests.
add some steps to test this change.
Tick all the checkboxes that are relevant - you may find there are things that the checkboxes will point out (e.g. for a bug fix 5 is not the correct branch to target)

wilr commented 4 months ago

@GuySartorelli updated to v6 now. If you want to see the issue in place have a fetch('page.url') somewhere on your page. You'll find this works on development environment, once you're in production this will return a 301 redirect to page.url/

GuySartorelli commented 4 months ago

You'll find this works on development environment, once you're in production this will return a 301 redirect to page.url/

I assume you mean this happens from a fresh installation - there are multiple items of configuration that could change this result, including setting the enabled environments via injector config.

GuySartorelli commented 4 months ago

I'll be happy to merge this once there is a matching PR to the silverstripe/developer-docs as per https://github.com/silverstripe/silverstripe-framework/pull/11154#discussion_r1506847367

wilr commented 4 months ago

I'll be happy to merge this once there is a matching PR to the silverstripe/developer-docs as per #11154 (comment)

Won't it just be automated generated as it's a bug fix?

GuySartorelli commented 4 months ago

Won't it just be automated generated as it's a bug fix?

The only things that are automatically generated for changelogs are:

commit references
deleted or changes API signatures (classes, interfaces, methods, etc) in minor releases

Changes to default values in protected member properties are not automatically documented in changelogs - and the potential ramifications and how to change those settings yourself are definitely not automatically documented.

wilr commented 4 months ago

Done! Created at https://github.com/silverstripe/developer-docs/pull/465

GuySartorelli commented 4 months ago

Fantastic. Can you please also create an issue and link both PRs to it as per https://github.com/silverstripe/silverstripe-framework/pull/11154#issuecomment-1970058062?

It's harder to track PRs than issues, especially when there are multiple related PRs.

silverstripe / silverstripe-framework