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 Fluent now respects existing base URL prefixes in URL segment fields when no fluent domain exists #523

Conversation

robbieaverill
Copy link
Contributor

Fluent sets the URLSegment field's URLPrefix to be the base domain plus the parent page path including the current locale. When no fluent domains are configured, this defaulted to Director::absoluteBaseURL(). This was a bit heavy handed, since it doesn't consider any previous modifications to SiteTreeURLSegmentField::setURLPrefix() (e.g. in subsites, where it sets the subsite domain).

I've changed this to use the existing URL prefix instead.

There will still be conflicts if you use subsite domains and fluent domains at the same time, since they represent the same functionality, but that can be documented.

Fixes silverstripe/silverstripe-subsites#416

@tractorcow
Copy link
Collaborator

Looks good to me.

@tractorcow tractorcow merged commit fd0848e into tractorcow-farm:4.2 Jun 4, 2019
@tractorcow tractorcow deleted the pulls/4.2/respect-existing-urlprefix branch June 4, 2019 02:08
@@ -264,15 +264,14 @@ protected function addLocalePrefixToUrlSegment(FieldList $fields)
if ($domain) {
$parentBase = Controller::join_links($domain->Link(), Director::baseURL());
} else {
$parentBase = Director::absoluteBaseURL();
// Use any existing pre-configured base URL prefix for the field
$parentBase = $segmentField->getURLPrefix();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the bug is that this link includes the parent url, whereas so does $parentRelative. It's adding parent urlsegment twice.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to revert this at #539

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants