Permalink
Commits on Mar 6, 2012
  1. merged branch jmikola/logout-url-helper-request-dep (PR #3511)

    fabpot committed Mar 6, 2012
    Commits
    -------
    
    8796276 [SecurityBundle] Avoid direct request dependency in LogoutUrlHelper
    
    Discussion
    ----------
    
    [SecurityBundle] Avoid direct request dependency in LogoutUrlHelper
    
    This quickly addresses the problem when the helper is constructed in a console environment without request scope. Ideally, the helper should be able to construct the absolute logout URL using data already available in the UrlGenerator's RequestContext and the $_SERVER environment variable; however, that will require copying some code from the Request class to create a base URI and path.
    
    Fixes #3508
    
    [![Build Status](https://secure.travis-ci.org/jmikola/symfony.png?branch=master)](http://travis-ci.org/jmikola/symfony)
  2. [SecurityBundle] Avoid direct request dependency in LogoutUrlHelper

    jmikola committed Mar 6, 2012
    This quickly addresses the problem when the helper is constructed in a console environment without request scope. Ideally, the helper should be able to construct the absolute logout URL using data already available in the UrlGenerator's RequestContext and the $_SERVER environment variable; however, that will require copying some code from the Request class to create a base URI and path.
    
    Fixes #3508
  3. merged branch fixe/patch-1 (PR #3510)

    fabpot committed Mar 6, 2012
    Commits
    -------
    
    85fd9f3 This should be 3 not 4, otherwiser I get the following error:
    
    Discussion
    ----------
    
    Error in logout success handler
    
    I'm getting the following error:
    
    OutOfBoundsException: The index "4" is not in the range [0, 3].
    
    ---------------------------------------------------------------------------
    
    by jmikola at 2012-03-06T06:48:08Z
    
    Thanks for catching this. My mistake in b1f545b. SecurityBundle's functional tests for StandardFormLogin and CsrfFormLogin, which I added, don't test the success handler option.
    
    @fabpot: Looks good to merge.
  4. This should be 3 not 4, otherwiser I get the following error:

    fixe committed Mar 6, 2012
    
    OutOfBoundsException: The index "4" is not in the range [0, 3].
Commits on Mar 5, 2012
  1. merged branch jmikola/patch-1 (PR #3507)

    fabpot committed Mar 5, 2012
    Commits
    -------
    
    654beee [Security] Document CSRF protection for LogoutListener
    
    Discussion
    ----------
    
    [Security] Document CSRF protection for LogoutListener
    
    ---------------------------------------------------------------------------
    
    by Seldaek at 2012-03-05T18:01:36Z
    
    I haven't checked, but for such things I find it way easier to find them in cookbooks than in the changelog - if you don't mind reformatting/copy that in a docs PR it'd be great.
  2. merged branch rdohms/patch-2 (PR #3484)

    fabpot committed Mar 5, 2012
    Commits
    -------
    
    b73c703 Reverting return type left by mistake
    881d290 Updating use of DoctrineBundle Registry to use the proper path to Doctrine\Bundle\DoctrineBundle\Registry
    
    Discussion
    ----------
    
    Updating use of DoctrineBundle Registry to use the proper path
    
    Pointed to the new class: Doctrine\Bundle\DoctrineBundle\Registry
    
    ---------------------------------------------------------------------------
    
    by adrienbrault at 2012-03-01T22:12:42Z
    
    I think the return type should stay ```Registry```
    
    ---------------------------------------------------------------------------
    
    by rdohms at 2012-03-01T22:48:35Z
    
    Yes, that was a mistake, reverted.
  3. merged branch jeremyFreeAgent/propel_dataCollector_time (PR #3506)

    fabpot committed Mar 5, 2012
    Commits
    -------
    
    eb759c5 [Propel1] Fixed data collector
    
    Discussion
    ----------
    
    [Propel1] Fixed data collector
    
    ---------------------------------------------------------------------------
    
    by jeremyFreeAgent at 2012-03-05T16:25:58Z
    
    Sorry for the two previous pull requests :(
  4. merged branch jmikola/logout-csrf (PR #3007)

    fabpot committed Mar 5, 2012
    Commits
    -------
    
    49a8654 [Security] Use LogoutException for invalid CSRF token in LogoutListener
    a96105e [SecurityBundle] Use assertCount() in tests
    4837407 [SecurityBundle] Fix execution of functional tests with different names
    66722b3 [SecurityBundle] Templating helpers to generate logout URL's with CSRF tokens
    aaaa040 [Security] Allow LogoutListener to validate CSRF tokens
    b1f545b [Security] Refactor LogoutListener constructor to take options
    c48c775 [SecurityBundle] Add functional test for form login with CSRF token
    
    Discussion
    ----------
    
    [Security] Implement support for CSRF tokens in logout URL's
    
    ```
    Bug fix: no
    Feature addition: yes
    Backwards compatibility break: no
    Symfony2 tests pass: yes
    Fixes the following tickets: -
    Todo: -
    ```
    
    [![Build Status](https://secure.travis-ci.org/jmikola/symfony.png?branch=logout-csrf)](http://travis-ci.org/jmikola/symfony)
    
    This derived from #3006 but properly targeting on the master branch.
    
    This exposes new configuration options to the logout listener to enable CSRF protection, as already exists for the form login listener. The individual commits and their extended messages should suffice for explaining the logical changes of the PR.
    
    In addition to changing LogoutListener, I also created a templating helper to generate logout URL's, which includes a CSRF token if necessary. This may or may not using routing, depending on how the listener is configured since both route names or hard-coded paths are valid options.
    
    Additionally, I added unit tests for LogoutListener and functional tests for both CSRF-enabled form logins and the new logout listener work.
    
    Kudo's to @henrikbjorn for taking the time to document CSRF validation for form login listeners (see [here](http://henrik.bjrnskov.dk/symfony2-cross-site-request-forgery/)). The [Logout CSRF Protection](http://www.yiiframework.com/wiki/190/logout-csrf-protection/) article on the Yii Framework wiki was also helpful in drafting this.
    
    ---------------------------------------------------------------------------
    
    by jmikola at 2011-12-31T07:50:31Z
    
    Odd that Travis CI reported a build failure for PHP 5.3.2, but both 5.3 and 5.4 passed: http://travis-ci.org/#!/jmikola/symfony/builds/463356
    
    My local machine passes as well.
    
    ---------------------------------------------------------------------------
    
    by jmikola at 2012-02-06T20:05:30Z
    
    @schmittjoh: Please let me know your thoughts on the last commit. I think it would be overkill to add support for another handler service and/or error page just for logout exceptions.
    
    Perhaps as an alternative, we might just want to consider an invalid CSRF token on logout imply a false return value for `LogoutListener::requiresLogout()`. That would sacrifice the ability to handle the error separately (which a 403 response allows us), although we could still add logging (currently done in ExceptionListener).
    
    ---------------------------------------------------------------------------
    
    by jmikola at 2012-02-13T17:41:33Z
    
    @schmittjoh: ping
    
    ---------------------------------------------------------------------------
    
    by fabpot at 2012-02-14T23:36:22Z
    
    @jmikola: Instead of merging symfony/master, can you rebase?
    
    ---------------------------------------------------------------------------
    
    by jmikola at 2012-02-15T00:00:49Z
    
    Will do.
    
    ---------------------------------------------------------------------------
    
    by jmikola at 2012-02-15T00:05:48Z
    
    ```
    [avocado: symfony] logout-csrf (+9/-216) $ git rebase master
    First, rewinding head to replay your work on top of it...
    Applying: [SecurityBundle] Add functional test for form login with CSRF token
    Applying: [Security] Refactor LogoutListener constructor to take options
    Applying: [Security] Allow LogoutListener to validate CSRF tokens
    Applying: [SecurityBundle] Templating helpers to generate logout URL's with CSRF tokens
    Applying: [SecurityBundle] Fix execution of functional tests with different names
    Applying: [SecurityBundle] Use assertCount() in tests
    Using index info to reconstruct a base tree...
    Falling back to patching base and 3-way merge...
    Applying: [Security] Use LogoutException for invalid CSRF token in LogoutListener
    
    [avocado: symfony] logout-csrf (+7) $ git st
    # On branch logout-csrf
    # Your branch and 'origin/logout-csrf' have diverged,
    # and have 223 and 9 different commit(s) each, respectively.
    #
    nothing to commit (working directory clean)
    
    [avocado: symfony] logout-csrf (+7) $
    ```
    
    After rebasing, my merge commits disappeared. Is this normal?
    
    ---------------------------------------------------------------------------
    
    by stof at 2012-02-15T00:15:07Z
    
    Are you sure they disappeared ? Diverging from the remote branch is logical (you rewrote the history and so changed the commit id) but are you sure it does not have the commits on top of master ? Try ``git log master..logout-scrf``
    
    If your commut are there, you simply need to force the push for the logout-csrf branch (take care to push only this branch during the force push to avoid messing all others as git won't warn you when asking to force)
    
    ---------------------------------------------------------------------------
    
    by stof at 2012-02-15T00:17:09Z
    
    ah sorry, you talked only about the merge commit. Yeah it is normal. When reapplying your commits on top of master, the merge commit are not kept as you are reapplying the changes linearly on top of the other branch (and deleting the merge commit was the reason why @fabpot asked you to rebase instead of merging btw)
    
    ---------------------------------------------------------------------------
    
    by jmikola at 2012-02-15T00:18:00Z
    
    The merge commits are not present in `git log master..logout-csrf`. Perhaps it used those merge commits when rebasing, as there were definitely conflicts resolved when I originally merged in symfony/master (@fabpot had made his own changes to LogoutListener).
    
    I'll force-push the changes to my PR brange. IIRC, GitHub is smart enough to preserve inline diff comments, provided they were made through the PR and not on the original commits.
    
    ---------------------------------------------------------------------------
    
    by jmikola at 2012-02-15T00:19:38Z
    
    That worked well. In the future, I think I'll stick to merging upstream in and then rebasing afterwards. Resolving conflicts is much easier during a merge than interactive rebase.
    
    ---------------------------------------------------------------------------
    
    by jmikola at 2012-02-23T18:46:13Z
    
    @fabpot @schmittjoh: Is there anything else I can do for this PR? I believe the exception was the only outstanding question (see: [this comment](symfony#3007 (comment))).
  5. merged branch pulzarraider/memcache_profiler_settings_change (PR #3499)

    fabpot committed Mar 5, 2012
    Commits
    -------
    
    100d59b Modified Memcache(d) dsn to be more intuitive. Chnged Exception texts in other storages.
    
    Discussion
    ----------
    
    [HttpKernel] Modified Memcache(d)ProfilerStorage dsn to be more intuitive
    
    Bug fix: no
    Feature addition: -
    Backwards compatibility break: no
    Symfony2 tests pass: yes
    Fixes the following tickets: -
    Todo: -
    
    Before:
    
    ```
    #app/config/config_dev.yml
    ...
    framework:
        ...
        profiler:
            ...
            dsn: memcache://127.0.0.1/11211
    ...
    ```
    
    Now:
    
    ```
    #app/config/config_dev.yml
    ...
    framework:
        ...
        profiler:
            ...
            dsn: memcache://127.0.0.1:11211
    ...
    ```
    
    If Memcache host is IPv6 address:
    
    ```
    #app/config/config_dev.yml
    ...
    framework:
        ...
        profiler:
            ...
            dsn: memcache://[::1]:11211
    ...
    ```
    
    I changed texts of some exceptions to be more consistent, too.
  6. [Process] fixed CS

    fabpot committed Mar 5, 2012
  7. merged branch Seldaek/processb (PR #3381)

    fabpot committed Mar 5, 2012
    Commits
    -------
    
    7444fdf Feedback fixes
    54cfd44 Restore bypass_shell by default with windows compat
    38df47a Fix env inheritance and added tests
    f555c62 [Process] Add windows compatibility to Process component
    c4e8ff7 [Process] Always escape commands properly and remove windows-specific handling
    9e237f6 [Process] Add ProcessBuilder::create() for more fluidity in the interface until 5.4
    4882777 [Process] Code clean up
    
    Discussion
    ----------
    
    ProcessBuilder clean up
    
    - Code cleanup
    - Added create() static method for easy creation until we can do `$process = (new ProcessBuilder())->add()->getProcess();`
    - Removed windows wrapping of commands. This does not belong there IMO. If assetic needs that it should add it, and if it's generally beneficial to everyone then we should add it to Process, but having it implicitly only when using ProcessBuilder makes on sense.
    
    ---------------------------------------------------------------------------
    
    by beberlei at 2012-02-16T16:10:15Z
    
    I agree on the windows stuff. I know it fixes a bunch of issues in Assetic, but it also caused my tons of headaches in my windows commands that didnt need strict escaping. Also this messes with parameters in Powershell for example, when you have "foo /bar:baz" then it makes this to ""foo" "/bar:baz"" which in some circumstances fails. Its all messy.
    
    ---------------------------------------------------------------------------
    
    by schmittjoh at 2012-02-16T17:53:30Z
    
    Can you move the wrapping to the Process class instead? It's generally causing no bad side effects, but fixes a few issues in the proc_open implementation. It is also necessary for Assetic, and potentially other tools to work on Windows.
    
    ---------------------------------------------------------------------------
    
    by Seldaek at 2012-02-16T17:56:02Z
    
    Sure, although "generally" sounds a bit scary in your sentence :)
    
    What about the bypass_shell option?
    
    ---------------------------------------------------------------------------
    
    by schmittjoh at 2012-02-16T18:02:12Z
    
    "generally" means I don't know of any, but what I do know is that the alternative you are suggesting is not working. Have there been any bug reports on Assetic/symfony/your own code that "cmd" wrapping causes problems?
    
    ---------------------------------------------------------------------------
    
    by Seldaek at 2012-02-16T18:04:59Z
    
    No no, don't get me wrong, I'm not suggesting this should be removed. I'm just saying it should be done for all processes or none, but not just for those run via the ProcessBuilder because that's a good recipe for WTFs.
    
    ---------------------------------------------------------------------------
    
    by schmittjoh at 2012-02-16T18:09:38Z
    
    Yeah, I understand, and it makes sense.
    
    What I would suggest is to move it to the process class, and let a wider audience test this to see if we get any bug reports on strange behavior etc.
    
    ---------------------------------------------------------------------------
    
    by Seldaek at 2012-02-16T18:12:00Z
    
    Still not sure about the bypass_shell option though. And @beberlei mentioned problems? Can you expand on that?
    
    ---------------------------------------------------------------------------
    
    by Seldaek at 2012-02-16T18:16:34Z
    
    Added back to Process, with a switch so if anyone runs into problems they can easily disable it.
    
    ---------------------------------------------------------------------------
    
    by Seldaek at 2012-02-22T10:59:58Z
    
    Ping @fabpot - I think this is ready now
    Ping @kriswallsmith if this gets merged please update Assetic stuff to restore the bypass_shell option if it's really needed.
    
    ---------------------------------------------------------------------------
    
    by kriswallsmith at 2012-02-22T12:41:15Z
    
    Posting a PR under "code cleanup" that tinkers with a class that is inherently difficult to test for regression and has been tested by the community for over a year is… a bit hard to swallow, honestly. Everything is there for a reason and should not be tinkered with lightly.
    
    For example, it's important that the `$env` variable default to `null` so the current environment is inherited by default — why change that?
    
    I don't know what the `bypass_shell` option does, but @pierrejoye does… which is why he put it there.
    
    I'm okay with adding an "enhanced Windows compatibility" switch, but I personally think is should be on the builder, not `Process`. The builder is where we manipulate the strings that compose the command line, not in `Process`. You're introducing manipulation of the command line to `Process`, which blurs the responsibilities of the two classes.
    
    I'm also okay with the static factory method :)
    
    ---------------------------------------------------------------------------
    
    by Seldaek at 2012-02-22T13:19:40Z
    
    @kriswallsmith (Sorry about the confusing title) My concern is just that if you use Process then decide to "upgrade" to the ProcessBuilder, you suddenly have a change of behavior that might break stuff without you noticing. I just want to avoid this unexpected behavior.
    
    As for the $env stuff, I added a couple tests now, and then expanded that ternary operator a bit.. It actually was broken before. It passed null if you had no env set, but even if you did not call `inheritEnvironmentVariables`. If you want to inherit by default - which I agree it should - then why was `inheritEnv = false` in the constructor? I changed it too and now there is hopefully less confusion.
    
    Restored bypass_shell=true unless it's explicitly set to false.
    
    ---------------------------------------------------------------------------
    
    by kriswallsmith at 2012-02-22T13:25:23Z
    
    We should also add the PHPUnit `@backupGlobals enabled` annotation while we're in here.
    
    ---------------------------------------------------------------------------
    
    by kriswallsmith at 2012-02-22T13:31:41Z
    
    @Seldaek Looks better, thanks for the changes. If `enhanceWindowsCompatibility` is going to live on `Process` we should expose the switch on the builder as well. Speaking of `enhanceWindowsCompatibility`… is there a more descriptive name for that? What exactly does that do, any why would anyone want to switch it off? The name is so vague we might as well call it `enableMagicalWindowsFix()`.
    
    ---------------------------------------------------------------------------
    
    by pierrejoye at 2012-02-22T13:33:55Z
    
    I really do not think that having a flag to enable portability is a
    good idea, at all.
    
    I do not remember the context right now but a flag is definitively a
    bad idea (you will need other on other platforms).
    
    I will take a look again at this next week (end of), as I am still OOF.
    
    On Wed, Feb 22, 2012 at 2:31 PM, Kris Wallsmith
    <reply@reply.github.com>
    wrote:
    > @Seldaek Looks better, thanks for the changes. If `enhanceWindowsCompatibility` is going to live on `Process` we should expose the switch on the builder as well. Speaking of `enhanceWindowsCompatibility`… is there a more descriptive name for that? What exactly does that do, any why would anyone want to switch it off? The name is so vague we might as well call it `enableMagicalWindowsFix()`.
    >
    > ---
    > Reply to this email directly or view it on GitHub:
    > symfony#3381 (comment)
    
    --
    Pierre
    
    @pierrejoye | http://blog.thepimp.net | http://www.libgd.org
    
    ---------------------------------------------------------------------------
    
    by Seldaek at 2012-02-22T13:42:56Z
    
    backupGlobals seems to be enabled by default.
    
    As for the enhanceWindowsCompatibility, yes. It's a poor name, but no I don't have any idea for a better one, because nobody could explain me what it does. People just scream that it's necessary.
    
    @pierrejoye: If you or anyone can conclusively confirm that this stuff is always better, then we always do it. If it's not then it must be optional, and if it's not a flag then what? The point of the component is to abstract the proc_open horrors. If people have to know about windows quirks with regard to proc_open to use it, then it's not a very useful abstraction.
    
    Additionally, if it *is* always better to use those portability fixes, then why isn't php doing it itself?
    
    ---------------------------------------------------------------------------
    
    by pierrejoye at 2012-02-22T13:47:02Z
    
    On Wed, Feb 22, 2012 at 2:42 PM, Jordi Boggiano
    <reply@reply.github.com>
    wrote:
    > backupGlobals seems to be enabled by default.
    >
    > As for the enhanceWindowsCompatibility, yes. It's a poor name, but no I don't have any idea for a better one, because nobody could explain me what it does. People just scream that it's necessary.
    
    > @pierrejoye: If you or anyone can conclusively confirm that this stuff is always better, then we always do it. If it's not then it must be optional, and if it's not a flag then what? The point of the component is to abstract the proc_open horrors. If people have to know about windows quirks with regard to proc_open to use it, then it's not a very useful abstraction.
    
    proc_open has many quirks, not only on windows. That's why it should
    work and detect what is needed, that may force you to slightly change
    the split between builder and process.
    
    > Additionally, if it *is* always better to use those portability fixes, then why isn't php doing it itself?
    
    BC, like it or not (I do not).
    
    However we cannot change past versions, so today code has to deal it
    with it anyway.
    
    I will take a look at what you are trying to fix here next week, if
    you have any other requests regarding proc_open&portability, let me
    know :)
    
    Cheers,
    --
    Pierre
    
    @pierrejoye | http://blog.thepimp.net | http://www.libgd.org
    
    ---------------------------------------------------------------------------
    
    by Seldaek at 2012-02-22T13:54:38Z
    
    Ok so it sounds to me like the current code is correct, it tries to fix
    things as best as we know how to by default, and just gives you a way to
    disable things in the odd case we messed up and some of those fixes are
    harmful to some use cases.
    
    ---------------------------------------------------------------------------
    
    by fabpot at 2012-03-02T21:38:18Z
    
    @Seldaek @kriswallsmith is it ready for merge now?
    
    ---------------------------------------------------------------------------
    
    by kriswallsmith at 2012-03-02T21:42:22Z
    
    I'm still not happy with the name of `enhanceWindowsCompatibility`. We need to be more specific about what that does. It sounds like a marketing term right now ;)
    
    ---------------------------------------------------------------------------
    
    by Seldaek at 2012-03-05T13:44:56Z
    
    Agreed, but I can't think of anything better. It is indeed esoteric magic fixes that should work better but nobody seems 100% sure about it, so I think it's fairly accurate.
Commits on Mar 4, 2012
  1. merged branch jeremyFreeAgent/evo_propelCollectorQueriesTime (PR #3495)

    fabpot committed Mar 4, 2012
    Commits
    -------
    
    26496d0 Added Queries duration in Propel1 Bridge DataCollector
    
    Discussion
    ----------
    
    Added Queries duration in Propel1 Bridge DataCollector
Commits on Mar 3, 2012
  1. [HttpKernel] fixed CS

    fabpot committed Mar 3, 2012
  2. merged branch pulzarraider/redis_profiler_storage (PR #3451)

    fabpot committed Mar 3, 2012
    Commits
    -------
    
    86ebe5b Redis Profiler Storage
    
    Discussion
    ----------
    
    [HttpKernel] Redis Profiler Storage added
    
    Bug fix: no
    Feature addition: yes
    Backwards compatibility break: no
    Symfony2 tests pass: yes
    Fixes the following tickets: -
    Todo: -
    
    Usage:
    
    ```yml
    #config_dev.yml
    framework:
    ...
        profiler:
        ...
            dsn: redis://127.0.0.1:6379
    ```
    
    Redis PHP extension: https://github.com/nicolasff/phpredis
    
    ---------------------------------------------------------------------------
    
    by fabpot at 2012-03-02T20:38:57Z
    
    #3454 has been merged now.
    
    ---------------------------------------------------------------------------
    
    by pulzarraider at 2012-03-02T23:41:12Z
    
    @fabpot Tests updated and passed.
Commits on Mar 2, 2012
  1. Redis Profiler Storage

    pulzarraider committed Feb 26, 2012
    fixed typo and tests
    
    - updated profiler tests
    - added testPurge() method
    - fixed find() method
  2. fixed CS

    fabpot committed Mar 2, 2012
  3. merged branch Toflar/patch-1 (PR #3408)

    fabpot committed Mar 2, 2012
    Commits
    -------
    
    4f8e8ef Improving performance on digit filtering
    
    Discussion
    ----------
    
    Improving performance on digit filtering
    
    I haven't tested it on a productive system but I think it should be way faster to use filter_var() instead of preg_replace() for several reasons.
    
    This is my first pull request for symfony and I don't know how you do those kind of performance tests but please verify my assumption if you can :-)
    
    Maybe we can also use filter_var() to replace other regular expressions :-)
    
    HTH =)
    
    ---------------------------------------------------------------------------
    
    by drak at 2012-02-22T00:35:44Z
    
    @Toflar - nice move +1
    
    ---------------------------------------------------------------------------
    
    by drak at 2012-02-22T18:53:40Z
    
    @Toflar - Maybe you can bench the changes using this as a template: https://gist.github.com/1356129
    
    ---------------------------------------------------------------------------
    
    by Toflar at 2012-02-23T13:18:18Z
    
    I have already. And it's way faster, otherwise I wouldn't have opened a pull request ;) But obviously it strongly depends on the length of the string and the environment. That's why I was wondering whether you have a general performance tests environment ;) Because the results strongly depend on other factors, there's - in my opinion - no point in exact results. If a general info is sufficient: my tests for the regex resulted in about 7 - 8 microseconds whereas the filter version only took 1.5 - 2 microseconds for the same string.
  4. merged branch jonathaningram/patch-6 (PR #3422)

    fabpot committed Mar 2, 2012
    Commits
    -------
    
    88ccb9b Added another corner case
    7c4343f Added 4 assertions related to simple URLs containing ? and #
    
    Discussion
    ----------
    
    [Validator] added assertions related to simple URLs containing ? and #
    
    Bug fix: no
    Feature addition: no
    Backwards compatibility break: no
    Symfony2 tests pass: yes
    
    This adds 5 assertions for corner cases when validating a URL containing `?` and/or `#`.
    
    Note: this does not actually fix any bugs, it just adds a few more cases.
    
    I hope I've sent this to the correct branch - it's not a bug fix so I've not sent it to 2.0.x, but it's not a feature either...
  5. merged branch jmikola/patch-3 (PR #3450)

    fabpot committed Mar 2, 2012
    Commits
    -------
    
    265360d [DoctrineBridge] Simpler result checking in UniqueEntityValidator
    
    Discussion
    ----------
    
    [DoctrineBridge] Simpler result checking in UniqueEntityValidator
    
    In 928e352, support for MongoDB cursors was implemented by converting an Iterable, non-ArrayAccess object to an array. The ArrayAccess check didn't seem purposeful, since cursors are only Iterable and ORM returns real arrays. Since we only need to access the first element of the cursor (and only in cases where the count is exactly 1), we can simply use current() to handle Iterables and arrays.
    
    @henrikbjorn: Any thoughts on this? I was testing @stof's work in doctrine/DoctrineMongoDBBundle#68 and our Symfony submodule was a bit old, so I fixed UniqueEntityValidator on my local machine before I realized you had come up with a solution a few weeks ago.
  6. merged branch snc/profiler-tests (PR #3454)

    fabpot committed Mar 2, 2012
    Commits
    -------
    
    ed8c1c0 Fixed AbstractProfilerStorageTest and some minor CS changes.
    1ac581e Overwrite the profile data if the token already exists like in the other implementations.
    198d406 Return profiler results sorted by time in descending order like in the other implementations.
    9d8e3f2 Refactored profiler storage tests to share some code.
    
    Discussion
    ----------
    
    [WIP] Refactored profiler tests including some storage fixes
    
    Bug fix: yes
    Feature addition: no
    Backwards compatibility break: no
    Symfony2 tests pass: yes
    
    While refactoring the tests I came across some inconsistencies. Two of them are already fixed in this PR.
    
    One thing left is the [MongoDbProfilerStorageTest::testCleanup()](https://github.com/snc/symfony/blob/9d8e3f2da4bea58316e32ef41d4e856a0cfdc872/tests/Symfony/Tests/Component/HttpKernel/Profiler/MongoDbProfilerStorageTest.php#L51) test which fails in all other storage implementations. The mongodb implementation uses the `time` value from the profiler data to clean up the storage while the others additionally save a `created_at` value which is then used. For me this `created_at` value does not make any sense and I would suggest to change the other implementations to use the `time` value for cleaning up. What do you think?
    
    ---------------------------------------------------------------------------
    
    by pulzarraider at 2012-02-27T06:55:06Z
    
    +1 for refactoring profiler tests, I will update my RedisProfilerStorage after your changes will be merged.
    
    ---------------------------------------------------------------------------
    
    by snc at 2012-02-28T20:05:12Z
    
    Any suggestions about the cleanup issue?
  7. merged branch blogsh/router-patch-1 (PR #3483)

    fabpot committed Mar 2, 2012
    Commits
    -------
    
    ba251d8 [Routing] Updated Router::match and Router::generate documentation
    2ce15bd [Routing] Fixed Router::match documentation
    
    Discussion
    ----------
    
    [Routing] Fixed Router::match and Router::generate documentation
    
    Documentation of Router::match has been deprecated/invalid.
    
    ---------------------------------------------------------------------------
    
    by stof at 2012-03-01T17:41:41Z
    
    even better way to fix this: replace it with ``{@inheritdoc}``
    
    ---------------------------------------------------------------------------
    
    by blogsh at 2012-03-01T19:22:06Z
    
    Okay, wasn't sure whether this is appreciated because it inherits the method over 3 corners :)
  8. merged branch vicb/patch-1 (PR #3481)

    fabpot committed Mar 2, 2012
    Commits
    -------
    
    5886c55 [WebProfilerBundle] Router panel: take the request method into account
    
    Discussion
    ----------
    
    [WebProfilerBundle] Router panel: take the request method into account
    
    Before this change we were always using the default method (i.e. 'GET').
Commits on Mar 1, 2012
  1. Updating use of DoctrineBundle Registry to use the proper path to Doc…

    rdohms committed Mar 1, 2012
    …trine\Bundle\DoctrineBundle\Registry
  2. merged branch mvrhov/session_cookie_merge (PR #3423)

    fabpot committed Mar 1, 2012
    Commits
    -------
    
    471b564 auto_start should be false
    6e2a7da Support session cookie options with cookie_ prefix
    e0fba80 Properly merge session cookie_* parameters
    
    Discussion
    ----------
    
    Set session.cookie_* parameters properly
    
    Bug fix: yes
    Feature addition: no
    Backwards compatibility break: yes
    Symfony2 tests pass: yes
    Fixes the following tickets: /
    
    Cookie parameters in $options are not prefixed with cookie_ the same is true for data returned from session_get_cookie_params.
    
    I've marked this as BC because the options that get dumped into the container have different name. But I don't think anybody was actually changing them or accessing them in their bundles.
    
    P.S. @drak also desires some credits for this PR as I incorporated some lines written by him in one of the iterations.
    
    ---------------------------------------------------------------------------
    
    by drak at 2012-02-23T14:24:42Z
    
    @mvrhov - what does this fix exactly? It looks like a different way of doing the same thing but now there is no default value on `cookie_httponly`.
    
    ---------------------------------------------------------------------------
    
    by mvrhov at 2012-02-23T15:09:17Z
    
    Like I said in description. $option contains some cookie options and none of them has cookie_ prefix.
    And this prefix is needed in two cases:
    - to properly merge defaults and override them with what user set
    - in a foreach for for proper ini_set
    
    Sorry non native speaker an a bit hard to explain, could you ping me in a couple of hours on IRC if this still doesn't make any sense.
    
    ---------------------------------------------------------------------------
    
    by drak at 2012-02-23T15:29:41Z
    
    @mvrhov - I wrote some tests for this particular code and I still don't see what this PR fixes. I'll try to catch you on IRC later on but can't guarantee it.
    
    ---------------------------------------------------------------------------
    
    by mvrhov at 2012-02-23T16:02:41Z
    
    added test
    
    ---------------------------------------------------------------------------
    
    by drak at 2012-02-24T08:30:51Z
    
    Just for reference for those reading this ticket, `session_set_cookie_params()` alters the runtime ini settings it corresponds to see http://docs.php.net/manual/en/function.session-set-cookie-params.php so we agreed to remove the special handling that was present since it is redundant.
    
    ---------------------------------------------------------------------------
    
    by dlsniper at 2012-02-28T22:19:32Z
    
    Hi, Is this patch relevant or not after all?
    ping @drak @mvrhov
    
    Thanks :)
    
    ---------------------------------------------------------------------------
    
    by drak at 2012-02-29T03:34:22Z
    
    It is relevant.  Maybe I'll do the cleanup this PR by forking it if @mvrhov doesn't have time.
    
    ---------------------------------------------------------------------------
    
    by mvrhov at 2012-02-29T05:40:47Z
    
    Fixed the typo and changed the false to ture as reported in comments. I've also rebased. I'll see what I can do about config file change later today. Sorry for the delay, been too busy for the past week.
    
    ---------------------------------------------------------------------------
    
    by mvrhov at 2012-02-29T08:49:23Z
    
    I've also done the config part.
    
    ---------------------------------------------------------------------------
    
    by mvrhov at 2012-02-29T11:01:14Z
    
    Ok, this should be it.
    
    ---------------------------------------------------------------------------
    
    by drak at 2012-03-01T00:59:16Z
    
    @fabpot - looks good from my side.
  3. merged branch drak/session_docblocks (PR #3413)

    fabpot committed Mar 1, 2012
    Commits
    -------
    
    09be5cb [HttpFoundation] Documentation.
    7f8c293 [HttpFoudation] Add ability to configure sqlite session storage.
    
    Discussion
    ----------
    
    [HttpFoundation] Session docblocks
    
    Bug fix: no
    Feature addition: no
    Backwards compatibility break: no
    Symfony2 tests pass: yes
    Fixes the following tickets: -
    Todo: -
    
    ---------------------------------------------------------------------------
    
    by drak at 2012-02-22T18:19:45Z
    
    Please note the documentation referred to will be published a php.net this Friday (docs are built weekly).
    
    ---------------------------------------------------------------------------
    
    by drak at 2012-02-24T11:16:57Z
    
    @fabpot - this is ready for merge.
    
    ---------------------------------------------------------------------------
    
    by fabpot at 2012-03-01T00:02:09Z
    
    Can you squash your commits before I merge? Thanks.
    
    ---------------------------------------------------------------------------
    
    by drak at 2012-03-01T00:57:50Z
    
    Done.