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

Cache host in request #33129

Closed
wants to merge 2 commits into from
Closed

Cache host in request #33129

wants to merge 2 commits into from

Conversation

lbassin
Copy link
Contributor

@lbassin lbassin commented Aug 12, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #32709
License MIT
Doc PR -

This PR stores request's host in a private variable, so we don't have run the entire function again the second time you call $request->getHost()

Copy link
Contributor

@seriquynh seriquynh left a comment

Choose a reason for hiding this comment

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

Some tests are broken because $request doesn't change the host name when new HTTP_HOST or SERVER_NAME is set.

$request = new Request();
'' === $request->getHost(); // true
$request->initialize([], [], [], [], [], ['HTTP_HOST' => 'www.example.com']);
'' === $request->getHost(); // true
$request->initialize([], [], [], [], [], ['SERVER_NAME' => 'www.example.com']);
'' === $request->getHost(); // true

@seriquynh
Copy link
Contributor

For the above example, we may clear the $host property while new server indexes are set or this is a breaking change, I think.

@fabpot
Copy link
Member

fabpot commented Sep 23, 2019

@lbassin What's the status of this PR?

@nicolas-grekas
Copy link
Member

I just had a look at the code: this cannot work because in order to reset the cached host, we'd need to track changes to headers and server variables. That's not possible.

I'd recommend getting the host once and reusing the return value instead.

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
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.

None yet

7 participants