Default to current port in urlRedirectAction #5984
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The idea is right but the implementation not. Seems this patch is not as "simple" as you said. |
|
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... |
|
That would be the best solution that is currently possible but not the best solution that should be possible. |
|
Bummer, I forgot to check if the current port is a standard port... |
|
add some tests |
|
Added tests and fixed my previous error |
|
@Tobion is there anything else I needed for this? |
|
To be consistent with how we manage HTTP ports elsewhere, I'd rather use the values of the 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 The parameter name is probably not the best one, but that could be changed then in master. |
|
@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: |
|
Your order of preference looks good to me. |
|
Man this was more involved than I thought... :) |
fabpot
added a commit
that referenced
this pull request
Nov 19, 2012
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!
|
@flojon Looks really good. Thanks a lot! |
|
@fabpot You're welcome! :) |
| } elseif ('https' === $scheme && 443 != $httpsPort) { | ||
| $port = ':'.$httpsPort; | ||
| if ('http' === $scheme) { | ||
| if ($httpPort == null) { |
There was a problem hiding this comment.
null === $httpPort according to CS
Tobion
added a commit
to Tobion/symfony
that referenced
this pull request
Nov 21, 2012
this can happen when the config for the router is unset, but this method does not need to depend on routing. reading an unset config would raise an exception.
fabpot
added a commit
that referenced
this pull request
Nov 24, 2012
This PR was merged into the 2.0 branch. Commits ------- 29bfa13 small fix of #5984 when the container param is not set Discussion ---------- small fix of #5984 when the container param is not set this can happen when the config for the router is unset, but this method does not need to depend on routing. reading an unset config would raise an exception. --------------------------------------------------------------------------- by Tobion at 2012-11-19T20:44:31Z Ops, I guess it's wrong. Travis will probably confirm this in a moment. I will correct that. --------------------------------------------------------------------------- by flojon at 2012-11-20T22:40:07Z Yeah you changed the logic... --------------------------------------------------------------------------- by Tobion at 2012-11-21T14:42:48Z ok it's fixed.
fabpot
added a commit
that referenced
this pull request
Nov 29, 2012
* 2.0: [DependencyInjection] fixed composer.json [Form] Updated checks for the ICU version from 4.5+ to 4.7+ due to test failures with ICU 4.6 fixed CS small fix of #5984 when the container param is not set fixed CS Use better default ports in urlRedirectAction Add tests for urlRedirectAction Update src/Symfony/Component/DomCrawler/Tests/FormTest.php Update src/Symfony/Component/DomCrawler/Form.php [Security] remove escape charters from username provided by Digest DigestAuthenticationListener [Security] added test extra for digest authentication fixed CS [Security] Fixed digest authentication [Security] Fixed digest authentication [SecurityBundle] Convert Http method to uppercase in the config Use Norm Data instead of Data Conflicts: src/Symfony/Bridge/Doctrine/Form/EventListener/MergeCollectionListener.php src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php src/Symfony/Component/DependencyInjection/composer.json
fabpot
added a commit
that referenced
this pull request
Nov 29, 2012
* 2.1: (29 commits) [DependencyInjection] fixed composer.json [Validator] Fix typos in validators.ru.xlf Edited some minor grammar and style errors in russian validation file Updated Bulgarian translation [Form] improve error message with a "hasser" hint for PropertyAccessDeniedException [Form] Updated checks for the ICU version from 4.5+ to 4.7+ due to test failures with ICU 4.6 [Form] simplified a test from previous merge Update src/Symfony/Component/Form/Extension/Core/Type/FileType.php fixed CS Xliff with other node than source or target are ignored small fix of #5984 when the container param is not set Filesystem Component mirror symlinked directory fix [Process][Tests] fixed chainedCommandsOutput tests fixed CS Use better default ports in urlRedirectAction Add tests for urlRedirectAction info about session namespace fix upgrade info about locale Update src/Symfony/Component/DomCrawler/Tests/FormTest.php Update src/Symfony/Component/DomCrawler/Form.php ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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...