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`](6ffea1dc1c) 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!
This PR was merged into the master branch.
Commits
-------
e39b709 [Routing] initialize the Route properties
Discussion
----------
[Routing] initialize the Route properties
They should normally be initialized anyway in the constructor. But when extending the Route (like in CMF) and using an ORM/ODM to persist them in the DB, the constructor is not called. Then a new property that is not saved like hostnamePattern stays null which in turn makes the RouteCompiler fails as it expects '' instead of null.
This PR was squashed before being merged into the master branch (closes#6063).
Commits
-------
d3d8ff4 [travis-ci] Zend Garbage Collection only for PHP5.4
Discussion
----------
[travis-ci] Zend Garbage Collection only for PHP5.4
They should normally be initialized anyway in the constructor. But when extending the Route (like in CMF) and using an ORM/ODM to persist them in the DB, the constructor is not called. Then a new property that is not saved like hostnamePattern stays null which in turn makes the RouteCompiler fails as it expects '' instead of null.
This PR was squashed before being merged into the master branch (closes#5888).
Commits
-------
2379d86 CS Fixes - Replaced "array of type" by "Type[]" in PHPDoc block
Discussion
----------
CS Fixes - Replaced "array of type" by "Type[]" in PHPDoc block
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: no (but tests doesn't pass on master too). See Travis.
License of the code: MIT
Documentation PR: Not Applicable
Status: Finished
To improve support of the eclipse PDT pluggin (for autocompletion), I propose to change the array notation in PHPDoc blocks to match the phpDocumentor notation for "array of type".
Modifications are made for the following components:
- BrowserKit
- ClassLoader
- Config
- Console
- CssSelector
- DependencyInjection
- DomCrawler
- EventDispatcher (no changes)
- Filesystem (no changes)
- Finder
- Form
- HttpFoundation
- HttpKernel
- Locale
- OptionResolver (no changes)
- Process (no changes)
- Routing (no changes)
- Serializer (no changes)
- Templating
- Translation
- Validator
- Yaml (no changes)
- Security
- Stopwatch (no changes)
See Proposal https://github.com/symfony/symfony/pull/5852
---------------------------------------------------------------------------
by pborreli at 2012-11-01T15:19:27Z
will you make a PR for each component ? why not only one PR with one commit for each component instead ?
---------------------------------------------------------------------------
by raziel057 at 2012-11-01T15:32:39Z
Ok, I'm going try to do it.
---------------------------------------------------------------------------
by raziel057 at 2012-11-01T16:12:56Z
I would like to rename my branch from COMPONENT_Form to changes-phpdoc (as all modifications would be commited in only one branch), so I tried to execute the following command but I have an error.
git remote rename COMPONENT_Form changes-phpdoc
error: Could not rename config section 'remote.COMPONENT_Form' to 'remote.changes-phpdoc'
Do you know how to do it?
---------------------------------------------------------------------------
by pborreli at 2012-11-01T16:14:26Z
don't rename it, you will have to close and make another PR which is useless here, just edit the title.
---------------------------------------------------------------------------
by stof at 2012-11-01T16:16:17Z
and ``git remote rename`` is about renaming a remote repo, not a branch
---------------------------------------------------------------------------
by raziel057 at 2012-11-03T11:36:02Z
Is it normal that all my commit are duplicated? I would like just update my master and merge with my branch.
---------------------------------------------------------------------------
by fabpot at 2012-11-06T10:22:55Z
@raziel057 Can you rebase on master? That should fix your problem.
---------------------------------------------------------------------------
by fabpot at 2012-11-09T13:28:53Z
@raziel057 Can you finish this PR?
---------------------------------------------------------------------------
by Tobion at 2012-11-09T13:34:45Z
I'll do it for the routing component this evening because I know it by heart. ^^
---------------------------------------------------------------------------
by raziel057 at 2012-11-09T15:06:26Z
@Tobion ok Thanks!
@fabpot Yes, I will try to finish it this week end.
---------------------------------------------------------------------------
by raziel057 at 2012-11-11T13:04:07Z
@Tobion Did you already change PHPDoc in the Routing component?
---------------------------------------------------------------------------
by Tobion at 2012-11-11T15:21:18Z
@raziel057 Yes I'm working on it.
---------------------------------------------------------------------------
by Tobion at 2012-11-12T15:16:31Z
@raziel057 Done. See #5994
This PR was merged into the master branch.
Commits
-------
824a0f3 [Routing] compatibility with older PCRE (pre 8)
Discussion
----------
[Routing] compatibility with older PCRE (pre 8)
#6062 for master
This PR was merged into the 2.1 branch.
Commits
-------
1daefa5 [Routing] made it compatible with older PCRE version (pre 8)
Discussion
----------
[Routing] compatibility with older PCRE version (pre 8)
fixes#4093
Ok I changed my mind about this issue.
1. I figured more people are affected than I thought and CentOS is stubborn.
2. Symfony still uses the old regex style `?P<param>` in several other components. So also doing so in the routing makes it more consistent.
3. Even if it's definitely not good to use an over 6 year old PCRE version with a recent PHP version, we can still try to provide the best experience. It doesn't mean we support outdated software stacks of custom PHP compilations as we won't and cannot specifically test against it.
@fabpot: I will do a seperate PR on master when you merged this because the code changed alot in master so it cannot easily be merged I guess. I will also convert the symfony requirement for PCRE in the requirements check to a recommendation.
This PR was merged into the master branch.
Commits
-------
acbb393 Renamed variable for consistency
Discussion
----------
[SecurityBundle] Renamed variable for consistency
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
License of the code: MIT
Documentation PR: -
This PR was merged into the 2.1 branch.
Commits
-------
ea2bb09 tiny refactoring for consistency
Discussion
----------
tiny refactoring for consistency
no need to use the iterator within the class. not done anywhere else.
This PR was merged into the master branch.
Commits
-------
966e7d6 [DI] removed unneeded is_object() calls
Discussion
----------
[DI] removed unneeded is_object() calls
I searched through all of symfony for occurences of the coding style `(is_object($value) && $value instanceof Object)` with a regex like `is_object\(\$[a-zA-z0-9]+\) && \$[a-zA-z0-9]+ instanceof `.
The `is_object` calls are not needed in this case. Only the DI component made such duplicate checks.
This PR was merged into the master branch.
Commits
-------
acf8a70 [Routing] fix Route recompilation when hostname changed
Discussion
----------
[Routing] fix Route recompilation when hostname changed
This PR was squashed before being merged into the master branch (closes#6030).
Commits
-------
749dac1 Improve docBlock
Discussion
----------
Improve docBlock
This is just a minor change documenting the return type of `SerializerInterface::deserialize()`.
This PR was merged into the master branch.
Commits
-------
644de74 Fix docblock in Doctrine Bridge
Discussion
----------
Fix docblocks in Doctrine Bridge
This PR was merged into the master branch.
Commits
-------
a146156 Merge pull request #2 from Tobion/patch-2
38802ea remove logic that could not be triggered anyway
f7ea68f [Routing] Fixed undefined variable + typo
Discussion
----------
[Routing] Fixed typo + removed dead code
---------------------------------------------------------------------------
by Tobion at 2012-11-17T16:00:04Z
@pborreli: pborreli/symfony#2
---------------------------------------------------------------------------
by pborreli at 2012-11-17T16:02:08Z
@Tobion totally agree, tried to setup a phpunit test which could trigger this exception but couldn't ..
This PR was merged into the master branch.
Commits
-------
83b37ff [DependencyInjection] Return self for add...
Discussion
----------
[DependencyInjection] Return self for add...
Bug fix: no
Forget fix: yes
Feature addition: no
Symfony2 tests pass: yes
License of the code: MIT
Return self instance when call an ADD something method.
---------------------------------------------------------------------------
by pborreli at 2012-11-16T13:24:45Z
Please fix PHPDoc accordingly
---------------------------------------------------------------------------
by ruian at 2012-11-16T13:38:41Z
@pborreli done.
This PR was merged into the master branch.
Commits
-------
97f6a1b [Form] Update password type trimming to false
Discussion
----------
[Form] Update password trimming to false by default
Bug fix: yes
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: ~
Todo: -
License of the code: MIT
Documentation PR: ~
Hey!
Today, I realize that the password type is by default trimmed. IMHO, this is not the expected behavior. By default, the password type should not trim the input value.
Regards
---------------------------------------------------------------------------
by nomack84 at 2012-11-13T19:16:29Z
👍
---------------------------------------------------------------------------
by mvrhov at 2012-11-13T19:57:29Z
IMHO password and username fields should be trimmed. whitespace at the beginning and at the end of those fields are not wanted. At least I don't want to deal with a user support where WS on those fields is not trimmed.
---------------------------------------------------------------------------
by egeloen at 2012-11-13T20:08:08Z
@mvrhov I agree with you about username fields and other "text" fields but in case of a password field, if the end user specifies white space at the begin/end of his password, it should not be trimmed. It should simply let it as it is. I open this PR due to two customers who reports me this behavior.
---------------------------------------------------------------------------
by clemherreman at 2012-11-14T10:06:15Z
@mvrhov I agree, username shouldn't be trimmed, however password are kind of special. They should be used *"as is"*, as lots of users have wicked passwords.
Moreover, usually the password is asked twice, so if there are spaces, they are most likely wanted by the end user.
So 👍
---------------------------------------------------------------------------
by clemherreman at 2012-11-14T10:07:27Z
Also Travis status on this PR is **failed** because of an error when downloading the deps.
---------------------------------------------------------------------------
by geoffrey-brier at 2012-11-14T10:34:56Z
👍
---------------------------------------------------------------------------
by bschussek at 2012-11-14T15:01:43Z
Could you please add a test case to PasswordTypeTest?
Please also reference this PR in the test
(= add the comment `// https://github.com/symfony/symfony/pull/6007` before the test)
---------------------------------------------------------------------------
by egeloen at 2012-11-14T15:10:36Z
@bschussek I have updated the PR.
---------------------------------------------------------------------------
by bschussek at 2012-11-14T15:24:34Z
Thanks! Could you please squash the commits?
---------------------------------------------------------------------------
by egeloen at 2012-11-14T15:30:11Z
@bschussek Done.
---------------------------------------------------------------------------
by stloyd at 2012-11-14T15:39:47Z
Should this be noted in `UPGRADE` file ? (as this is change of actually BC break =))
---------------------------------------------------------------------------
by egeloen at 2012-11-15T22:59:45Z
@stloyd Where can I put it? In the [UPGRADE-2.2](https://github.com/symfony/symfony/blob/master/UPGRADE-2.2.md) file?
---------------------------------------------------------------------------
by stloyd at 2012-11-15T23:02:51Z
@egeloen IMO yes, according this will go to `master` (which is actual _dev_ branch for `2.2`)
---------------------------------------------------------------------------
by egeloen at 2012-11-16T13:54:04Z
@fabpot I have removed the comment & added an entry in the `UPGRADE-2.2` file.
Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
License of the code: MIT
Return self instance when call an ADD something method.
This PR was squashed before being merged into the master branch (closes#6018).
Commits
-------
6334343 [Routing] removed getHostnamePattern from RouteCollection
Discussion
----------
[Routing] removed getHostnamePattern from RouteCollection
this method is not useful and can introduce inconsistencies when a sub-route has a different hostname, which already has a getter for the hostname
This PR was submitted for the master branch but it was merged into the 2.1 branch instead (closes#6016).
Commits
-------
9b0cad4 Added HttpCache\Store::generateContentDigest() + changed visibility
Discussion
----------
Added HttpCache\Store::generateContentDigest() + changed $root visibility
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
License of the code: MIT
Documentation PR: -
## Description
This PR adds 2 small changes in base `HttpCache\Store`:
1. Adds `generateContentDigest()` protected method, allowing to easily override the default behavior for special cases.
2. Change `$root` visibility to *protected* to let descendant access it.
This allows special implementations of HttpCache when storing specific cache files in a different place is needed.
## Example
An example of implementation can be found [in eZ Publish 5 source code](https://github.com/ezsystems/ezp-next/blob/master/eZ/Publish/Core/MVC/Symfony/Cache/Http/LocationAwareStore.php). In this example, eZ Publish content cache entities are stored under a different folder than regular cache entities in order to ease cache purge based on a content location.
## Testing
No unit tests were added since the behavior didn't change and the new method is protected.
PS : It would be nice to have this in 2.1 branch as well 😃
This PR was merged into the master branch.
Commits
-------
f4b630d [HttpFoundation] fix#6002
Discussion
----------
[HttpFoundation] fix#6002
---------------------------------------------------------------------------
by fabpot at 2012-11-13T17:02:45Z
Can you add a test?
---------------------------------------------------------------------------
by Tobion at 2012-11-13T17:04:23Z
hehe see #6004 there is also a test
---------------------------------------------------------------------------
by vicb at 2012-11-13T17:05:06Z
There is a test in the original PR, no need for 6004.
---------------------------------------------------------------------------
by vicb at 2012-11-13T17:05:20Z
Which is why is was failing btw
---------------------------------------------------------------------------
by Tobion at 2012-11-13T17:06:36Z
The test in 6002 did not fail for me without your patch.
---------------------------------------------------------------------------
by fabpot at 2012-11-14T12:47:46Z
@Tobion @vicb What do we do? Just revert #6002 or merge this PR?
---------------------------------------------------------------------------
by vicb at 2012-11-14T13:25:51Z
Merge. Go go go :)
----- Reply message -----
De : "Fabien Potencier" <notifications@github.com>
Pour : "symfony/symfony" <symfony@noreply.github.com>
Cc : "Victor Berchet" <victor@suumit.com>
Objet : [symfony] [HttpFoundation] fix#6002 (#6003)
Date : mer., nov. 14, 2012 13:47
@Tobion @vicb What do we do? Just revert #6002 or merge this PR?
—
Reply to this email directly or view it on GitHub.
---------------------------------------------------------------------------
by Tobion at 2012-11-14T13:31:22Z
@vicb can you explain what it fixes? As I said, your test does not cover something that would fail without the patch. So I don't see the bug.
---------------------------------------------------------------------------
by vicb at 2012-11-14T15:30:55Z
@Tobion php.net states: The `current()` function simply returns the value of the array element that's currently being pointed to by the internal pointer and `reset()` returns the value of the first array element.
I have no clue what the "element that's currently being pointed to by the internal pointer" in this method so `reset()` is probably what you want.
Validated ?
---------------------------------------------------------------------------
by Tobion at 2012-11-14T16:12:03Z
I agree `reset()` is more explicit here. But `current()` should work just as well in this case because the array pointer can only be at the first item when calling the method. Anyway, this is good to merge. I just hoped there was a unit test that ensures this on my machine. This is why I added the test in my patch. Maybe Fabien can just merge the second commit on #6004
---------------------------------------------------------------------------
by vicb at 2012-11-14T16:20:57Z
As explained in #6004, there is already a test from my first PR that made Travis go red.
