Permalink
Commits on Apr 19, 2012
  1. Merge remote branch 'symfony/master'

    Samy Dindane
    Samy Dindane committed Apr 19, 2012
  2. merged branch rouffj/fix-3971 (PR #3998)

    fabpot committed Apr 19, 2012
    Commits
    -------
    
    991076a Fix #3971
    
    Discussion
    ----------
    
    Fix #3971
  3. Fix #3971

    rouffj committed Apr 19, 2012
  4. merged branch michal-pipa/composer-fix (PR #3986)

    fabpot committed Apr 19, 2012
    Commits
    -------
    
    a514072 Added Monolog to composer.json as dev dependency
    
    Discussion
    ----------
    
    Added Monolog to composer.json as dev dependency
    
    Bug fix: yes
    Feature addition: no
    Backwards compatibility break: no
    Symfony2 tests pass: [![Build Status](https://secure.travis-ci.org/michal-pipa/symfony.png?branch=composer-fix)](http://travis-ci.org/michal-pipa/symfony)
    Fixes the following tickets: -
    Todo: -
    
    Monolog was missing in dev composer dependences. It was present in `vendors.php`: https://github.com/symfony/symfony/blob/35e6c8ed0fb1ee329c26868c87e0fb5c54eef06a/vendors.php
    
    Without this code coverage report generation fails with error message: `Fatal error: Class 'Monolog\Handler\ChromePHPHandler' not found`.
  5. merged branch msabramo/patch-1 (PR #3988)

    fabpot committed Apr 19, 2012
    Commits
    -------
    
    1863b28 Fix typo: Resonse -> Response
    
    Discussion
    ----------
    
    Fix typo: Resonse -> Response
  6. merged branch willdurand/patch-1 (PR #3991)

    fabpot committed Apr 19, 2012
    Commits
    -------
    
    e5eed5c Fixed autoload.php.dist
    
    Discussion
    ----------
    
    Fixed autoload.php.dist
Commits on Apr 18, 2012
  1. Fixed autoload.php.dist

    willdurand committed Apr 18, 2012
  2. merged branch vicb/FormTypeValidatorExtension_fix (PR #3984)

    fabpot committed Apr 18, 2012
    Commits
    -------
    
    99ec873 [Form] Fix the FormTypeValidatorExtension (required by PR 3923)
    
    Discussion
    ----------
    
    [Form] Fix the FormTypeValidatorExtension (required by PR 3923)
    
    Thanks to @jrfish for the fix (#3983)
  3. merged branch gajdaw/finder_comparator_not_equal (PR #3974)

    fabpot committed Apr 18, 2012
    Commits
    -------
    
    1b320c8 [Finder][Comparator] not equal operator
    
    Discussion
    ----------
    
    [Finder][Comparator] not equal operator
    
    Bug fix: no
    Feature addition: yes
    Backwards compatibility break: no
    Symfony2 tests pass: yes
    Fixes the following tickets: -
    Todo: -
  4. merged branch bschussek/issue3878 (PR #3923)

    fabpot committed Apr 18, 2012
    Commits
    -------
    
    6e4ed9e [Form] Fixed regression: bind(null) was not converted to an empty string anymore
    fcb2227 [Form] Deprecated FieldType, which has been merged into FormType
    bfa7ef2 [Form] Removed obsolete exceptions
    2a49449 [Form] Simplified CSRF mechanism and removed "csrf" type
    
    Discussion
    ----------
    
    [Form] Merged FieldType into FormType
    
    Bug fix: no
    Feature addition: no
    Backwards compatibility break: yes
    Symfony2 tests pass: yes
    Fixes the following tickets: #3878
    Todo: update the documentation on theming
    
    ![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3878)
    
    This PR is a preparatory PR for #3879. See also #3878.
    
    ---------------------------------------------------------------------------
    
    by juliendidier at 2012-04-13T14:25:19Z
    
    What's the benefit ?
    
    ---------------------------------------------------------------------------
    
    by henrikbjorn at 2012-04-13T14:26:40Z
    
    why `input_widget` ? and not just `widget`
    
    ---------------------------------------------------------------------------
    
    by Burgov at 2012-04-13T14:27:49Z
    
    @juliendidier dynamic inheritance is now obsolete which fixes some other issues
    
    ---------------------------------------------------------------------------
    
    by stloyd at 2012-04-13T14:37:26Z
    
    What about __not__ breaking API so *badly* and leaving `FieldType` which will be simple like (with marking as deprecated):
    
    ```php
    <?php
    
    class FieldType extends AbstractType
    {
        public function getParent(array $options)
        {
            return 'form';
        }
    
        public function getName()
        {
            return 'field';
        }
    }
    
    ---------------------------------------------------------------------------
    
    by bschussek at 2012-04-13T14:43:41Z
    
    @stloyd That's a very good idea.
    
    ---------------------------------------------------------------------------
    
    by mvrhov at 2012-04-13T17:41:21Z
    
    IMHO what @stloyd proposed sounds like a good idea, but removing FieldType class, if #3903 will come into life might ensure that more forms will broke and people will check them thoroughly.
    
    ---------------------------------------------------------------------------
    
    by r1pp3rj4ck at 2012-04-13T18:46:08Z
    
    @bschussek looks great, but I'm concerned about how quickly will the third-party bundles adapt to this BC break. I hope really quick, because if they don't the whole stuff will be useless :S of course it's not your problem to solve.
    
    ---------------------------------------------------------------------------
    
    by stof at 2012-04-13T18:50:32Z
    
    @r1pp3rj4ck there is already another BC break requiring to update custom types for Symfony master. So third party bundles already have to do some work.
    
    ---------------------------------------------------------------------------
    
    by r1pp3rj4ck at 2012-04-13T18:59:37Z
    
    @stof which one? I've looked into @bschussek 's RFC about these [foo].bar stuff, but it's not yet implemented. Are you refering to this or another one I've missed?
    
    ---------------------------------------------------------------------------
    
    by stof at 2012-04-13T19:04:06Z
    
    @r1pp3rj4ck the change regarding default options
    
    ---------------------------------------------------------------------------
    
    by r1pp3rj4ck at 2012-04-13T19:06:10Z
    
    @stof oh, I forgot that one. Weird thing is that I've already changed my default options today and still forgetting these stuff :D
    
    ---------------------------------------------------------------------------
    
    by bschussek at 2012-04-14T08:58:29Z
    
    I restored and deprecated FieldType now. I'd appreciate further reviews.
    
    ---------------------------------------------------------------------------
    
    by stloyd at 2012-04-14T09:02:32Z
    
    Maybe we should try to avoid this BC in templates ? What do you think about similar move like with `FieldType` ? (hold old, but inside just render new)
    
    ---------------------------------------------------------------------------
    
    by bschussek at 2012-04-14T09:07:22Z
    
    @stloyd You mean for those cases where people explicitely render the block "field_*"? We can do that.
    
    ---------------------------------------------------------------------------
    
    by stloyd at 2012-04-14T09:09:45Z
    
    @bschussek Yes I mean this case =) Sorry for not being explicit, I need some coffee I think =)
    
    ---------------------------------------------------------------------------
    
    by bschussek at 2012-04-17T14:45:35Z
    
    I added the field_* blocks again for BC. Could someone please review again? Otherwise this can be merged.
    
    ---------------------------------------------------------------------------
    
    by Burgov at 2012-04-17T15:11:16Z
    
    @bschussek I'm not sure what has changed to cause this, but if I try out your branch on our forms, if I leave the value of an input empty, eventually the reverseTransform method receives a null value, rather than a '' (empty string) value, as on the current symfony master.
    
    DateTimeToLocalizedStringTransformer, for example, will throw an Exception if the value is not a string
    
    ```php
    if (!is_string($value)) {
       throw new UnexpectedTypeException($value, 'string');
    }
    ```
    
    Other than that, all forms render just the same as they do on symfony master
    
    ---------------------------------------------------------------------------
    
    by bschussek at 2012-04-17T15:30:29Z
    
    @Burgov Fixed.
  5. merged branch umpirsky/issue-3379 (PR #3922)

    fabpot committed Apr 18, 2012
    Commits
    -------
    
    24bd8f4 Added missing dot to translation messages.
    4bff221 Added missing dot to translation messages.
    7454894 Added missing dot to translation messages.
    6e90c50 Updated upgrade instructions.
    7e21dd1 Added missing dot to translation messages.
    
    Discussion
    ----------
    
    Issue 3379
    
    This should fix [issues 3379](symfony#3379)
    
    ---------------------------------------------------------------------------
    
    by stof at 2012-04-13T15:06:32Z
    
    Your branch conflicts with master. Please rebase it
    
    ---------------------------------------------------------------------------
    
    by umpirsky at 2012-04-13T19:11:54Z
    
    @stof I tried to rebase, I'm not sure if I did everything right. Is it ok now?
    
    ---------------------------------------------------------------------------
    
    by umpirsky at 2012-04-13T19:12:06Z
    
    @stof I tried to rebase, I'm not sure if I did everything right. Is it ok now?
    
    ---------------------------------------------------------------------------
    
    by mvrhov at 2012-04-13T19:19:34Z
    
    IMHO no, because there are commits from other people. Did you follow the [instructions](http://symfony.com/doc/current/contributing/code/patches.html#id1)?
    
    ---------------------------------------------------------------------------
    
    by stof at 2012-04-13T19:36:53Z
    
    @mvrhov commits from others ?
    
    ---------------------------------------------------------------------------
    
    by umpirsky at 2012-04-13T19:41:53Z
    
    @stof There were some, so I reverted. Now I'm trying again following instructions from Symfony doc.
    
    I come to this:
    
    ```
    $ git push origin issue-3379
    To git@github.com:umpirsky/symfony.git
     ! [rejected]        issue-3379 -> issue-3379 (non-fast-forward)
    error: failed to push some refs to 'git@github.com:umpirsky/symfony.git'
    To prevent you from losing history, non-fast-forward updates were rejected
    Merge the remote changes (e.g. 'git pull') before pushing again.  See the
    'Note about fast-forwards' section of 'git push --help' for details.
    ```
    
    And I don't know how to fix this. Any idea?
    
    ---------------------------------------------------------------------------
    
    by stof at 2012-04-13T19:43:45Z
    
    @umpirsky when you rebase, it is logical to need to force the push
    
    ---------------------------------------------------------------------------
    
    by umpirsky at 2012-04-13T19:44:38Z
    
    @stof I did `git push -f origin issue-3379`. I hope it's fixed now.
    
    ---------------------------------------------------------------------------
    
    by maoueh at 2012-04-13T20:39:34Z
    
    @umpirsky seems better than last time I checked :)
    
    ---------------------------------------------------------------------------
    
    by umpirsky at 2012-04-13T20:43:04Z
    
    @maoueh Is it good enough? :)
    
    ---------------------------------------------------------------------------
    
    by maoueh at 2012-04-13T20:51:27Z
    
    @umpirsky At least, the rebase seems good enough :D As for the subject of the PR, I don't pronounce myself ;)
    
    ---------------------------------------------------------------------------
    
    by vicb at 2012-04-13T20:53:23Z
    
    you should probably squash the commits
  6. Merge branch '2.0'

    fabpot committed Apr 18, 2012
    * 2.0:
      Revert "merged branch jakzal/2.0-StaticMethodLoaderFix (PR #3937)"
      fixed CS
  7. Revert "merged branch jakzal/2.0-StaticMethodLoaderFix (PR #3937)"

    fabpot committed Apr 18, 2012
    This reverts commit 0078faa, reversing
    changes made to 098b934.
  8. tweaked previous merge

    fabpot committed Apr 18, 2012
  9. merged branch canni/composer (PR #3291)

    fabpot committed Apr 18, 2012
    Commits
    -------
    
    aa055df [Composer] Stwitch to composer vendors management
    
    Discussion
    ----------
    
    [Composer] Stwitch to composer vendors management
    
    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/canni/symfony.png?branch=composer)](http://travis-ci.org/canni/symfony)
    
    This speeds up Travis CI builds to `~2 min` also makes vendor management
    a lot easier.
    
    ---------------------------------------------------------------------------
    
    by fabpot at 2012-02-09T06:24:24Z
    
    I'm -1 on this change. The `vendors.php` script is *only* for people working on the core so that we can run the unit tests. So, we need the flexibility to test on many different versions of the code and having the repository here is kind of mandatory.
    
    ---------------------------------------------------------------------------
    
    by Seldaek at 2012-02-09T08:15:28Z
    
    You can `composer install --dev` to get proper clones. I'm not really pro or against, just saying it's an option.
    
    ---------------------------------------------------------------------------
    
    by canni at 2012-02-09T08:28:54Z
    
    @fabpot I understand yours point, but from my view transferring the whole git structure of *vendors* is little pointless IMO (especially in Travis env)
    but I think I can make this change optional, so Travis and anyone that prefer to, can use `composer` an with old functionality available.
    
    (There will be almost no duplication, as anyway we're updating `composer.json`)
    
    ---------------------------------------------------------------------------
    
    by canni at 2012-02-09T09:20:17Z
    
    @fabpot I've enabled both behaviors, everything will work regardless of using `composer` or `vendors.php` this lets the developer decide what to use
    
    ---------------------------------------------------------------------------
    
    by drak at 2012-02-16T12:05:28Z
    
    Since there is a `--dev` option in Composer then I think this is a good idea.  You could also add composer.phar to the repo bin  directory.
    
    ---------------------------------------------------------------------------
    
    by henrikbjorn at 2012-02-16T12:06:55Z
    
    `--dev` have been renamed to `--prefer-source`
    
    ---------------------------------------------------------------------------
    
    by canni at 2012-02-16T12:22:01Z
    
    @fabpot any chance to consider this merge? If not, this PR can be closed.
    
    ---------------------------------------------------------------------------
    
    by henrikbjorn at 2012-02-16T12:25:51Z
    
    @canni This is the goal eventually. But i think we need composer to be a bit more stable in its solver.
    
    ---------------------------------------------------------------------------
    
    by francoispluchino at 2012-02-16T12:39:24Z
    
    👍
    
    ---------------------------------------------------------------------------
    
    by jmikola at 2012-04-06T18:19:27Z
    
    @fabpot: Is this PR still off the table, or are you reconsidering it with the `--prefer-source` option? I was just running symfony unit tests, and attempted to install deps with composer as I thought this PR or another like it had recently been merged to core. It wasn't :)
    
    Admittedly, it's a downside that vendor libs, even if git repositories, will be nestled within the `.composer/` directory.
    
    ---------------------------------------------------------------------------
    
    by drak at 2012-04-07T00:20:33Z
    
    @canni This PR needs to be rebased and reviewed because of the changed tests directory (there is no longer a central `tests/` folder).
    
    ---------------------------------------------------------------------------
    
    by canni at 2012-04-07T06:34:28Z
    
    Hey,
    
    will do after a weekend.
    
    canni
    
    Użytkownik Drak <reply@reply.github.com> napisał:
    
    >@canni This PR needs to be rebased and reviewed because of the changed tests directory (there is no longer a central `tests/` folder).
    >
    >---
    >Reply to this email directly or view it on GitHub:
    >symfony#3291 (comment)
    
    ---------------------------------------------------------------------------
    
    by canni at 2012-04-08T19:02:03Z
    
    @drak done.
  10. fixed typo

    fabpot committed Apr 18, 2012
  11. fixed previous merge

    fabpot committed Apr 18, 2012
  12. fixed CS

    fabpot committed Apr 18, 2012
  13. merged 2.0

    fabpot committed Apr 18, 2012
  14. merged branch stealth35/locale_intl_error_name (PR #3959)

    fabpot committed Apr 18, 2012
    Commits
    -------
    
    5799d25 Add BOGUS UErrorCode test
    6f9c05d [Locale] Complete Stub with intl_error_name
    
    Discussion
    ----------
    
    [Locale] Complete StubIntl with the missing intl_error_name
    
        Bug fix: yes
        Feature addition: no
        Backwards compatibility break: no
        Symfony2 tests pass: yes
        Fixes the following tickets: -
        Todo: -
  15. merged branch eriksencosta/locale-fixes-2.0 (PR #3955)

    fabpot committed Apr 18, 2012
    Commits
    -------
    
    fab1b5a [Locale] changed inequality operator to strict checking and updated some assertions
    09d30d3 [Locale] refactored some code
    e4cbbf3 [Locale] fixed StubNumberFormatter::format() to behave like the NumberFormatter::parse() regarding to error flagging
    f16ff89 [Locale] fixed StubNumberFormatter::parse() to behave like the NumberFormatter::parse() regarding to error flagging
    0a60664 [Locale] updated StubIntlDateFormatter::format() exception message when timestamp argument is an array for PHP >= 5.3.4
    e4769d9 [Locale] reordered test methods
    312a5a4 [Locale] fixed StubIntlDateFormatter::format() to set the right error for PHP >= 5.3.4 and to behave like the intl when formatting successfully
    
    Discussion
    ----------
    
    [2.0][Locale] some fixes
    
    Bug fix: yes
    Feature addition: no
    Backwards compatibility break: no
    Symfony2 tests pass: yes
    Todo: -
    
    Fixed some inconsistencies between the stub and the intl implementation:
    
     - `StubIntlDateFormatter::format()` to set the right error for PHP >= 5.3.4 and to behave like the intl when formatting successfully
     - updated `StubIntlDateFormatter::format()` exception message when timestamp argument is an array for PHP >= 5.3.4
     - `StubNumberFormatter::parse()` to behave like the NumberFormatter::parse() regarding to error flagging
     - `StubNumberFormatter::format()` to behave like the NumberFormatter::parse() regarding to error flagging
  16. merged branch lsmith77/router_debug_refactoring (PR #3963)

    fabpot committed Apr 18, 2012
    Commits
    -------
    
    98a0052 improved readability
    b06537e refactored code to use get() when outputting a single route
    
    Discussion
    ----------
    
    Router debug refactoring
    
    Bug fix: no
    Feature addition: yes
    Backwards compatibility break: no
    Symfony2 tests pass: [![Build Status](https: //secure.travis-ci.org/lsmith77/symfony.png?branch=router_debug_refactoring)](http://travis-ci.org/lsmith77/symfony)
    Fixes the following tickets: -
    
    refactored code to use get() when outputting a single route
    this is useful for a CMS, where in most cases there will be too many routes to make it feasible to load all of them. here a router implementation will be used that will return an empty collection for ->all(). with this refactoring the given routes will not be listed via router:debug, but would still be shown when using router:debug [name]
  17. merged branch bschussek/issue3228 (PR #3317)

    fabpot committed Apr 18, 2012
    Commits
    -------
    
    5208bbe [Validator] Fixed typo, updated CHANGELOG and UPGRADE
    dc059ab [Validator] Added default validate() implementation to ConstraintValidator for BC
    6336d93 [Validator] Renamed ConstraintValidatorInterface::isValid() to validate() because of the lack of a return value
    46f0393 [Validator] Removed return value from ConstraintValidatorInterface::isValid()
    
    Discussion
    ----------
    
    [Validator] Renamed ConstraintValidatorInterface::isValid() to validate() and removed return value
    
    Bug fix: no
    Feature addition: no
    Backwards compatibility break: **YES**
    Symfony2 tests pass: yes
    Fixes the following tickets: -
    Todo: update the documentation
    
    ![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3228)
    
    Before I begin, this PR is up for discussion.
    
    I removed the return value of ConstraintValidator::isValid() because it wasn't used anymore within the framework. Removing it also means a simplification for userland implementations. Already now the validation component only depended on violation errors being present for deciding whether the validation was considered failed or not.
    
    Because the name `isValid` does not make a lot of sense without a return value, I changed it to `validate`. Note that this affects an interface (ConstraintValidatorInterface) previously marked with `@api` by us!
    
    What do you think about this change?
    
    ---------------------------------------------------------------------------
    
    by stof at 2012-02-09T17:51:38Z
    
    @bschussek IIRC, the Validator component was part of the components that are not considered as stable for 2.0 (there is 4 of them IIRC, including Config, Security and Form) so changing the interface should not be an issue here
    
    ---------------------------------------------------------------------------
    
    by lsmith77 at 2012-02-09T17:54:55Z
    
    No it was .. form wasn't:
    http://symfony.com/doc/2.0/book/stable_api.html
    
    ---------------------------------------------------------------------------
    
    by rdohms at 2012-02-10T13:23:32Z
    
    I fail to see the value of the BC in this case.
    Just because the framework does not use given functionality anymore is not reason to drop it, since all of this was clearly proposed as a "component" to be used in other projects. Other implementations of validator in other projects might actually depend on this.
    
    Is it possible to just add a new value and have both functionalities available?
    
    ---------------------------------------------------------------------------
    
    by stof at 2012-02-10T13:25:12Z
    
    @rdohms the point is that the return value is confusing. Someone may return ``false`` by thinking it will mark the field as invalid (which is implied by the name ``isValid``) whereas it is not the case at all
    
    ---------------------------------------------------------------------------
    
    by bschussek at 2012-02-10T13:30:13Z
    
    Exactly. UniqueEntityValidator for example always returned `true` and nobody ever noticed.
    
    ---------------------------------------------------------------------------
    
    by beberlei at 2012-02-10T13:53:03Z
    
    @bschussek but its not a bug, setting the execution context failure is enough. returning false would lead to a second error message being evicted. This is the weird problem of the API imho
    
    ---------------------------------------------------------------------------
    
    by bschussek at 2012-02-10T13:54:49Z
    
    @beberlei This has already been fixed.
    
    ---------------------------------------------------------------------------
    
    by stof at 2012-02-10T13:59:59Z
    
    @beberlei in the master branch, errors are not duplicated anymore as the return value is simply ignored.
    
    ---------------------------------------------------------------------------
    
    by Tobion at 2012-02-10T14:29:03Z
    
    I'm +1. If people are concerned about this strong BC break we could maybe add a fallback for the majority.
    Most people propably have extended the ConstraintValidator and not used the interface directly. So we can change the Interface and at the same time provide a default proxy method in ConstraintValidator for validate. I.e.
    
        public function validate($value, Constraint $constraint)
        {
            $this->isValid($value, $constraint);
        }
    
    Thus all people who have extended ConstraintValidator won't notice a BC break.
    
    ---------------------------------------------------------------------------
    
    by hades200082 at 2012-02-10T16:10:31Z
    
    Could you not have both validate and isValid as separate methods with distinct purposes?
    
    ---------------------------------------------------------------------------
    
    by stof at 2012-02-10T16:55:12Z
    
    @hades200082 which distinct purposes ?
    
    ---------------------------------------------------------------------------
    
    by hades200082 at 2012-02-10T17:02:57Z
    
    One should actually validate.  The other should return whether it is valid or not as a bool.
    
    Even if isValid calls validate to determine this surely they are distinct purposes?  doing it this way would not require a break to BC but the existing components in the framework could be switched to use validate.
    
    ---------------------------------------------------------------------------
    
    by bschussek at 2012-02-10T17:10:50Z
    
    @hades200082 Validators are stateless. They don't "remember" whether they validated successfully or not.
    
    ---------------------------------------------------------------------------
    
    by hades200082 at 2012-02-10T17:13:25Z
    
    Maybe they should?  Would save revalidating every time
    
    ---------------------------------------------------------------------------
    
    by stof at 2012-02-10T17:16:10Z
    
    @hades200082 how could they be stateless ? you can use the same instance to validate several values. For instance, the UniqueEntityValidator is a service and so will be reused.
    
    ---------------------------------------------------------------------------
    
    by fabpot at 2012-02-11T23:40:09Z
    
    I would really like that we do not break BC in this case.
    
    ---------------------------------------------------------------------------
    
    by stof at 2012-02-11T23:59:02Z
    
    @fabpot there is also a BC break in the previous changes: the return value has no meaning at all now (it is not even considered by the GraphWalker.
    Most 2.0 validator will continue working because of the new implementation of setMessage but I can provide the 2 broken cases:
    
    ```php
    <?php
    
    /**
     * This validator always set the message, even when it is valid to keep things simple.
     * This works fine in 2.0.x (as the return value is what makes the decision) but will
     * add a violation in 2.1 (setMessage adds the violation to keep things working for
     * cases setting the message only for invalid values, like the core used to do).
     */
    public function isValid($value, Constraint $constraint)
    {
        $this->setMessage($constraint->message);
    
        return true;
    }
    
    /**
     * This validator never set the message, failing with an empty message.
     * This works fine in 2.0.x (as the return value is what makes the decision) but will
     * not add the violation in 2.1.
     */
    public function isValid($value, Constraint $constraint)
    {
        return false;
    }
    ```
    
    The second one is clearly an edge case as it would absolutely not be user-friendly but the first one makes totally sense when using the 2.0.x API (with a boolean expression using the value of course)
    
    ---------------------------------------------------------------------------
    
    by fabpot at 2012-02-12T00:11:19Z
    
    I agree with you; I should probably have refused to merge the previous PR. And I think we need to reconsider this change. If not, why are we even bothering tagging stuff with the @api tag?
    
    ---------------------------------------------------------------------------
    
    by bschussek at 2012-02-12T10:15:55Z
    
    @stof I disagree with you. Setting an error message but not letting the validation fail is not how the API is supposed to work. Also the opposite was not meant to work, as it results in empty error messages. The third example is that a validator *had* to return true if it called `addViolation` directly. These cases show that the previous implementation was clearly buggy and needed to be fixed.
    
    This PR is only a consequence that cleans the API up.
    
    @fabpot IMHO validator was too young and not tried enough to be marked as stable. But as we can't change this anymore, I think the decision we have to make is
    
    * BC/reliance on `@api` marks vs.
    * API usability (also considering the coming LTR)
    
    ---------------------------------------------------------------------------
    
    by bschussek at 2012-02-12T10:18:12Z
    
    BTW @Tobion's suggestion could definitely make a transition easier.
    
    ---------------------------------------------------------------------------
    
    by fabpot at 2012-02-15T10:26:10Z
    
    @bschussek +1 for @Tobion's suggestion.
    
    ---------------------------------------------------------------------------
    
    by Brouznouf at 2012-02-15T16:06:12Z
    
    Could be nice to comment function as deprecated and/or trigger a E_USER_DEPRECATED error when using this method to prevent user calling this method.
    
    ---------------------------------------------------------------------------
    
    by stof at 2012-02-15T16:09:37Z
    
    trigger E_USER_DEPRECATED would be wrong as the kernel set the error reporting to ``-1`` and registers an error handler tuning all reported errors  to exception in debug mode, so it would be a BC break.
    Commenting the function as deprecated in indeed possible
    
    ---------------------------------------------------------------------------
    
    by rdohms at 2012-02-29T11:15:01Z
    
    Went back to working on validators and it really makes me disagree with these changes a little more. Let me explain.
    
    In the isValid method, i like to work with return early checks, so straight up i check some stuff and return early either true/false.
    
    From the frameworks perspective true/false does not make a difference, but from a reader's perspective it adds a lot of value:
    
            if ($object->getId() === null) {
                return true;
            }
    
    versus
    
            if ($object->getId() === null) {
                return;
            }
    
    having the return true make it clear that in this case the object is valid for anyone who is reading my validator. i think this is a good practice to push forward.
    
    Anyway, my 2 cents on it.
    
    ---------------------------------------------------------------------------
    
    by stof at 2012-04-04T00:05:09Z
    
    @fabpot what do you think about this ?
    
    ---------------------------------------------------------------------------
    
    by bschussek at 2012-04-05T16:37:38Z
    
    @rdohms: Still, how do you want to deal with the fact that the return value is ignored anyway?
    
    ---------------------------------------------------------------------------
    
    by rdohms at 2012-04-06T06:51:07Z
    
    @bschussek Nobody has to know? I would keep it as it is, i have noticed that returning false without any error messages does not get me the expected results, so it seems there is no harm in keeping the parctice of true/false even if it is misleading.
    
    Other then that.. i would alter the code to self create a error message if false is returned, thus making true/false still work, but i'm guessing that's not what your vision says, even if i find it les readable and understandable. So yeah, just my opinioin.
    
    ---------------------------------------------------------------------------
    
    by bschussek at 2012-04-06T07:02:53Z
    
    @rdohms: Your opinion is appreciated. Self-creation of error messages is what we did before, unfortunately it's very hacky then to suppress the self-creation if you want to return false and add (potentially more than one) error messages yourself.
    
    ---------------------------------------------------------------------------
    
    by bschussek at 2012-04-17T14:58:07Z
    
    I added @Tobion's suggestion now. Can you please review again? Otherwise this is ready for merge.
    
    ---------------------------------------------------------------------------
    
    by Tobion at 2012-04-17T15:05:16Z
    
    Statement in changelog and upgrade is missing, or?
  18. merged branch willdurand/propel-type-guesser (PR #3953)

    fabpot committed Apr 18, 2012
    Commits
    -------
    
    b7c2d3d [Propel1] Added tests for guessType() method
    897a389 [Propel1] Added tests for the PropelTypeGuesser
    cffcdc9 Improve the TypeGuesser to match the latest Sf2.1
    
    Discussion
    ----------
    
    [Form] Propel type guesser
    
    Thanks to @vicb, the PropelTypeGuesser has been updated. I've added unit tests to prove his improvement, and everything is ok from my point of view.
    
    ---------------------------------------------------------------------------
    
    by willdurand at 2012-04-15T16:38:09Z
    
    Well, I made the changes, but I really don't care about fixing these comments.
    
    To write `assertNull` doesn't improve readabilty, as I expect to read the returned value in the test. And, it's better to read `null` than to read the assertion method. Moreover, that makes the test suite inconsistent, as you are not able to read each tests the same way :)
    
    ---------------------------------------------------------------------------
    
    by vicb at 2012-04-15T17:20:01Z
    
    Great ! thanks @willdurand
  19. merged branch Crell/response-chain-prepare (PR #3954)

    fabpot committed Apr 18, 2012
    Commits
    -------
    
    a0d047b Return  from Response::prepare() so that the method may be chained.
    
    Discussion
    ----------
    
    Return  from Response::prepare() so that the method may be chained.
    
    Currently, Response::prepare() returns nothing, ever.  That's such a waste.  This one-liner returns $this, so that the method may be chained.  That allows for, for instance:
    
    $kernel->handle($request)->prepare($request)->send();
    
    ---------------------------------------------------------------------------
    
    by Crell at 2012-04-15T23:48:44Z
    
    Done and done.
    
    ---------------------------------------------------------------------------
    
    by vicb at 2012-04-16T06:05:08Z
    
    Please update https://github.com/Crell/symfony/blob/e05fbf19a9dca656f89e4f8396e7c7e80f221eba/src/Symfony/Component/HttpFoundation/StreamedResponse.php#L77 and squash your commits. thanks.
    
    ---------------------------------------------------------------------------
    
    by Crell at 2012-04-17T00:26:07Z
    
    Fixed and squished.
Commits on Apr 17, 2012
  1. [Validator] Renamed ConstraintValidatorInterface::isValid() to valida…

    webmozart committed Feb 9, 2012
    …te() because of the lack of a return value