10000 [HttpFoundation] fixed Request::create() method by fabpot · Pull Request #6995 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpFoundation] fixed Request::create() method #6995

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 7, 2013
Merged

[HttpFoundation] fixed Request::create() method #6995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Symfony/Component/HttpFoundation/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ CHANGELOG
2.2.0
-----

* fixed the Request::create() precedence (URI information always take precedence now)
Copy link
Contributor

Choose a reason for hiding this comment

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

As a widely used and @api tagged class, this probably deserves a "[BC BREAK]"

* added Request::getTrustedProxies()
* deprecated Request::isProxyTrusted()
* [BC BREAK] JsonResponse does not turn a top level empty array to an object anymore, use an ArrayObject to enforce objects
Expand Down
41 changes: 22 additions & 19 deletions src/Symfony/Component/HttpFoundation/Request.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,9 @@ public static function createFromGlobals()
/**
* Creates a Request based on a given URI and configuration.
*
* The information contained in the URI always take precedence
* over the other information (server and parameters).
*
* @param string $uri The URI
* @param string $method The HTTP method
* @param array $parameters The query (GET) or request (POST) parameters
Expand All @@ -267,7 +270,7 @@ public static function createFromGlobals()
*/
public static function create($uri, $method = 'GET', $parameters = array(), $cookies = array(), $files = array(), $server = array(), $content = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason not to use array type hinting for array arguments ($parameters, $cookies, etc.)?

Copy link
Contributor

Choose a reason for hiding this comment

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

This would be BC break, and it's to late to have such in 2.2.

{
$defaults = array(
$server = array_replace(array(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just $server += array(...);?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rybakit do you have any idea on the perf impact ? (out of my own curiosity)

Copy link
Contributor

Choose a reason for hiding this comment

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

@vicb, a simple benchmark - https://gist.github.com/rybakit/4737807

Copy link
Contributor

Choose a reason for hiding this comment

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

Quite micro difference anyway (notice it's visible when looping more than 10000 times)...

Copy link
Contributor

Choose a reason for hiding this comment

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

@rybakit, do you have XDebug enabled ? (If yes could you please re-test by disabling it).

Twice as fast is a notable difference IMO in favor of union.

Copy link
Contributor

Choose a reason for hiding this comment

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

@stloyd, true, but + array() is also more readable, at least for me.

@vicb, I had ZendDebugger enabled (rev 3). Check the latest gist rev 4 (without ZendDebugger).

Copy link
Contributor

Choose a reason for hiding this comment

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

@rybakit could you please create a new issue to discuss this as the PR has been closed - you suggestion is applicable at several places in the code. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vicb, done.

'SERVER_NAME' => 'localhost',
'SERVER_PORT' => 80,
'HTTP_HOST' => 'localhost',
Expand All @@ -280,32 +283,38 @@ public static function create($uri, $method = 'GET', $parameters = array(), $coo
'SCRIPT_FILENAME' => '',
'SERVER_PROTOCOL' => 'HTTP/1.1',
'REQUEST_TIME' => time(),
);
), $server);

$server['PATH_INFO'] = '';
$server['REQUEST_METHOD'] = strtoupper($method);

$components = parse_url($uri);
if (isset($components['host'])) {
$defaults['SERVER_NAME'] = $components['host'];
$defaults['HTTP_HOST'] = $components['host'];
$server['SERVER_NAME'] = $components['host'];
$server['HTTP_HOST'] = $components['host'];
}

if (isset($components['scheme'])) {
if ('https' === $components['scheme']) {
$defaults['HTTPS'] = 'on';
$defaults['SERVER_PORT'] = 443;
$server['HTTPS'] = 'on';
$server['SERVER_PORT'] = 443;
} else {
unset($server['HTTPS']);
$server['SERVER_PORT'] = 80;
}
}

if (isset($components['port'])) {
$defaults['SERVER_PORT'] = $components['port'];
$defaults['HTTP_HOST'] = $defaults['HTTP_HOST'].':'.$components['port'];
$server['SERVER_PORT'] = $components['port'];
$server['HTTP_HOST'] = $server['HTTP_HOST'].':'.$components['port'];
}

if (isset($components['user'])) {
$defaults['PHP_AUTH_USER'] = $components['user'];
$server['PHP_AUTH_USER'] = $components['user'];
}

if (isset($components['pass'])) {
$defaults['PHP_AUTH_PW'] = $components['pass'];
$server['PHP_AUTH_PW'] = $components['pass'];
}

if (!isset($components['path'])) {
Expand All @@ -316,7 +325,7 @@ public static function create($uri, $method = 'GET', $parameters = array(), $coo
case 'POST':
case 'PUT':
case 'DELETE':
$defaults['CONTENT_TYPE'] = 'application/x-www-form-urlencoded';
$server['CONTENT_TYPE'] = 'application/x-www-form-urlencoded';
case 'PATCH':
$request = $parameters;
$query = array();
Expand All @@ -333,14 +342,8 @@ public static function create($uri, $method = 'GET', $parameters = array(), $coo
}
$queryString = http_build_query($query, '', '&');

$uri = $components['path'].('' !== $queryString ? '?'.$queryString : '');

$server = array_replace($defaults, $server, array(
'REQUEST_METHOD' => strtoupper($method),
'PATH_INFO' => '',
'REQUEST_URI' => $uri,
'QUERY_STRING' => $queryString,
));
$server['REQUEST_URI'] = $components['path'].('' !== $queryString ? '?'.$queryString : '');
$server['QUERY_STRING'] = $queryString;

return new static($query, $request, array(), $cookies, $files, $server, $content);
}
Expand Down
35 changes: 35 additions & 0 deletions src/Symfony/Component/HttpFoundation/Tests/RequestTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,41 @@ public function testCreate()
$this->assertFalse($request->isSecure());
}

/**
* @covers Symfony\Component\HttpFoundation\Request::create
*/
public function testCreateCheckPrecedence()
{
// server is used by default
$request = Request::create('/', 'GET', array(), array(), array(), array(
'HTTP_HOST' => 'example.com',
'HTTPS' => 'on',
'SERVER_PORT' => 443,
'PHP_AUTH_USER' => 'fabien',
'PHP_AUTH_PW' => 'pa$$',
'QUERY_STRING' => 'foo=bar',
));
$this->assertEquals('example.com', $request->getHost());
$this->assertEquals(443, $request->getPort());
$this->assertTrue($request->isSecure());
$this->assertEquals('fabien', $request->getUser());
$this->assertEquals('pa$$', $request->getPassword());
$this->assertEquals('', $request->getQueryString());

// URI has precedence over server
$request = Request::create('http://thomas:pokemon@example.net:8080/?foo=bar', 'GET', array(), array(), array(), array(
'HTTP_HOST' => 'example.com',
'HTTPS' => 'on',
'SERVER_PORT' => 443,
));
$this->assertEquals('example.net', $request->getHost());
$this->assertEquals(8080, $request->getPort());
$this->assertFalse($request->isSecure());
$this->assertEquals('thomas', $request->getUser());
$this->assertEquals('pokemon', $request->getPassword());
$this->assertEquals('foo=bar', $request->getQueryString());
}

/**
* @covers Symfony\Component\HttpFoundation\Request::duplicate
*/
Expand Down
0