Commits
-------
f2fea97 [Component][Finder] tests and condition: contains() used on dir
Discussion
----------
[Component][Finder] tests and condition: contains() used on dir
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
`Finder::contains()` and `Finder::notContains()` can't be used on directories.
---------------------------------------------------------------------------
by travisbot at 2012-05-08T06:33:11Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1273818) (merged f2fea974 into 919604ab).
Commits
-------
b865b09 [Session] Fix the PDO handler for mysql concurrent write
Discussion
----------
[RFC][Session] Make the PDO handler looks less hacky
Related discussion: ebc2f01e5b (commitcomment-1304221)
The current code works but looks hacky (`$dbTimeCol = CASE WHEN $dbTimeCol = :time THEN (VALUES($dbTimeCol) + 1) ELSE VALUES($dbTimeCol) END`).
Todo: wrap the mysql specific code in a `try...catch` if we choose this PR way (to be consistent with all other PDO invocations).
---------------------------------------------------------------------------
by travisbot at 2012-05-10T07:50:39Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1293131) (merged b865b096 into 48099a85).
Commits
-------
12e22c0 [Session] Memcache/d cleanup, test improvements
788adfb [Session] Pdo Handler cleanup
0216e05 [HttpFoundation][Session] Assume that memcache(d) instances are already configured
72d21c6 [HttpFoundation][Session] change possible replace() & set() for set only()
Discussion
----------
[Session] Non-native Session handlers
A few item to discuss. Needs @drak inputs.
* 72d21c66 is trivial,
* 0216e056 is about memcache(d) handlers
* I don't think the handlers should configure the memcache(d) instances. Those instances are injected into the storage so they should already be confidured (this will be done in the CacheBundle when available)
* A SW prefix has been added to the memcached handlers so that the same instance of memcached can be shared - you can still set the `Memcached::OPT_PREFIX_KEY` before injecting the memcached instance.
* It was not possible to use an expiration > 30days before, see [php.net](http://www.php.net/manual/en/memcached.expiration.php)
* 788adfb6 is trivial (cleanup in the PDO handler)
---------------------------------------------------------------------------
by travisbot at 2012-05-08T09:49:03Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1274808) (merged 788adfb6 into e54f4e46).
---------------------------------------------------------------------------
by drak at 2012-05-09T15:20:38Z
Overall this PR looks good to me. Since Memcache/d objects are passed by DI anyway, there is no need to provide a way to configure the objects here.
However, I am not sure it's consistent to provide internal handling of the prefix/expire if we are saying the objects should be configured and injected - if we hand over all configuration to the injected objects, that's exactly what we should do. In the case of the `Memcache` handler there is no handling for prefix by the Memcache object that is why it's handled internally.
Unless there are some other technical consideration I've missed, I would also not expect the same Memcache/d object to be used in all use cases (e.g. session storage and database caching layer). I realise we are trying to unify things in one cache component, but I am not entirely convinced session storage would necessarily have to be part of that nor that "one object fits all" is practical or wise.
As far as I am aware, apart from default settings, memcache/memcached instances retain their own settings once configured so it's quite feasible to expect there might be a couple of differently configured instances in a complex system.
In summary, I would remove the `$memcachedOptions` config entirely from the `MemcachedHander` along with the associated prefix and time and let it all be configured by the injected `Memcached` instance.
---------------------------------------------------------------------------
by travisbot at 2012-05-10T07:32:53Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1293064) (merged 12e22c0d into e54f4e46).
---------------------------------------------------------------------------
by vicb at 2012-05-10T07:34:31Z
@drak thanks for your feeback.
About the prefix: it might be necessary to avoid collisions when you re-use the same instance of `memcache/d`. This is why the prefix is handled internally and not by `memcached` (it would be global and not serve the purpose then).
About the ttl:
* `memcache/d` can not handle ttl > 30 days (they would consider the time as an absolute timestamp then) and this is why the PR always convert the ttl to an absolute ts (`time() + $ttl`)
* Moreover I think that the ttl should be initialized by the `Session`: there is no reason why the ttl should be different from the `gc_maxlifetime`. I think this is out of the scope of this PR.
About sharing `memcache/d ` instances: it will be possible but it does not mean that you have to, you still can use different instances if this suit your needs.
The tests have been improved.
If you are ok with the latest changes, this PR should be ready to be merged
---------------------------------------------------------------------------
by drak at 2012-05-10T09:29:18Z
@vicb - I think it's ok to merge now. You are right about the TTL since PHP will pass a maxlifetime not a timestamp, and since memcached varies how it treats $expire, it does need to be normalised in the handler. I'm not necessarily 100% convinced about the prefix, but I don't object. Nice work.
/cc @fabpot
Commits
-------
80a2a92 [2.1][Component][Yaml] fix 4022
Discussion
----------
[2.1][Component][Yaml] fix 4022
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: [](http://travis-ci.org/gajdaw/symfony)
Fixes the following tickets: #4121, #4022, #4135
Todo:
---------------------------------------------------------------------------
by stof at 2012-04-27T13:03:15Z
Why is it marked as ``[2.2]`` if it is a bugfix ?
@fabpot ping
---------------------------------------------------------------------------
by gajdaw at 2012-04-27T14:42:21Z
The title should be [2.1] - now it is correct.
I marked it 2.0 and PR was for 2.0 originally.
Fabien suggested that it should go to master branch: https://github.com/symfony/symfony/pull/4121#issuecomment-5362990
---------------------------------------------------------------------------
by fabpot at 2012-05-07T09:17:31Z
That does not work when you have something after the unindented collection:
collection:
key:
- a
- b
- c
foo: bar
---------------------------------------------------------------------------
by gajdaw at 2012-05-07T11:11:30Z
@fabpot Last commit contains test with your yaml:
collection:
key:
- a
- b
- c
foo: bar
Everything seems fine. Can you give me a hint: what do you mean, when you say "That does not work"?
---------------------------------------------------------------------------
by fabpot at 2012-05-07T12:36:19Z
Sorry, the failing test is the following:
test: Key/value after unindented collection
brief: >
Key/value after unindented collection
yaml: |
collection:
key:
- a
- b
- c
foo: bar
php: |
array('collection' => array('key' => array('a', 'b', 'c'), 'foo' => 'bar'))
---------------------------------------------------------------------------
by gajdaw at 2012-05-07T15:48:26Z
@fabpot Last commit passed your test.
---------------------------------------------------------------------------
by fabpot at 2012-05-07T17:28:21Z
Can you squash your commits? Thanks.
---------------------------------------------------------------------------
by travisbot at 2012-05-08T05:32:58Z
This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1273487) (merged 20891c58 into 919604ab).
---------------------------------------------------------------------------
by gajdaw at 2012-05-08T05:36:51Z
Done.
---------------------------------------------------------------------------
by travisbot at 2012-05-08T07:23:47Z
This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1274162) (merged 80a2a92e into 898ff4e0).
Commits
-------
c9ebe67 [DomCrawler] fixed encoding when using addHtmlContent() (fixes#3881)
Discussion
----------
[DomCrawler] fixed encoding when using addHtmlContent() (fixes#3881)
After looking around, this is clear that loadHtml() resets the encoding set on the DomDocument instance. So, the only workaround that actually works (and which is not an ugly hack) is to use `mb_convert_encoding` when it exists.
---------------------------------------------------------------------------
by Seldaek at 2012-05-07T12:38:43Z
+1 (Side note: Using your fork of symfony for PRs would be good I think, otherwise it creates noisy versions on packagist.)
Commits
-------
8ff11c1 [HttpFoundation] fixed docblock typos in session class
Discussion
----------
[HttpFoundation] fixed docblock typos in session class
Commits
-------
bdc21b4 [Validator] Add a base AbstractLoader
ead4908 [Validator] Some cleanup of the GraphWalker
23e15bb [Validator] Fix a bug in the ExecutionContext
Discussion
----------
[Validator] Fix/cleanup
Bug fix: yes
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: [](http://travis-ci.org/vicb/symfony)
* d2100a27 has some fixes for the EC,
* 51769e03 has some cleanup in the graph walker,
* f9b3591c add an AbstractLoader (namespace aliases does not belong to FileLoaders).
---------------------------------------------------------------------------
by vicb at 2012-05-07T08:32:40Z
@fabpot PR ready
Commits
-------
a196ca0 [Routing] Compiler: remove lazy quantifiers with no effect
8232aa1 [Routing] Compiler: fix in the computing of the segment separators
Discussion
----------
[Routing] Fix the matching process
This PR is based on the PR #3678, #4139.
[](http://travis-ci.org/vicb/symfony)
**The spec**
A pattern is composed of both text and variable segments: `/{variable}-test/{other_variable}`.
A variable segment will match anything until a separator is encountered. The separator is the character following the variable segment when available or preceding the variable otherwise (i.e. at the end of the pattern).
That is:
* the separator is `-` for the `variable`,
* the separator is `/` for the `other_variable`.
*Note: This default matching behavior can be overridden if a requirement is specified for a variable)*
**Fixes**
* The current behavior is to consider booth the preceding and following characters as separators (considering availability),
* The "preceding" separator of the first variable is always set to `/` whatever the preceding character is (due to `$pos = 0` for the first iteration).
**Todo**
Update the doc once this is merged
Commits
-------
bc63fb2 Fix some cs
Discussion
----------
Fix some cs
---------------------------------------------------------------------------
by fabpot at 2012-05-03T21:13:33Z
Can you squash your commits? Thanks.
---------------------------------------------------------------------------
by stephpy at 2012-05-03T22:18:07Z
It's ok
Commits
-------
95b8e29 [BrowserKit] Remove dependency of CookieJar to Response
Discussion
----------
[BrowserKit] Remove dependency of CookieJar to Response
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
The CookieJar has currently a hard dependency to `BrowserKit\Response`, but this dependency could be avoided.
---------------------------------------------------------------------------
by stof at 2012-05-01T21:52:34Z
Renaming a method *is* a BC break.
You should add a new method and keep the old one accepting the Response (and make it calling the new method internally). This way, it would add the new feature without breaking the BC.
---------------------------------------------------------------------------
by stof at 2012-05-01T21:53:31Z
And btw, I don't see the issue with BrowerKit depending on BrowserKit. If you have a class, you also have the other one.
---------------------------------------------------------------------------
by GromNaN at 2012-05-02T05:57:51Z
The issue is that I want to use the CookieJar without the Request/Response of BrowserKit.
---------------------------------------------------------------------------
by fabpot at 2012-05-03T06:37:02Z
You should also keep some unit tests for the old method.
---------------------------------------------------------------------------
by GromNaN at 2012-05-03T08:22:39Z
@fabpot I've made the changes.
---------------------------------------------------------------------------
by fabpot at 2012-05-03T10:53:47Z
Can you squash your commits before I merge? Thanks.
---------------------------------------------------------------------------
by GromNaN at 2012-05-03T10:57:30Z
@fabpot Squashed
Commits
-------
b1de2a2 [HttpKernel] fix typo, commit 9fed41 fixed only half of it
Discussion
----------
[HttpKernel] fix typo
commit 9fed41 fixed only half of it
Commits
-------
69e0451 [Security] fixed English grammar in exception message
Discussion
----------
[Security] fixed English grammar in exception message
Commits
-------
c195957 [Components] Tests/Autoloading fixes
Discussion
----------
Fix components
See #4141
----
This PR:
* configures each component to use composer to manage "dev" dependencies instead of env variables;
* adds phpunit configuration file on Filesystem component;
* fixes READMEs.
It's mergeable without any problems, but I would recommend to wait a fix in Composer in order to use `self.version` in `require`/`require-dev` sections.
Note: I kept `suggest` sections because it makes sense but this PR doesn't aim to provide useful explanations for each entry. It could be another PR, not that one.
---------------------------------------------------------------------------
by willdurand at 2012-04-30T20:43:13Z
@fabpot I reviewed each component, one by one. Now `phpunit` always works, even if tests are skipped. A simple `composer install --dev` allows to run the complete test suite. Each commit is well separated from the others. I guess, everything is ok now.
---------------------------------------------------------------------------
by Tobion at 2012-04-30T20:47:00Z
Please squash, as it makes no sense to have the same commit for each component.
---------------------------------------------------------------------------
by fabpot at 2012-05-01T14:26:11Z
Can you squash your commits before I merge? Thanks.
---------------------------------------------------------------------------
by willdurand at 2012-05-01T14:29:38Z
done
---------------------------------------------------------------------------
by fabpot at 2012-05-01T15:48:25Z
It does not seem that the commits are squashed.
---------------------------------------------------------------------------
by willdurand at 2012-05-01T15:54:08Z
done
* Switched to Composer to manage "dev" dependencies
* Fixed READMEs
* Excluded vendor in phpunit.xml.dist files
* Fixed message in bootstrap.php files
* Added autoloader for the component itself
Commits
-------
1f6c8d5 [HttpKernel] Added mock objects for Memcache(d) and Redis
e17217b [HttpKernel] Remove destructive flush() from memcache(d) storage profilers
Discussion
----------
[HttpKernel] Memcache and Redis profiler storage update
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Changes of this PR:
- change ```purge()``` method of memcache(d) profiler storage to delete only required items and be less destructive,
- mock objects for Redis and Memcache(d) storages were added to make unit tests independent from memcache(d)/redis extensions and memcache(d)/redis servers running on localhost.
Addresses issues with writing console output for IBM i5 Series (OS400).
The normal QP2TERM shell outputs garbage text when attempting to write
directly to STDOUT, likely because of EBCDIC character-encoding used
on IBM platforms. Writing to the OUTPUT mimics using 'echo' or 'print'
and prints properly in the console.
Fixes#1434
Commits
-------
246c885 [Form] Fixed: Default value of 'error_bubbling' is now determined by the 'single_control' option
d3bb4d0 [Form] Renamed option 'primitive' to 'single_control'
167e64f [Form] Fixed: Field attributes are not rendered in the label anymore. Label attributes are now passed in "label_attr"
68018a1 [Form] Dropped useless test that is guaranteed by OptionsParser tests and that needs to be adapted very often
649752c [Form] Fixed: CSRF token was not displayed on empty complex forms
c623fcf [Form] Fixed: CSRF protection did not run if token was missing
eb75ab1 [Form] Fixed results of the FieldType+FormType merge.
Discussion
----------
[Form] Fixed errors introduced in the FieldType+FormType merge
Bug fix: yes
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #3994, #4000, #2294, #4118
Todo: -
---------------------------------------------------------------------------
by Tobion at 2012-04-22T15:39:20Z
`primitive` is a pretty abstract option name. It depends on the person what he considers primitive. Maybe more explicit naming or better documentation what it means.
---------------------------------------------------------------------------
by bschussek at 2012-04-22T15:47:29Z
Better suggestions?
The distinction here is between primitive and complex forms, where primitive forms are such forms that can be represented by a single HTML tag. This obviously needs to be documented.
---------------------------------------------------------------------------
by Tobion at 2012-04-22T15:49:45Z
Maybe `single_widget` or something like that.
---------------------------------------------------------------------------
by vicb at 2012-04-23T13:09:43Z
@Tobion @bschussek would `elementary` be better than `primitive` ?
---------------------------------------------------------------------------
by vicb at 2012-04-23T13:17:04Z
and `compound \ composite` better than `complex` ?
---------------------------------------------------------------------------
by bschussek at 2012-04-23T14:08:33Z
@vicb I fail to see how elementary/compound is easier to understand than primitive/complex. Maybe single_widget, but what's the opposite of this case? multi_widget?
---------------------------------------------------------------------------
by vicb at 2012-04-23T14:15:09Z
Actually I am fine with anything... as long as it is documented.
---------------------------------------------------------------------------
by bschussek at 2012-04-23T14:22:31Z
Still I think that this unveals a more profound naming problem. How do we (also in the documentation) name forms with children (formerly "forms") and forms without children (formerly "fields")?
Should we refer to them as
* forms and fields?
* complex and primitive forms?
* ...
We must first answer this question before we can find an intuitive option name. If the documentation always switches between different terminologies, neither will it be understandable nor will this option be easy to remember.
---------------------------------------------------------------------------
by vicb at 2012-04-23T15:10:32Z
> Still I think that this unveals a more profound naming problem. How do we (also in the documentation) name forms with children (formerly "forms") and forms without children (formerly "fields")?
To make it clear, I would rather say forms that **can have** children and forms that **can not have** children (i.e. Empty collections have no children but they can have and this is reason why you have to introduce those options, right ? - that could be a good example for the doc).
It will probably be better to refer to "complex" / "primitive" forms in the doc (and use the "form" / "field" terms to explain them).
Note: I think @Tobion concern is that "primitive" / "complex" could be pejorative terms (this is why I have proposed "elementary" / "compound").
---------------------------------------------------------------------------
by Tobion at 2012-04-23T16:00:54Z
1. primitive/complex is subjective (and could be pejorative too)
2. elementary/compound is more explicit so probably better than primitive/complex
3. I dislike this option in general. Does it make sense to change this option from a user perspective? I guess it's always the same as long as the widget structure stays the same. So it should be resolved at a higher level dynamically from the widget structure and not exposed to any configuration.
4. In documentation I would use the terms forms and fields. Because all people with HTML knowledge will understand that fields cannot have sub-fields whereas forms can. But since this distinction is not findable in code, it should be mentioned that all these are implemented as a form hierarchy.
---------------------------------------------------------------------------
by mvrhov at 2012-04-23T16:02:00Z
how about simple and complex?
---------------------------------------------------------------------------
by bschussek at 2012-04-23T16:06:33Z
@Tobion It does not make sense to change this option from the user perspective, still the overloading type has to propagate to FormType whether it is a form or a field, so that the default behaviour is correct.
A second option how to implement this is to add a method `isField` to FormTypeInterface that can be overloaded and receives the options. I don't really like to introduce new methods here unless absolutely required.
What about renaming the option "primitive" to "is_field"? The blocks in the template would then be named "form_widget_field" and "form_widget_form".
---------------------------------------------------------------------------
by tristanbes at 2012-04-25T14:01:06Z
Oh, I should've seen this before, i thought I was doing something wrong. (empty collections gets an input field bug)
Please big :UP: on this. When will it be merged ? @bschussek
---------------------------------------------------------------------------
by Tobion at 2012-04-25T15:30:28Z
+1 for "is_field" and "form_widget_field" but I would rather use "form_widget_compound" instead of "form_widget_form" which is quite strange.
---------------------------------------------------------------------------
by bschussek at 2012-04-26T16:34:04Z
@Tobion "simple" and "compound" then?
---------------------------------------------------------------------------
by Tobion at 2012-04-26T16:49:58Z
no "field" and "compound"
---------------------------------------------------------------------------
by bschussek at 2012-04-26T17:17:02Z
I don't like "field" for a simple reason: Consider the "date" type. We are typically speaking of the "date" field there. But technically, the "date" field is a compound field. So?
---------------------------------------------------------------------------
by Tobion at 2012-04-26T21:17:37Z
I don't understand the open question. You proposed "is_field" and "form_widget_field" yourself. So calling the template block "form_widget_field" is a comprehensible consequence of "is_field". I wouldn't call the date type with multiple inputs a field.
---------------------------------------------------------------------------
by tristanbes at 2012-04-26T21:52:39Z
We should take a decision cause right here i got all my forms that are broken because of the empty collection rendering as input field :-).
I guess we are many in that situation.
---------------------------------------------------------------------------
by bschussek at 2012-04-27T08:28:16Z
I renamed "primitive" to "single_control" now to match with the HTML specification which names all input elements (input, select etc.) "controls". The opposite is now "compound".
Meanwhile, I added a fix for #4118.
@fabpot This is ready for merge now.
---------------------------------------------------------------------------
by Tobion at 2012-04-27T10:22:49Z
Hm, I know naming things is hard and sometimes not really important. But since users need to know which block to override, it is essential to make it clear. I think there is still one issue.
The block is named `form_widget_single_control` in order, as you said, to abstract away if it's an input, select etc. But in fact it can only render `input` and nothing else. So this is misleading.
So you could also simply name it `form_widget_input`.
Apart from that I agree with everything.
Commits
-------
6756f28 [Session] Fixed Backward Compatibility issue with getFlashes()
Discussion
----------
[Session] Fixed Backward Compatibility issue with getFlashes()
---------------------------------------------------------------------------
by fabpot at 2012-04-25T22:35:42Z
ping @drak
---------------------------------------------------------------------------
by willdurand at 2012-04-25T22:37:01Z
By the way, I had this issue on a real application I upgraded from Symfony2 2.0.x to 2.1 (and written by @Seldaek)
The code looks like:
``` php
<?php
// in a controller
$this->session->setFlash('foo', array(
'code' => 'success',
'message' => 'lalala',
'params' => array())
);
```
---------------------------------------------------------------------------
by Seldaek at 2012-04-26T07:25:03Z
Yup, to be fair in retrospective maybe that should have been translated in the controller directly (that's why it had message + params as an array), but this is code that predates 2.0 by at least six months, so it was obviously not clear what best practices were. Anyway it seems it can be fixed without much harm, so for the sake of safety and because I may not be the only crazy person having done this, it'd be good to fix IMO.
Commits
-------
1c03a16 [Process] Fixed ProcessFailedException not populating exception message due to a missing sprintf parameter
Discussion
----------
[Process] Fixed ProcessFailedException not populating exception message ...
...due to a missing sprintf parameter
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: http://travis-ci.org/#!/proofek/symfony/builds/1172817
Fixes the following tickets: -
Todo: -
Found the issue when started using Cilex with Process and raised an exception using ProcessFailedException. Not sure whether this will get into the main release, as I couldn't find that on 2.0 branch, so I am guessing it's quite a recent addition to Process component.
Commits
-------
4171305 [Console] Use proc_open instead of exec to suppress errors when run on windows and stty is not present
Discussion
----------
[Console] Use proc_open instead of exec to suppress errors.
stty is *sometimes* there on windows, but not always, so with proc_open we can quietly return null instead of outputting errors.
/cc @gnutix: that's the error you told me about this morning in https://gist.github.com/2478037
---------------------------------------------------------------------------
by maoueh at 2012-04-24T17:46:25Z
Thx, I'm getting this in my output each time an exception is thrown in CLI. Good this should fix it :)
Commits
-------
9f0daf4 put parentheses back
885104c fix typo
Discussion
----------
fix typo
Fix typo introduced in #4069
Past participle of `read` is `read`
Commits
-------
2e7d3b1 http_build_query fix
de73de0 http_build_query fix
3b7ee9a http_build_query fix
Discussion
----------
[2.0] http_build_query extra parameters
Bug fix: yes
arg_separator.output is not always "&", so it is better ini_set it or put an extra parameters to http_build_query
---------------------------------------------------------------------------
by fabpot at 2012-04-23T10:20:05Z
Can you squash your commits? It will be much easier to get back to this change later on. Thanks.
---------------------------------------------------------------------------
by Ziumin at 2012-04-23T10:46:35Z
I have no idea how to do it using web interface. I'm not familiar with git (prefer hg). Sorry.
Commits
-------
f7200e4 [Form] added method `guessPattern` to FormTypeGuesserInterface
Discussion
----------
[Form] add guess pattern
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: https://github.com/symfony/symfony/issues/3766
Todo: -
Due to some trouble when rebase my previous PR i open a new one with Master merged
Refs PR: https://github.com/symfony/symfony/pull/3927
---------------------------------------------------------------------------
by fabpot at 2012-04-23T10:25:57Z
@vicb @bschussek ok for you?
---------------------------------------------------------------------------
by bschussek at 2012-04-23T10:26:51Z
please do also rephrase the commit message to something clearer, like
[Form] added method `guessPattern` to FormTypeGuesserInterface
---------------------------------------------------------------------------
by bschussek at 2012-04-23T10:27:35Z
Otherwise this looks good :)
---------------------------------------------------------------------------
by ruian at 2012-04-23T10:29:18Z
every changes done
Commits
-------
40df3bf Add mongodb session storage
Discussion
----------
[HttpFoundation][Session] Add mongodb session storage
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
---------------------------------------------------------------------------
by Baachi at 2012-04-19T19:05:19Z
Review please :)
---------------------------------------------------------------------------
by Baachi at 2012-04-19T19:49:42Z
@stof Can be merged?
---------------------------------------------------------------------------
by stof at 2012-04-19T19:51:28Z
I'm not a Mongo expert but it seems fine. You simply need to wait @fabpot's final review now
---------------------------------------------------------------------------
by Baachi at 2012-04-19T19:52:53Z
Okay, thanks :)
---------------------------------------------------------------------------
by Baachi at 2012-04-20T06:21:52Z
@vicb Sorry, for the email flood :)
I implemented all your suggestions.
---------------------------------------------------------------------------
by fabpot at 2012-04-22T08:27:19Z
@drak, @vicb: Is it ok now?
---------------------------------------------------------------------------
by vicb at 2012-04-22T08:33:31Z
I am ok with this PR
Commits
-------
e3296cb fix php5.4 problem
c2405c0 fix hanging of unit tests on Windows
Discussion
----------
[WIP] [Process] Fix hanging of unit tests on Windows
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3798
Todo: -
This PR tries to fix hanging unit tests on Windows platform. The problem is caused by known [PHP bug](https://bugs.php.net/bug.php?id=51800). PHP hangs forever while reading from STDOUT pipe if the output is too big.
I tried different combinatios and this is not ideal, but a working patch - STDOUT is sending to a file.
@Tobion, @drak and other developers working on Windows - can you please confirm, if this solution works for you, too?
---------------------------------------------------------------------------
by Seldaek at 2012-04-22T20:56:20Z
Tried it on 5.4.0 - I get the following when I checkout your branch:
```
SS..........ESSSSSSSSSS...E.S
Time: 0 seconds, Memory: 19.75Mb
There were 2 errors:
1) Symfony\Component\Process\Tests\ProcessTest::testProcessResponses with data set #0 ('output', 'getOutput', 'echo \'output\';')
Undefined offset: 1
C:\Users\seld\Web\symfony\framework\src\Symfony\Component\Process\Process.php:342
C:\Users\seld\Web\symfony\framework\src\Symfony\Component\Process\Process.php:168
C:\Users\seld\Web\symfony\framework\src\Symfony\Component\Process\Tests\ProcessTest.php:46
2) Symfony\Component\Process\Tests\ProcessTest::testIsRunning
Undefined offset: 1
C:\Users\seld\Web\symfony\framework\src\Symfony\Component\Process\Process.php:342
C:\Users\seld\Web\symfony\framework\src\Symfony\Component\Process\Tests\ProcessTest.php:106
```
When I remove the skipping of `ProcessTest::testProcessPipes` - it still hangs so it looks like your fix is not working.
Just for reference, with latest master checkout I get this:
```
SS...........SSSSSSSSSS.....S
Time: 2 seconds, Memory: 19.75Mb
OK, but incomplete or skipped tests!
```
Don't have time right now, but if I find time to look at your diff in more details I will, maybe it's possible to fix it.
---------------------------------------------------------------------------
by pulzarraider at 2012-04-22T22:23:05Z
@Seldaek Thanks, fixed php5.4 problem, my php5.3.8 passed every Process tests.
This patch isn't fixing problem with pipes in general. I don't think it's even possible from PHP code. We have to wait for PHP core developers to fix this problem. This patch only fix issue with hanging unit tests (not the one with pipes you mentioned). In my environment, symfony unit tests never finished and hangs on SwitchUserTest from SecurityBundle on 98%. Now with this patch, I can see final informations about all tests and their errors.
---------------------------------------------------------------------------
by Seldaek at 2012-04-23T07:44:16Z
Ok, it passes again on 5.4.
Commits
-------
bc8855e [2.1][Component][ClassLoader] cs
Discussion
----------
[2.1][Component][ClassLoader] cs
Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
---------------------------------------------------------------------------
by fabpot at 2012-04-23T06:18:26Z
Can you please remove the changes you have already made in the other PR as I merge 2.0 into master regularly. Then, you will need to squash you commits to avoid any conflict when merging will occur. Thanks.
---------------------------------------------------------------------------
by gajdaw at 2012-04-23T06:50:58Z
I hope that's it.
Commits
-------
e344609 [DependencyInjection] Fixed composer.json
1aa0786 [FrameworkBundle] Fixed composer.json
3601f61 [DoctrineBridge] Fixed composer.json
Discussion
----------
Fix composer json
---------------------------------------------------------------------------
by jalliot at 2012-04-22T14:22:24Z
`suggest` no longer requires a version constraint. While you're at it, maybe you could change those to more meaningful strings explaining what each optional dependency provides.
---------------------------------------------------------------------------
by willdurand at 2012-04-22T14:24:27Z
I know, but the version is fine. It's more up to @fabpot to add description in each `suggest` entries.
Anyway, this is not the purpose of this PR. If you want to contribute on that, feel free :)
Commits
-------
e509e6f Skip PDOSessionHandlerTest if PDO SQLite is not available
Discussion
----------
Skip PDOSessionHandlerTest if PDO SQLite is not available
Commits
-------
128ac26 Added missing '%' in DI component README
Discussion
----------
Added missing '%' in DI component README
---------------------------------------------------------------------------
by ruian at 2012-04-21T10:37:12Z
@fabpot PR ok
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #1813
Todo: -
In order to work, add this to the .htaccess:
RewriteEngine on
RewriteRule .* - [E=HTTP_AUTHORIZATION:%{HTTP:Authorization}]
RewriteCond %{REQUEST_FILENAME} !-f
RewriteRule ^(.*)$ app.php [QSA,L]
Commits
-------
218813c [Finder] contains(), notContains()
33e119a Merge branch 'master' of https://github.com/symfony/symfony into finder_search_by_contents
082d86e [Finder] content(), notContent() methods
Discussion
----------
[Finder] search by contents
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Sometimes I need to search for files containing some text:
```
$finder
->content('lorem')->notContent('ipsum')
->content('/^Begining/m')->notContent('/the end$/m');
```
I don't know how to tests exceptions thrown by `file_get_contents()` calls.
---------------------------------------------------------------------------
by gajdaw at 2012-04-19T15:53:05Z
To keep it as close as possible to `name` and `notName`.
Commits
-------
94bee7a [Filesystem] symlink() creates target directories
Discussion
----------
[Filesystem] symlink() creates target directories
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes [](http://travis-ci.org/michal-pipa/symfony)
Fixes the following tickets: #3967
Todo: -
Changed symlink() method behavior to recursively create target directory if it does not exist. It makes Filesystem component methods more consistent since copy() does the same. It is also more convenient.
Also mirror() fails to create symlink in non-existent directory (if we don't want to change symlink(), than we need to fix mirror()).
Fixes: #3967
Commits
-------
1c290d7 Add unit tests for FlattenException::getLine() and FlattenException::getFile().
a22f0cd Enhance FlattenException to include more methods from Exception. That allows it to be used in place of Exception in more places.
Discussion
----------
[HttpKernel] Enhance FlattenException to include more methods from Exception.
I'm trying to retrofit FlattenException into Drupal, in places where Drupal expects an Exception. That doesn't quite work though, as FlattenException only has some of the methods from Exception. I'm not entirely clear why it only has some, but this PR adds getFile() and getLine() so that it's a more ready drop-in. I did not add them to the toArray() method for fear of breaking BC somewhere, but that could be done as well no doubt if folks felt it was appropriate.
Note: While the parts of Drupal in question will get rewritten later anyway, I think having this information exposed is a good thing in general for logging purposes if nothing else. It's already possible to dig it out of the trace, so this is just an improved "Developer eXperience" (DX).
---------------------------------------------------------------------------
by fabpot at 2012-04-20T04:34:54Z
I'm +1 to make `FlattenException` more "compatible" with `Exception`. Can you add the other missing methods? Also, you need to populate the `$this->file` and `$this->line` value in the constructor.
---------------------------------------------------------------------------
by Crell at 2012-04-20T04:48:40Z
I knew I was forgetting something obvious...
According to http://us.php.net/manual/en/class.exception.php, I think the only other missing method is http://us.php.net/manual/en/exception.gettraceasstring.php. I'm not sure how useful that is, but I can try to approximate it if you think it's necessary. (Honestly I've never used that method on an exception myself.)
I should probably add some tests, too. Stand by for those.
---------------------------------------------------------------------------
by Crell at 2012-04-20T05:00:28Z
Now includes unit tests to make sure I didn't do anything stupid this time. I'll hold off on getTraceAsString() for now unless you think it's needed. (I'm not sure it is since it's harder to do and IMO less useful.)
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
-------
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
-------
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?
CSRF fields are now only added when the view is built. For this reason we already know if
the form is the root form and avoid to create unnecessary CSRF fields for nested fields.
Commits
-------
b1ea552 [Locale] micro-optimization
663d218 [Locale] changed method name
bb61e09 [Locale] use the correct way for Intl error
Discussion
----------
[2.0][Locale] rebased PR 3765 plus few changes
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
#3765 was right but was made in master. Cherry-picked and rebased for 2.0.
Tests are passing.
Commits
-------
05b2238 [DependencyInjection] Fix for issue introduced in 3ae826a
Discussion
----------
[2.0][DependencyInjection] Fix for issue introduced in 3ae826a
Bug fix: yes
Tests pass: 
Fix for issue introduced in 3ae826a
---------------------------------------------------------------------------
by eriksencosta at 2012-04-14T21:08:40Z
@fabpot The 2.0 test suite is broken without this fix.
Commits
-------
80f96b7 [DependencyInjection] Add ability to clear tags by name.
Discussion
----------
[DependencyInjection] Add ability to clear tags by name.
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
---------------------------------------------------------------------------
by jalliot at 2012-04-14T07:50:51Z
@drak Just for information, what is the use case?
---------------------------------------------------------------------------
by drak at 2012-04-14T08:40:13Z
I'm using this to filter (and prevent) some listeners from being loaded
into the dispatcher.
---------------------------------------------------------------------------
by lsmith77 at 2012-04-14T09:37:39Z
tags are used by compiler passes. bundles that set these tags expect some defined bundle to take some specific actions on the given services. as such this is already a fairly "fragile" relationship. once other compiler passes then start messing with these tags it can get even more tricky. then again, as long as you know what you are doing ..
that being said .. this method just adds convenience, since one could always just get all the tags, unset and reset them in bulk.
---------------------------------------------------------------------------
by drak at 2012-04-14T09:42:04Z
@lsmith77 - exactly.
Commits
-------
01fcb08 [HttpKernel] Fix the ProfilerListener (fix#3620)
Discussion
----------
[HttpKernel] Fix the ProfilerListener (fix#3620)
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: [](http://travis-ci.org/vicb/symfony)
Fixes the following tickets: #3620, PR #3618
Many thanks to @guilhermeblanco for helping with the tests.
---------------------------------------------------------------------------
by vicb at 2012-04-13T20:10:15Z
For ref: that's basically a backport of #3920 for 2.0
