8000 merged branch flojon/patch-3 (PR #5984) · symfony/symfony@ebd5e92 · GitHub
[go: up one dir, main page]
Skip to content

Commit ebd5e92

Browse files
fabpotfabpot
committed
merged branch flojon/patch-3 (PR #5984)
This PR was merged into the 2.0 branch. Commits ------- 64b54dc Use better default ports in urlRedirectAction 64216f2 Add tests for urlRedirectAction Discussion ---------- Default to current port in urlRedirectAction I was a bit surprised when I used urlRedirectAction from a non-standard port (8000) it redirected me to port 80. I would argue that the default should be to use the current port instead. This is a simple patch to change that. This should only break in the case someone is relying on the current default to redirect from a non-standard port to the standard port, which should be a really rare case... --------------------------------------------------------------------------- by Tobion at 2012-11-11T20:29:54Z The idea is right but the implementation not. Seems this patch is not as "simple" as you said. When you're on HTTPS and want to redirect to $scheme = HTTP, then it still uses the current HTTPS port which is wrong. --------------------------------------------------------------------------- by flojon at 2012-11-11T20:36:47Z Ah, I see the problem. So I guess the correct behavior would be to use the current port if staying with the same scheme or go to standard port if switching scheme. Unless the user has specified a port which will always override... --------------------------------------------------------------------------- by Tobion at 2012-11-11T20:42:18Z That would be the best solution that is currently possible but not the best solution that should be possible. Because if you switch scheme but the other scheme does not use the standard port, it still doesn't work. Ideally the Request class had an option that allows to define the ports symfony should use for HTTP and HTTPS. This logic is in RequestContext, but it's not used here. --------------------------------------------------------------------------- by flojon at 2012-11-11T21:32:55Z Bummer, I forgot to check if the current port is a standard port... --------------------------------------------------------------------------- by Tobion at 2012-11-11T21:35:13Z add some tests --------------------------------------------------------------------------- by flojon at 2012-11-11T23:28:18Z Added tests and fixed my previous error --------------------------------------------------------------------------- by flojon at 2012-11-15T18:25:12Z @Tobion is there anything else I needed for this? --------------------------------------------------------------------------- by fabpot at 2012-11-19T12:56:04Z To be consistent with how we manage HTTP ports elsewhere, I'd rather use the values of the `request_listener.http_port` and `request_listener.https_port`: ```php if (null === $httpPort) { $httpPort = $this->container->getParameter('request_listener.http_port'); } if (null === $httpsPort) { $httpsPort = $this->container->getParameter('request_listener.https_port'); } ``` This is done in the `security.authentication.retry_entry_point` service and for the `router_listener` listener. The parameter name is probably not the best one, but that could be changed then in master. --------------------------------------------------------------------------- by flojon at 2012-11-19T13:49:18Z @fabpot But then you would need to set that parameter manually right? It wouldn't automatically redirect you to the same port, which was what I wanted to achieve... Could this be the right order of preference: If a value was specified in the route use that. Otherwise use the current port unless switching scheme then use the parameter value --------------------------------------------------------------------------- by fabpot at 2012-11-19T13:52:17Z Your order of preference looks good to me. --------------------------------------------------------------------------- by flojon at 2012-11-19T19:13:19Z Man this was more involved than I thought... :) Changed the logic to use the parameters when not using the current port. Also tried clean up the tests a little bit... Enjoy!
2 parents 54ffd9e + 64b54dc commit ebd5e92

File tree

2 files changed

+150
-5
lines changed

2 files changed

+150
-5
lines changed

‎src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ public function redirectAction($route, $permanent = false)
6464
*
6565
* @return Response A Response instance
6666
*/
67-
public function urlRedirectAction($path, $permanent = false, $scheme = null, $httpPort = 80, $httpsPort = 443)
67+
public function urlRedirectAction($path, $permanent = false, $scheme = null, $httpPort = null, $httpsPort = null)
6868
{
6969
if (!$path) {
7070
return new Response(null, 410);
@@ -88,10 +88,28 @@ public function urlRedirectAction($path, $permanent = false, $scheme = null, $ht
8888
}
8989

9090
$port = '';
91-
if ('http' === $scheme && 80 != $httpPort) {
92-
$port = ':'.$httpPort;
93-
} elseif ('https' === $scheme && 443 != $httpsPort) {
94-
$port = ':'.$httpsPort;
91+
if ('http' === $scheme) {
92+
if ($httpPort == null) {
93+
if ('http' === $request->getScheme()) {
94+
$httpPort = $request->getPort();
95+
} else {
96+
$httpPort = $this->container->getParameter('request_listener.http_port');
97+
}
98+
}
99+
if ($httpPort != null && $httpPort != 80) {
100+
$port = ":$httpPort";
101+
}
102+
} else if ('https' === $scheme) {
103+
if ($httpsPort == null) {
104+
if ('https' === $request->getScheme()) {
105+
$httpsPort = $request->getPort();
106+
} else {
107+
$httpsPort = $this->container->getParameter('request_listener.https_port');
108+
}
109+
}
110+
if ($httpsPort != null && $httpsPort != 443) {
111+
$port = ":$httpsPort";
112+
}
95113
}
96114

97115
$url = $scheme.'://'.$request->getHost().$port.$request->getBaseUrl().$path.$qs;

‎src/Symfony/Bundle/FrameworkBundle/Tests/Controller/RedirectControllerTest.php

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,4 +109,131 @@ public function testFullURL()
109109
$this->assertEquals('http://foo.bar/', $returnResponse->headers->get('Location'));
110110
$this->assertEquals(302, $returnResponse->getStatusCode());
111111
}
112+
113+
public function testUrlRedirectDefaultPortParameters()
114+
{
115+
$host = 'www.example.com';
116+
$baseUrl = '/base';
117+
$path = '/redirect-path';
118+
$httpPort = 1080;
119+
$httpsPort = 1443;
120+
121+
$expectedUrl = "https://$host:$httpsPort$baseUrl$path";
122+
$request = $this->createRequestObject('http', $host, $httpPort, $baseUrl);
123+
$controller = $this->createRedirectController($request, null, $httpsPort);
124+
$returnValue = $controller->urlRedirectAction($path, false, 'https');
125+
$this->assertRedirectUrl($returnValue, $expectedUrl);
126+
127+
$expectedUrl = "http://$host:$httpPort$baseUrl$path";
128+
$request = $this->createRequestObject('https', $host, $httpPort, $baseUrl);
129+
$controller = $this->createRedirectController($request, $httpPort);
130+
$returnValue = $controller->urlRedirectAction($path, false, 'http');
131+
$this->assertRedirectUrl($returnValue, $expectedUrl);
132+
}
133+
134+
public function urlRedirectProvider()
135+
{
136+
return array(
137+
// Standard ports
138+
array('http', null, null, 'http', 80, ""),
139+
array('http', 80, null, 'http', 80, ""),
140+
array('https', null, null, 'http', 80, ""),
141+
array('https', 80, null, 'http', 80, ""),
142+
143+
array('http', null, null, 'https', 443, ""),
144+
array('http', null, 443, 'https', 443, ""),
145+
array('https', null, null, 'https', 443, ""),
146+
array('https', null, 443, 'https', 443, ""),
147+
148+
// Non-standard ports
149+
array('http', null, null, 'http', 8080, ":8080"),
150+
array('http', 4080, null, 'http', 8080, ":4080"),
151+
array('http', 80, null, 'http', 8080, ""),
152+
array('https', null, null, 'http', 8080, ""),
153+
array('https', null, 8443, 'http', 8080, ":8443"),
154+
array('https', null, 443, 'http', 8080, ""),
155+
156+
array('https', null, null, 'https', 8443, ":8443"),
157+
array('https', null, 4443, 'https', 8443, ":4443"),
158+
array('https', null, 443, 'https', 8443, ""),
159+
array('http', null, null, 'https', 8443, ""),
160+
array('http', 8080, 4443, 'https', 8443, ":8080"),
161+
array('http', 80, 4443, 'https', 8443, ""),
162+
);
163+
}
164+
165+
/**
166+
* @dataProvider urlRedirectProvider
167+
*/
168+
public function testUrlRedirect($scheme, $httpPort, $httpsPort, $requestScheme, $requestPort, $expectedPort)
169+
{
170+
$host = 'www.example.com';
171+
$baseUrl = '/base';
172+
$path = '/redirect-path';
173+
$expectedUrl = "$scheme://$host$expectedPort$baseUrl$path";
174+
175+
$request = $this->createRequestObject($requestScheme, $host, $requestPort, $baseUrl);
176+
$controller = $this->createRedirectController($request);
177+
178+
$returnValue = $controller->urlRedirectAction($path, false, $scheme, $httpPort, $httpsPort);
179+
$this->assertRedirectUrl($returnValue, $expectedUrl);
180+
}
181+
182+
public function createRequestObject($scheme, $host, $port, $baseUrl)
183+
{
184+
$request = $this->getMock('Symfony\Component\HttpFoundation\Request');
185+
$request
186+
->expects($this->any())
187+
->method('getScheme')
188+
->will($this->returnValue($scheme));
189+
$request
190+
->expects($this->any())
191+
->method('getHost')
192+
->will($this->returnValue($host));
193+
$request
194+
->expects($this->any())
195+
->method('getPort')
196+
->will($this->returnValue($port));
197+
$request
198+
->expects($this->any())
199+
->method('getBaseUrl')
200+
->will($this->returnValue($baseUrl));
201+
202+
return $request;
203+
}
204+
205+
public function createRedirectController($request, $httpPort = null, $httpsPort = null)
206+
{
207+
$container = $this->getMock('Symfony\Component\DependencyInjection\ContainerInterface');
208+
$container
209+
->expects($this->at(0))
210+
->method('get')
211+
->with($this->equalTo('request'))
212+
->will($this->returnValue($request));
213+
if ($httpPort != null) {
214+
$container
215+
->expects($this->at(1))
216+
->method('getParameter')
217+
->with($this->equalTo('request_listener.http_port'))
218+
->will($this->returnValue($httpPort));
219+
}
220+
if ($httpsPort != null) {
221+
$container
222+
->expects($this->at(1))
223+
->method('getParameter')
224+
->with($this->equalTo('request_listener.https_port'))
225+
->will($this->returnValue($httpsPort));
226+
}
227+
228+
$controller = new RedirectController();
229+
$controller->setContainer($container);
230+
231+
return $controller;
232+
}
233+
234+
public function assertRedirectUrl($returnValue, $expectedUrl)
235+
{
236+
$this->assertTrue($returnValue->isRedirect($expectedUrl),
237+
"Expected: $expectedUrl\nGot: " . $returnValue->headers->get('Location'));
238+
}
112239
}

0 commit comments

Comments
 (0)
0