-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
refactor: remove $_SERVER['HTTP_HOST']
in RouteCollection
#5962
Conversation
0b45ebb
to
0fce2b8
Compare
Without it, tests fail. I don't know why.
4e91477
to
fe627b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change but that is a lot of adjustments to tests to accommodate it. Is this going to break people's tests if they are using anything HTTP-specific?
Maybe yes. This PR changes the point that fetches the Tests that sets The current CI4 code has many property or variable values that can be changed later. I want more classes that have complete constructor, and want more immutable classes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went and looked through my packages that I thought might set this directly, but none of them so. I think this is highly unlikely to break anything, and it brings good benefits and actually better testability.
Good to hear! |
Description
Depending on superglobals is bad practice.
Checklist: