Commits
-------
748bbe1 Make windows test run on windows only
e7f1295 [Filesystem] Fix Filesystem::chmod to apply umask properly
5c059aa Fix chmod() calls to apply umask
13c07d1 [Filesystem] Fix typo
c578d3a [Filesystem] Fix makePathRelative on windows with mixed paths, fix tests
Discussion
----------
[Filesystem][Others] Fix chmod method and all calls to chmod throughout the framework
Fixes the issue I mentioned in #4004 - basically php's chmod() does not apply the umask by default (unlike mkdir's mode arg which is masked by umask, from which I guess the confusion comes from).
So I expanded all cache writes and such to 0666 when they were 0644, and then mask it against umask, so that we respect the user settings a bit better.
Also fixed Filesystem::chmod which completely ignored the umask argument before.
Fixed a few tests on windows too.
Commits
-------
8bdff01 [DoctrineBridge][Form] added collection guess for array Doctrine type and array constraint type
Discussion
----------
[Form] [DoctrineBridge] Better field type guessing for array doctrine type and array validator type
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #1692
Todo: -
---------------------------------------------------------------------------
by bschussek at 2012-04-18T08:45:17Z
Could you please add an entry to the CHANGELOG and squash your commits into one?
---------------------------------------------------------------------------
by pvanliefland at 2012-04-18T17:20:39Z
Done
Commits
-------
725423c Add test to prevent regressions
3b2f542 Fix asset generation with an empty asset
Discussion
----------
[Templating] Return base URL when an empty path is given to asset()
I think it's straightforward enough in the patch. Tests pass. It's quite useful to generate the base path to send to a JS frontend.
Commits
-------
b7b26af [DependencyInjection] Added, implemented and tested IntrospectableContainerInterface::initialized()
Discussion
----------
[DependencyInjection] IntrospectableContainerInterface::initialized()
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Added, implemented and tested `IntrospectableContainerInterface::initialized()`, which allows checking for whether or not a service id has actually been loaded, without forcing it to load. This could be implemented in several places to prevent loading a service when it's only needed if it has been previously loaded - for example the SwiftmailBundle's event listener for the `kernel.terminate` event, which currently forces Swiftmail to load on every request to check for messages to send, even if it was not previously loaded.
---------------------------------------------------------------------------
by fabpot at 2012-03-11T09:10:32Z
Do you have any other examples in mind where it would be useful?
---------------------------------------------------------------------------
by stof at 2012-03-11T12:39:22Z
@fabpot 2 exemples:
- the Swiftmailer listener to avoid loading the initialization code of Swiftmailer in each kernel.terminate event just to check that there is no pending message in the spool (if the spool has never been retrieved, it cannot have messages) (this is the use case we discussed on IRC last night)
- closing Doctrine connections and Monolog handlers in the shutdown method without creating them if they were not used
---------------------------------------------------------------------------
by stof at 2012-03-11T12:39:53Z
However, I don't like the name of the method
---------------------------------------------------------------------------
by lsmith77 at 2012-03-11T13:26:34Z
sounds very useful
---------------------------------------------------------------------------
by evillemez at 2012-03-11T17:00:39Z
@fabpot another example:
* Forcing a session to write early, if it was previously loaded - but not having to load the session to check, thus potentially forcing a database connection (if that's how the session is being handled) when it's not needed.
@stof Would more or less verbose be better? Like, `isServiceLoaded()` or just `loaded()`.... or a better word than "loaded" ? :)
---------------------------------------------------------------------------
by stof at 2012-03-11T17:03:11Z
@evillemez My issue is the word ``loaded``. I don't think it is clear about what it means here
---------------------------------------------------------------------------
by lsmith77 at 2012-03-11T17:04:24Z
one thing we should also keep in mind here is the scope of the service.
BTW: there are also a couple of CS violations in your PR
---------------------------------------------------------------------------
by fabpot at 2012-03-11T17:07:43Z
@stof: I agree that we should think of a better name: `initialized()` or `exists()` (to differentiate from `has()`)?
@evillemez: After choosing the name, can you work on actually using the new method for some of the use cases mentioned in this PR?
---------------------------------------------------------------------------
by lsmith77 at 2012-03-11T17:20:12Z
what i meant with "scope" was if we are only talking about services instantiated in the current scope? but i guess there is no way to handle anything else anyway.
as for name .. i think ``instantiated()`` is more fitting than ``initialized()``.
``exists()`` could be confused with ``has()`` imho
---------------------------------------------------------------------------
by stof at 2012-03-11T17:26:06Z
The current implementation only works for container-scoped services. It does not make sense for prototyped-scope services anyway (as the container does not keep them) but supporting scoped services will be tricky IMO
---------------------------------------------------------------------------
by mvrhov at 2012-03-11T17:34:21Z
hasBeen(Used|Called|Initialized),
---------------------------------------------------------------------------
by evillemez at 2012-03-11T17:54:43Z
The next day or two I'm only around for an hour or so here and there, I may be a little slow to respond.
@stof @lsmith77 I agree with either `instantiated()` or `initialized()`, I also think `exists()` might be easily confused with `has()`
@lsmith77 Besides the opening/closing brace placements, was there anything else?
@fabpot I would happily implement it in the Swiftmail bundle - as of now that's the only area where I know absolutely it would be useful. The Session example I mentioned was an issue I ran into working on another app in another framework, and I'm not familiar yet with the internals of the Monolog/Doctrine Bundles. But if people could point me in the right direction I'd be willing to check it out.
I'm still relatively new to some of the Symfony2 internals, so if there are obvious things I'm missing, please don't hesitate to point them out. :)
---------------------------------------------------------------------------
by evillemez at 2012-03-11T18:00:29Z
@lsmith77 Oh... I think there were some tab issues I didn't see in my editor, I'll fix those too.
---------------------------------------------------------------------------
by stof at 2012-03-11T18:13:03Z
The places where it should be used for Doctrine and Monolog are in separate repos anyway so it cannot be part of this PR
---------------------------------------------------------------------------
by evillemez at 2012-03-12T03:38:50Z
Any thoughts on `instantiated` vs `initialized`? I'm leaning towards `initialized`.
How should I proceed, close this request and submit another with the changed name and fixed CS violations?
---------------------------------------------------------------------------
by fabpot at 2012-03-12T07:41:11Z
`initialized()` looks fine to me. Make your changes, squash your commits and then force the push to your branch (the PR will be updated automatically).
---------------------------------------------------------------------------
by evillemez at 2012-03-12T20:49:17Z
I was about to squash my commits to update this, but it just occurred to me that I hadn't considered the interface. Does anyone feel this method `initialized()` should be defined in ContainerInterface as well? It seems like it's generic enough that it would make sense. But I'm not immediately aware if this would cause BC breaks.
---------------------------------------------------------------------------
by fabpot at 2012-03-13T11:34:33Z
We cannot break BC for `ContainerInterface` as this is marked with the `@api` tag.
---------------------------------------------------------------------------
by henrikbjorn at 2012-03-13T12:34:42Z
Is it a BC break if we add a method? i thought only changing the already written methods would be a BC break.
---------------------------------------------------------------------------
by ooflorent at 2012-03-13T12:39:44Z
@henrikbjorn It will raise a fatal error if the method isn't implemented in existing class.
---------------------------------------------------------------------------
by lsmith77 at 2012-03-13T13:06:26Z
we could however add a new interface that extends from the previous one.
---------------------------------------------------------------------------
by evillemez at 2012-03-13T15:40:39Z
Are the BC breaks we are worried about for compatibility just within the Symfony repo - or in general in case others have implemented the interface elsewhere? As far as Symfony is concerned, the only class I can find that implements `ContainerInterface` is `Container`, so adding the method shouldn't be an issue.
If it's an issue of principle, in case others may have implemented the `ContainerInterface`, then... yeah, it's a break, and maybe should be left out. I suppose this would mean that other places in code that type hint for `ContainerInterface` would have to change the type hint to `Container` if they want to implement the `initialized()` method.
Is this more or less acceptable than updating the interface?
---------------------------------------------------------------------------
by evillemez at 2012-03-14T19:17:27Z
Hadn't properly squashed commits, just fixed.
---------------------------------------------------------------------------
by evillemez at 2012-03-15T14:06:38Z
I'm done with this PR, unless there is a consensus that I should also implement `Container::initialized()` in `ContainerInterface`.
Anything else I need to do?
---------------------------------------------------------------------------
by Seldaek at 2012-03-15T15:41:44Z
@evillemez the common pattern for BC is to add a new interface, say IntrospectableContainerInterface or something, that extends ContainerInterface, then you can type-hint that without restricting use of competing implementations of the Container class.
---------------------------------------------------------------------------
by stof at 2012-04-03T22:30:51Z
@evillemez Please update this PR. It conflicts with master because of the move of tests. And the new interface suggested by @Seldaek is a good idea IMO
---------------------------------------------------------------------------
by evillemez at 2012-04-04T14:57:29Z
@Stof I may not be able to get to this until the end of the week, but I'll do it as soon as I can.
I'll rebase against the current master, and add/implement the interface. Is `IntrospectableContainerInterface ` as suggested by @Seldaek ok with everyone?
Are there other features that we can think of that would be useful for this new interface? I don't want to start a precedent of adding a new interface for every new method that comes up... I understand it makes sense for not breaking backwards compatibility for something previously marked as stable, but still, it's yet another file that will likely be included on every request.
---------------------------------------------------------------------------
by stof at 2012-04-04T15:35:15Z
@evillemez classes used on every requests can be added to some cached bootstrap files (which are loaded during the ``$kernel->loadClassCache()`` call in your front controller) to avoid including many files through the autoloader. And for even better performances in prod, the solution is to use APC with ``apc.stat = 0``, as advocated by @lsmith77
---------------------------------------------------------------------------
by evillemez at 2012-04-15T19:00:07Z
Ok, rebased against current master and implemented the interface. I didn't mark it as `@api` because I think we may want to consider if there's any other functionality that could be implemented there before declaring it stable.
Sorry for the delay. My wife and I are in the process of buying a house, and some unexpected things have come up.
---------------------------------------------------------------------------
by fabpot at 2012-04-18T10:59:01Z
Can you rebase before I merge? Thanks.
Commits
-------
ccd6bbc [Form] Removed extra CSRF field on collection prototype
Discussion
----------
[Form] Removed extra CSRF field on collection prototype
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3987, #3990
Todo: -
---------------------------------------------------------------------------
by bschussek at 2012-04-19T08:43:32Z
Wait a minute please, I oversaw some tests that have to be adapted.
---------------------------------------------------------------------------
by bschussek at 2012-04-19T09:22:45Z
Fixed. ping @fabpot
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: [](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`: 35e6c8ed0f/vendors.php
Without this code coverage report generation fails with error message: `Fatal error: Class 'Monolog\Handler\ChromePHPHandler' not found`.
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)
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
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.
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](https://github.com/symfony/symfony/issues/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
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: -
[](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:
>https://github.com/symfony/symfony/pull/3291#issuecomment-5004750
---------------------------------------------------------------------------
by canni at 2012-04-08T19:02:03Z
@drak done.
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
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: [](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]
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
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?
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
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 e05fbf19a9/src/Symfony/Component/HttpFoundation/StreamedResponse.php (L77) and squash your commits. thanks.
---------------------------------------------------------------------------
by Crell at 2012-04-17T00:26:07Z
Fixed and squished.
