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

ENH Also check BASE_URL in host #10155

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Nov 21, 2021

@emteknetnz emteknetnz force-pushed the pulls/4/director-base-url branch 2 times, most recently from 306b601 to 458596f Compare November 21, 2021 22:41
@michalkleiner
Copy link
Contributor

I'm not 100% sure about the change.

On a correctly configured site (having the BASE_URL configured), with this change, it would always trump any Host header or HTTP_HOST server variable. That basically removes the functionality where a site can respond to multiple domains and let PHP decide what to do with it because it will always behave as on the single domain coming from BASE_URL.

@emteknetnz
Copy link
Member Author

That's a good point, probably makes sense to move the call to baseURL() to lower down in the host() method

@emteknetnz emteknetnz force-pushed the pulls/4/director-base-url branch 2 times, most recently from 8f083e7 to c4ce944 Compare November 21, 2021 23:21
@emteknetnz emteknetnz changed the title ENH Call baseURL() instead of just alternate_base_url ENH Also check BASE_URL in host Nov 21, 2021
@emteknetnz emteknetnz force-pushed the pulls/4/director-base-url branch from c4ce944 to c83d36a Compare November 21, 2021 23:22
@emteknetnz
Copy link
Member Author

On further inspection this is a pretty pointless PR.

$default_base_url comes from SS_BASE_URL https://github.com/silverstripe/silverstripe-framework/blob/4/src/Control/Director.php#L95

Which is also what feeds BASE_URL https://github.com/silverstripe/silverstripe-framework/blob/4/src/includes/constants.php#L111

@emteknetnz emteknetnz closed this Nov 21, 2021
@michalkleiner
Copy link
Contributor

michalkleiner commented Nov 21, 2021

The change now corresponds with what was already expected based on the method's docblock, and I think we could merge this, however, I'm not sure it solves the problem we discussed privately which might be coming from the place where it uses the current request to get the Host header.

EDIT: Ok to close if it functionally doesn't improve over the current state covered by other methods.

@GuySartorelli GuySartorelli deleted the pulls/4/director-base-url branch September 13, 2022 03:47
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