Use better default ports in urlRedirectAction
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!
fixed CS
[Process][Tests] fixed chainedCommandsOutput tests

for OS where echo does not support -n option (like MacOS X)
merged branch tucksaun/fix-process-tests-osx (PR #6072)

This PR was merged into the 2.1 branch.

Commits
-------

0f4d8af [Process][Tests] fixed chainedCommandsOutput tests

Discussion
----------

[Process][Tests] fixed chainedCommandsOutput tests

Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes (previously no)
Fixes the following tickets: -
Todo: -
License of the code: MIT

Currently, if you run the Process Component tests on an OS where echo does not support the -n option (like MacOS X), they will fail.

This PR fix that by using only a simple echo which is universal.
merged branch Tobion/patch-7 (PR #6068)

This PR was merged into the 2.1 branch.

Commits
-------

915dd30 info about session namespace
4b21d18 fix upgrade info about locale

Discussion
----------

fix upgrade info about locale

it duplicated the header and had an irrelevant point inbetween

I also added an entry about session namespace.
Filesystem Component mirror symlinked directory fix
merged branch malaney/FilesystemMirrorSymlinkFix (PR #6073)

This PR was submitted for the master branch but it was merged into the 2.1 branch instead (closes #6073).

Commits
-------

c1c822b Filesystem Component mirror symlinked directory fix

Discussion
----------

Filesystem Component mirror symlinked directory fix

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets:#6070
Todo:
License of the code: MIT
Documentation PR:

Because of the order in which file types (i.e. dir, link, file) are checked in the Filesystem component, symlinked directories get treated as directories instead of symlinks.  As a result symlinked directories are not truly mirrored when performing a php app/console assets:install.  This PR checks that a file is a symlink BEFORE checking that its a directory and properly symlinks as necessary.
"Updated" the changelog according to stof feedback
Added ability to set controller result in the kernel.view event
merged branch ltouroumov/2.1 (PR #5885)

This PR was submitted for the 2.1 branch but it was merged into the master branch instead (closes #5885).

Commits
-------

efc22a8 [HttpKernel] Added ability to set controller result in the kernel.view event

Discussion
----------

[HttpKernel] Added ability to set controller result in the kernel.view event

Added the method `GetResponseForControllerResultEvent:setControllerResult(array $controllerResult)` to allow kernel.view events to inject data into the result.

---------------------------------------------------------------------------

by stof at 2012-11-01T13:48:51Z

new features canot be added in the 2.1 branch, which is a maintenance branch and so receives only bugfixes.

---------------------------------------------------------------------------

by fabpot at 2012-11-02T07:13:56Z

I'm not opposed to the change but can you share your use case with us?

---------------------------------------------------------------------------

by ltouroumov at 2012-11-02T07:55:55Z

I have custom annotations that allow controller methods to be invoked before the actions is run (`@BeforeFilter`) and after the action is run (`@ViewFilter`) and after the response is sent (`@ResponseFilter`).
The view filter allows to alter the controller result for example to inject parent entities into the template.

If you need more details I suppose I could post the code but I would have to ask the boss first.

---------------------------------------------------------------------------

by ltouroumov at 2012-11-02T07:57:53Z

Also I fixed the missing dollar sign in the [`6ffea1dc1cfa23844c6d90a1bfa9a282d1807a61`](https://github.com/ltouroumov/symfony/commit/6ffea1dc1cfa23844c6d90a1bfa9a282d1807a61) commit. But I can't seem to attach it to the pull request.

---------------------------------------------------------------------------

by henrikbjorn at 2012-11-02T08:12:20Z

Needs tests.

---------------------------------------------------------------------------

by ltouroumov at 2012-11-02T08:38:03Z

Is it really necessary to write a test for a simple setter ? I'm not opposed to testing, just looking for a justification. And should I create a new test case or is there one ?

---------------------------------------------------------------------------

by henrikbjorn at 2012-11-02T08:42:49Z

Since your code had a clear syntax error i think that a test is in order ;)

---------------------------------------------------------------------------

by ltouroumov at 2012-11-02T08:45:03Z

Can't argue with that !

Would putting it in `Symfony\Component\HttpKernel\Tests\EventListener\ControllerResponseTest` do the trick ?

---------------------------------------------------------------------------

by henrikbjorn at 2012-11-02T09:16:55Z

Sounds fair enough.

---------------------------------------------------------------------------

by ltouroumov at 2012-11-02T09:31:09Z

This tests covers my use case(s), but there might be others I didn't think of.

---------------------------------------------------------------------------

by ludekstepan at 2012-11-20T17:15:45Z

+1
Please merge into master, I have a use case for it, now I'm relying on a Reflection to set the private property, but that's really a dirty solution. The setter would be perfect!
tweaked previous merge
Try to make sure cookies get deleted from the TokenProvider when no longer in use
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.
Xliff with other node than source or target are ignored
merged branch xkobal/2.1 (PR #6079)

This PR was squashed before being merged into the 2.1 branch (closes #6079).

Commits
-------

bbeff54 Xliff with other node than source or target are ignored

Discussion
----------

Xliff with other node than source or target are ignored

Referring to the xliff XSD, the format can allow other nodes like <note>. Check only count is dangerous if others nodes are present, and we aren't sure that nodes are the two we wan't (source and target)

And the real problem if that if there is other node in translation, the translation is silently ignored.

---------------------------------------------------------------------------

by stloyd at 2012-11-20T17:44:47Z

You should add test that covers this, as well as #6078 should be closed (branch `2.1` is merged from time to time to `master`).

---------------------------------------------------------------------------

by xkobal at 2012-11-21T11:13:32Z

I have added a new node to xliff fixtures and edit test to be sure node isn't ignored.
Add Route::hasOption() and Route::hasRequirement() methods.
merged branch Crell/route-requirements (PR #6098)

This PR was merged into the master branch.

Commits
-------

b930066 Add Route::hasOption() and Route::hasRequirement() methods.

Discussion
----------

Add Route::hasOption() and Route::hasRequirement() methods.

It seems odd that there's a hasDefault() method but not for options and requirements.  Drupal has logic that depends on the existence of a given requirement.  So let's add that.

I think I got the coding standards right the first time for once... :-)
merged branch Tobion/patch-8 (PR #6071)

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.
fixed CS