Skip to content
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

FIX Correct trailing slash for locale in another domain #872

Conversation

GuySartorelli
Copy link
Contributor

@GuySartorelli GuySartorelli commented Aug 13, 2024

Fixes https://github.com/tractorcow-farm/silverstripe-fluent/actions/runs/10362465353/job/28684501581 which consists of failures like the below:

 Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'http://www.example.com/about-us/my-staff/'
+'http://www.example.com/about-us/my-staff'

As of silverstripe/framework 5.2.20, Controller::normaliseTrailingSlash() won't normalise trailing slash for absolute URLs where the domain doesn't match the current site domain.

This means if getting a link to a page on Locale A from Locale B, if the domains don't match, the trailing slash won't be normalised.

updateLink() works in conjunction with updateRelativeLink(), so for pages other than the home page the trailing slash is normalised already in updateRelativeLink(). But for home pages updateRelativeLink() will always resolve to '/', which means before this PR, a trailing slash would always be added for home page links in different locales where domains don't match.

Note that this PR doesn't attempt to resolve this for any other type of data object.

It's possible that it would be better to implement the updateIsSiteUrl() extension hook in FluentDirectorExtension and saying "yes that's a site URL" if it belongs to a domain of any locale. I didn't attempt that because I don't know what sort of flow-on effects that might have. I'm especially hesitant to do that in a patch release.

@tractorcow do you have any thoughts on this?

Issue

@GuySartorelli GuySartorelli force-pushed the pulls/7.1/fix-trailing-slash branch from 762385d to 7234e98 Compare August 13, 2024 04:15
@GuySartorelli
Copy link
Contributor Author

Had to add some logic to Locale::getBaseURL() as well to resolve https://github.com/tractorcow-farm/silverstripe-fluent/actions/runs/10363343899/job/28686805029?pr=872

I'm now leaning towards implementing FluentDirectorExtension::updateIsSiteUrl() which seems like a more robust solution - but again I'm not sure what sort of flow-on effects that might have, so I'd like more opinions before I do that.

Probably this should be merged as-is either way - I think implementing that change should be done in a minor release if it's done at all.

@GuySartorelli
Copy link
Contributor Author

Also FWIW we have the same problem in subsites: https://github.com/silverstripe/silverstripe-subsites/actions/runs/10278313787/job/28441800184

@emteknetnz emteknetnz merged commit 8f7d803 into tractorcow-farm:7.1 Aug 13, 2024
9 checks passed
@emteknetnz emteknetnz deleted the pulls/7.1/fix-trailing-slash branch August 13, 2024 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants