Commits
-------
382b083 [Routing] improved generated class by PhpGeneratorDumper
27a05f4 [Routing] small optimization of PhpGeneratorDumper
Discussion
----------
[Routing] improved PhpGeneratorDumper
Test pass: yes
BC break: no
The first commit only replaces arrays with strings and makes some cosmetic changes, so that it's more readable. This makes the `PhpGeneratorDumper` consistent in style with `PhpMatcherDumper` that I fixed recently and should slightly improve performance of the generation of the class.
The second commit changes the output of the `PhpGeneratorDumper->dump` and tries to optimize the resulting class that is used to generate URLs. It's best explained with an example.
Before my changes:
```php
class ProjectUrlGenerator extends Symfony\Component\Routing\Generator\UrlGenerator
{
static private $declaredRouteNames = array(
'Test' => true,
'Test2' => true,
);
/**
* Constructor.
*/
public function __construct(RequestContext $context)
{
$this->context = $context;
}
public function generate($name, $parameters = array(), $absolute = false)
{
if (!isset(self::$declaredRouteNames[$name])) {
throw new RouteNotFoundException(sprintf('Route "%s" does not exist.', $name));
}
$escapedName = str_replace('.', '__', $name);
list($variables, $defaults, $requirements, $tokens) = $this->{'get'.$escapedName.'RouteInfo'}();
return $this->doGenerate($variables, $defaults, $requirements, $tokens, $parameters, $name, $absolute);
}
private function getTestRouteInfo()
{
return array(array ( 0 => 'foo',), array (), array (), array ( 0 => array ( 0 => 'variable', 1 => '/', 2 => '[^/]+?', 3 => 'foo', ), 1 => array ( 0 => 'text', 1 => '/testing', ),));
}
private function getTest2RouteInfo()
{
return array(array (), array (), array (), array ( 0 => array ( 0 => 'text', 1 => '/testing2', ),));
}
}
```
After my changes in second commit:
```php
class ProjectUrlGenerator extends Symfony\Component\Routing\Generator\UrlGenerator
{
static private $declaredRoutes = array(
'Test' => array ( 0 => array ( 0 => 'foo', ), 1 => array ( ), 2 => array ( ), 3 => array ( 0 => array ( 0 => 'variable', 1 => '/', 2 => '[^/]+?', 3 => 'foo', ), 1 => array ( 0 => 'text', 1 => '/testing', ), ),),
'Test2' => array ( 0 => array ( ), 1 => array ( ), 2 => array ( ), 3 => array ( 0 => array ( 0 => 'text', 1 => '/testing2', ), ),),
);
/**
* Constructor.
*/
public function __construct(RequestContext $context)
{
$this->context = $context;
}
public function generate($name, $parameters = array(), $absolute = false)
{
if (!isset(self::$declaredRoutes[$name])) {
throw new RouteNotFoundException(sprintf('Route "%s" does not exist.', $name));
}
list($variables, $defaults, $requirements, $tokens) = self::$declaredRoutes[$name];
return $this->doGenerate($variables, $defaults, $requirements, $tokens, $parameters, $name, $absolute);
}
}
```
As you can see, there is no need to escape the route name and invoke a special method anymore. Instead the route properties are included in the static route array directly, that existed anyway. Is also easier to read as defined routes and their properties are in the same place.
Commits
-------
03d30fd [Routing] remove duplicated cache of compiled routes
Discussion
----------
[Routing] remove duplicated cache of compiled routes
The UrlGenerator caches compiled routes for generating URLs. But the Route class caches it's compiled route itself as long as it does not get modified. So compiled routes are cached twice which makes no sense in my opinion.
Test pass: yes
BC break: no
Commits
-------
89a5c1a [process] Added destructor to process to make sure handles are always closed in the right order.
Discussion
----------
[process] Added destructor to process to make sure handles are always cl...
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Commits
-------
6f56dfc [Form] Fixed DateType default options
779d3bb [Form] Fixed documentation, and the DateType (default options)
Discussion
----------
[Form] Fixed documentation, and the DateType (default options)
---------------------------------------------------------------------------
by fabpot at 2012-04-11T16:48:04Z
That breaks the tests.
---------------------------------------------------------------------------
by willdurand at 2012-04-11T16:50:35Z
I got an error with the Form test suite before to write this patch..
---------------------------------------------------------------------------
by willdurand at 2012-04-11T16:53:30Z
Nevermind, I can see broken tests.. I'm on, sorry
---------------------------------------------------------------------------
by willdurand at 2012-04-11T16:57:52Z
@fabpot fixed.
```
OK, but incomplete or skipped tests!
Tests: 945, Assertions: 1439, Incomplete: 11.
```
In order to perform some introspection on a FormBuilder instance,
we need to be able to get its children. Almost everything is accessible
in this class, but the children are not.
This PR adds a all() method to respect the current API (get(), remove(),
...).
Commits
-------
8329087 [Form] Moved calculation of ChoiceType options to closures
5adec19 [Form] Fixed typos
cb87ccb [Form] Failing test for empty_data option BC break
b733045 [Form] Fixed option support in Form component
Discussion
----------
[Form] Fixed option support in Form component
Bug fix: yes
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #3354, #3512, #3685, #3694
Todo: -
This PR also introduces a new helper `DefaultOptions` for solving option graphs. It accepts default options to be defined on various layers of your class hierarchy. These options can then be merged with the options passed by the user. This is called *resolving*.
The important feature of this utility is that it lets you define *lazy options*. Lazy options are specified using closures that are evaluated when resolving and thus have access to the resolved values of other (potentially lazy) options. The class detects cyclic option dependencies and fails with an exception in this case.
For more information, check the inline documentation of the `DefaultOptions` class and the UPGRADE file.
@fabpot: Might this be worth a separate component? (in total the utility consists of five classes with two associated tests)
---------------------------------------------------------------------------
by beberlei at 2012-04-05T08:54:10Z
"The important feature of this utility is that it lets you define lazy options. Lazy options are specified using closures"
What about options that are closures? are those differentiated?
---------------------------------------------------------------------------
by bschussek at 2012-04-05T08:57:35Z
@beberlei Yes. Closures for lazy options receive a Symfony\Component\Form\Options instance as first argument. All other closures are interpreted as normal values.
---------------------------------------------------------------------------
by stof at 2012-04-05T11:09:49Z
I'm wondering if these classes should go in the Config component. My issue with it is that it would add a required dependency to the Config component and that the Config component mixes many different things in it already (the loader part, the resource part, the definition part...)
---------------------------------------------------------------------------
by sstok at 2012-04-06T13:36:36Z
Sharing the Options class would be great, and its more then one class so why not give it its own Component folder?
Filesystem is just one class, and that has its own folder.
Great job on the class bschussek 👏
---------------------------------------------------------------------------
by bschussek at 2012-04-10T12:32:34Z
@fabpot Any input?
---------------------------------------------------------------------------
by bschussek at 2012-04-10T13:54:13Z
@fabpot Apart from the decision about the final location of DefaultOptions et al., could you merge this soon? This would make my work a bit easier since this one is a blocker.
---------------------------------------------------------------------------
by fabpot at 2012-04-10T18:08:18Z
@bschussek: Can you rebase on master? I will merge afterwards. Thanks.
This demonstrates the issue described in symfony/symfony#3354. FieldType no longer has access to the child type's data_class option, which makes it unable to create the default closure for empty_data.
Commits
-------
f9a486e [Validator] Added support for pluralization of the SizeLengthValidator
c0715f1 [FrameworkBundle], [TwigBundle] added support for form error message pluralization
7a6376e [Form] added support for error message pluralization
345981f [Validator] added support for plural messages
Discussion
----------
[Validator] Added support for plural error messages
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Todo: create translations for en and update others (FrameworkBundle)
[](http://travis-ci.org/hason/symfony)
---------------------------------------------------------------------------
by fabpot at 2011-05-14T20:41:01Z
@bschussek: What's your opinion?
---------------------------------------------------------------------------
by stof at 2011-09-04T13:14:29Z
@hason could you rebase your branch on top of master and update the PR ?
You also need to change the messages in the constraint that uses the pluralization to a pluralized format.
---------------------------------------------------------------------------
by stof at 2011-10-16T18:06:22Z
@hason ping
---------------------------------------------------------------------------
by stof at 2011-11-11T14:58:19Z
@hason ping again
---------------------------------------------------------------------------
by stof at 2011-12-12T20:39:10Z
@hason ping again. Can you update your PR ?
---------------------------------------------------------------------------
by hason at 2011-12-12T21:29:14Z
@stof I hope that I will update PR this week.
---------------------------------------------------------------------------
by bschussek at 2012-01-15T19:07:32Z
Looks good to me.
---------------------------------------------------------------------------
by canni at 2012-02-02T17:28:54Z
@hason can you update this PR and squash commits, it conflicts with current master
---------------------------------------------------------------------------
by hason at 2012-02-09T07:21:41Z
@stof, @canni Rebased.
What is the best solution for the translation of messages?
1. Change messages in the classes and all xliff files?
2. Keep messages in the classes and change all xliff files?
---------------------------------------------------------------------------
by stof at 2012-02-09T08:19:41Z
The constraints contain the en message so you will need to modify them to update the message
---------------------------------------------------------------------------
by hason at 2012-02-09T08:55:55Z
I prefer second option. The Validator component should be decoupled from the Translation component. The constraints contain the en message which is also the key for Translation component. We should create validators.en.xlf in the FrameworkBundle for en message. I think that this is better solution. What do you think?
---------------------------------------------------------------------------
by stof at 2012-04-04T02:22:02Z
@hason Please rebase your branch. It conflicts with master because of the move of the tests
@fabpot ping
Commits
-------
efad5d5 [Filesystem] Prevented infiite loop on windows while calling mirror on symlink. Added test for mirroring symlinks.
Discussion
----------
[Filesystem] Prevented infinite loop on windows while mirrorring symlinks
First check for filetype in *mirror()* method is:
if (is_link($file)) {
$this->symlink($file, $target);
later we see:
} elseif (is_file($file) || ($copyOnWindows && is_link($file))) {
$this->copy($file, $target, isset($options['override']) ? $options['override'] : false);
The later check for links on windows (*$copyOnWindows && is_link($file)*) won't ever get called. Calling *symlink()* in *mirror()* on windows would lead to calling *mirror()* again.
Note that I didn't actually try running it on windows platform. I added a test for mirroring symlinks (non-windows test). I think it'd be good if someone added some windows specific tests to this class.
I also modified the target path:
$target = $targetDir.'/'.str_replace($originDir.DIRECTORY_SEPARATOR, '', $file->getPathname());
It didn't use DIRECTORY_SEPARATOR and is equivalent to:
$target = str_replace($originDir, $targetDir, $file->getPathname());
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: ~
Todo: ~
Commits
-------
9307f5b [Routing] Implement bug fixes and enhancements
Discussion
----------
[Routing] Implement bug fixes and enhancements (from @Tobion)
This is mainly #3754 with some minor formatting changes.
Original PR message from @Tobion:
Here a list of what is fixed. Tests pass.
1. The `RouteCollection` states
> it overrides existing routes with the same name defined in the instance or its children and parents.
But this is not true for `->addCollection()` but only for `->add()`. addCollection does not remove routes with the same name in its parents (only in its children). I don't think this is on purpose.
So I fixed it by making sure there can only be one route per name in all connected collections. This way we can also simplify `->get()` and `->remove()` and improve performance since we can stop iterating recursively when we found the first and only route with a given name.
See `testUniqueRouteWithGivenName` that fails in the old code but works now.
2. There was an bug with `$collection->addPrefix('0');` that didn't apply the starting slash. Fixed and test case added.
3. There is an issue with `->get()` that I also think is not intended. Currently it allows to access a sub-RouteCollection by specifing $name as array integer index. But according to the PHPdoc you should only be allowed to receive a Route instance and not a RouteCollection.
See `testGet` that has a failing test case. I fixed this behavior.
4. Then I recognized that `->addCollection` depended on the order of applying them. So
$collection1->addCollection($collection2, '/b');
$collection2->addCollection($collection3, '/c');
$rootCollection->addCollection($collection1, '/a');
had a different pattern result from
$collection2->addCollection($collection3, '/c');
$collection1->addCollection($collection2, '/b');
$rootCollection->addCollection($collection1, '/a');
Fixed and test case added. See `testPatternDoesNotChangeWhenDefinitionOrderChanges`.
5. PHP could have ended in an infinite loop when one tried to add an existing RouteCollection to the tree. Fixed by throwing an exception when this situation is detected. See tests `testCannotSelfJoinCollection` and `testCannotAddExistingCollectionToTree`.
6. I made `setParent()` private because its not useful outside the class itself. And `remove()` also removes the route from its parents. Added public `getRoot()` method.
7. The `Route` class throwed a PHP warning when trying to set an empty requirement.
8. Fixed issue #3777. See discussion there for more info. I fixed it by removing the over-optimization that was introduced in 91f4097a09 but didn't work properly. One cannot reorder the route definitions, as is was done, because then the wrong route might me matched before the correct one. If one really wanted to do that, it would require to calculate the intersection of two regular expressions to determine if they can be grouped together (a tool that would also be useful to check whether a route is unreachable, see discussion in #3678) We can only safely optimize routes with a static prefix within a RouteCollection, not across multiple RouteCollections with different parents. This is especially true when using variables and regular expressions requirements.
I could however apply an optimization that was missing yet: Collections with a single route were missing the static prefix optimization with `0 === strpos()`.
9. Fixed an issue where the `PhpMatcherDumper` would not apply the optimization if the root collection to be dumped has a prefix itself. For this I had to rewrite `compileRoutes`. It is also much easier to understand now. Addionally I added many comments and PHPdoc because complex recursive methods like this are still hard to grasp.
I added a test case for this (`url_matcher3.php`).
10. Fix that `Route::compile` needs to recompile a route once it is modified. Otherwise we have a wrong result. Test case added.
Commits
-------
f617e02 [Validator] added less-strict email host verification
Discussion
----------
[Validator] added less-strict email host verification
uhhhhh, my first pull request :>. uhm... tell me if i did something wrong :)
#### Request info
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes... well,not really (i guess master branch is not passing - at least i didnt broke the email test)
Fixes the following tickets: #2827
#### Description
New checkHost attribute in email constraint will make the validator check for only one of MX, A or AAAA DNS resource records to verify it as a valid email address.
Commits
-------
039ff6f allow more control on GetSetMethodNormalizer by using callback functions and an ignoreAttributes list
Discussion
----------
allow more control on GetSetMethodNormalizer
Here is an other attempt. You would use this as follows:
$serializer = new \Symfony\Component\Serializer\Serializer(
array(new \Symfony\Component\Serializer\Normalizer\GetSetMethodNormalizer()),
array('json' => new \Symfony\Component\Serializer\Encoder\JsonEncoder())
);
$callbacks = array('books' => function ($books) { return NULL; });
return new Response(
$serializer->serialize($paginator->getRows(), 'json', $callbacks),
200,
array('Content-Type' => 'application/json')
);
Besides of returning NULL, you could also do things like:
$callbacks = array(
'books' => function ($books) {
$ids = array();
foreach ($books as $book) {
$ids[] = $book->getId();
}
return $ids;
},
'author' => function ($author) {
return $author->getId();
},
'creationDate' => function ($creationDate) {
return $creationDate->format('d/m/Y');
},
);
The commit is not complete yet. But at this point I am interested in your opinions.
---------------------------------------------------------------------------
by lsmith77 at 2012-03-12T22:53:18Z
in general i agree that using a callback is a good solution to provide more power without complicating the API or implementation in this case.
please add a test case, this should also help illustrate how this can be used in practice.
---------------------------------------------------------------------------
by schmittjoh at 2012-03-13T04:54:33Z
Note that your change breaks the API defined by the interface, i.e. someone using this method needs to type-hint the serializer implementation, not the interface.
It also adds a parameter to the public API of the serializer which will only work with one specific normalizer. What if another normalizer needs additional information, should another parameter be added to the serialize method? What about deserialization?
Bottom line is, the serializer component was simply not designed for this kind of thing. I've tried to make it more flexible before creating the bundle, but some things simply cannot be fixed in a sane way.
---------------------------------------------------------------------------
by tvlooy at 2012-03-13T06:07:45Z
Would just adding a setCallbacks() to the GetSetMethodNormalizer be a better solution? That doesn't touch the API. I will try to write some tests this evening.
---------------------------------------------------------------------------
by schmittjoh at 2012-03-13T16:22:50Z
That would definitely be better.
You would then need to retrieve the normalizer instance before calling ``serialize`` on the serializer which also leaves a stale taste, but I have no other solution for now.
---------------------------------------------------------------------------
by tvlooy at 2012-03-13T21:32:26Z
So, this should be it then. Yet an other usage example:
$normalizer = new \Symfony\Component\Serializer\Normalizer\GetSetMethodNormalizer();
$normalizer->setCallbacks(
array(
'books' => function ($books) {
$ids = array();
foreach ($books as $book) {
$ids[] = $book->getId();
}
return $ids;
},
)
);
$serializer = new \Symfony\Component\Serializer\Serializer(
array($normalizer),
array('json' => new \Symfony\Component\Serializer\Encoder\JsonEncoder())
);
return new Response(
$serializer->serialize($paginator->getRows(), 'json'),
200,
array('Content-Type' => 'application/json')
);
---------------------------------------------------------------------------
by tvlooy at 2012-03-18T21:16:48Z
Anything else needed for this to get pulled in?
---------------------------------------------------------------------------
by tvlooy at 2012-03-19T18:33:58Z
Hm, I like to keep it that way because I like the fact that not passing a callable will result in a warning instead of silently skipping it. You don't get that behaviour by treating it as null.
---------------------------------------------------------------------------
by vicb at 2012-03-19T23:15:37Z
I was unclear: the code should throw an exception when an element is not callable, this is why `null` will not be supported any more (it is not a callback as the `setCallbacks` indicate).
They are several way to support the former behavior:
* the cb can return a defined interface,
* the cb can throw a defines exc,
* by adding a `setIgnoredAttributes` method
Please also squash your commits.
---------------------------------------------------------------------------
by tvlooy at 2012-03-20T21:02:06Z
Yes, I like the setIgnoredAttributes solution. I changed it and squashed the commits.
---------------------------------------------------------------------------
by tvlooy at 2012-03-26T20:07:36Z
some improvements and squashed the commits
---------------------------------------------------------------------------
by stof at 2012-04-03T22:36:15Z
@tvlooy Please rebase your branch. It conflicts with master because of the move of tests.
---------------------------------------------------------------------------
by tvlooy at 2012-04-04T07:43:47Z
@stof I will do it on saturday, if that is ok with you.
---------------------------------------------------------------------------
by fabpot at 2012-04-10T18:29:30Z
Is it mergeable now? ping @Seldaek, @schmittjoh.
---------------------------------------------------------------------------
by tvlooy at 2012-04-10T18:55:04Z
yes, it should be
Commits
-------
c36651b Fixed spelling error
f123684 Removed leftover from c/p
b74a5d4 Updated to new cache loader pattern.
7e66908 Added XCache class loader
Discussion
----------
[ClassLoader] Added XCache class loader
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
There is no tests, as it seems there is no way to use xcache storage functions from CLI.
---------------------------------------------------------------------------
by stof at 2012-04-06T20:12:09Z
Please implement a XcacheClassLoader following the same pattern than the new ApcClassLoader instead
---------------------------------------------------------------------------
by cordoval at 2012-04-07T14:20:47Z
- should include tests
- should include documentation (will you also update the component documentation for this new class?)
---------------------------------------------------------------------------
by stof at 2012-04-07T14:25:00Z
@cordoval the PR explains why there is no tests: xcache canot be used in the CLI.
---------------------------------------------------------------------------
by cordoval at 2012-04-07T14:26:43Z
ok @stof sorry it said it seemed not to be possible, i thought it was possible but I am wrong.
---------------------------------------------------------------------------
by kimhemsoe at 2012-04-07T15:01:24Z
@cordoval My english is really horrible. I would not mind if someone else could do that task for me. We also need to add doc for the new ApcClassLoader.
---------------------------------------------------------------------------
by cordoval at 2012-04-07T15:03:57Z
I wish you can explain me more then about this class and how to use it in code so then I can write easily the documentation :D deal?
---------------------------------------------------------------------------
by kimhemsoe at 2012-04-07T15:21:25Z
Deal :P
The XcacheClassLoader and ApcClassLoader replaces the old ApcUniversalClassLoader.
They giving us support for using another loader then UniversalClassLoader, without duplicating the cache layer.
Aslong it have a public function findFile($class) method.
$loader = new ClassLoader();
// register classes with namespaces
$loader->add('Symfony\Component', __DIR__.'/component');
$loader->add('Symfony', __DIR__.'/framework');
$cachedLoader = new XcacheClassLoader('my_prefix', $loader);
// activate the cached autoloader
$cachedLoader->register();
Think that is more or less the essence of this.
---------------------------------------------------------------------------
by cordoval at 2012-04-09T08:28:53Z
it is not add but registerNamespace right?
so the main idea is to get rid of the restriction to use Apc with Universal loader
what is the comparative advantage between APC and Xcache?
---------------------------------------------------------------------------
by kimhemsoe at 2012-04-09T08:55:23Z
Yes if the $loader (class finder) were to be a instance UniversalClassLoader.
Yes the main idea is to be able to reuse the cache layer with any class loader there obey to the one restriction.
Difference between apc and xcache and why to use what is coming down to taste and your setup. We use xcache as APC have some issues in fastcgi setups. when we upgrade to php54 at somepoint we get to chance to move to php-fpm wich solves these issues. Short story: Slightly out of scope for any documentation in here :-P
Commits
-------
77185e0 [Routing] Allow spaces in the script name for the apache dumper
6465a69 [Routing] Fixes to handle spaces in route pattern
Discussion
----------
[Routing] Handling of space characters in the dumpers
The compiler was using the 'x' modifier in order to ignore extra spaces and line feeds but the code was flawed:
- it was actually ignoring all the spaces, not only the extra ones added by the compiler,
- all the spaces were stripped in the php and apache matchers.
The proposed fix:
- do not use the 'x' modifier any more (and then do no add extra spaces / line feeds),
- do not strip the spaces in the matchers,
- escapes the spaces (both in regexs and script name) for the apache matcher.
It also include [a small optimization](https://github.com/vicb/symfony/pull/new#L9L89) when the only token of a route is an optional variable token - the idea is to make the regex easier to read.
---------------------------------------------------------------------------
by vicb at 2012-04-10T13:59:45Z
@Baachi fixed now. Thanks.
---------------------------------------------------------------------------
by Tobion at 2012-04-10T16:01:31Z
+1, I saw no reason for pretty printing the regex in the first place (just for debugging I guess).
@vicb since you want to make the regex easier to read, I propose the remove the `P` from the variable regex `?P<bar>`, which is not needed anymore in PHP 5.3 (and we only support PHP 5.3+ anyway).
---------------------------------------------------------------------------
by vicb at 2012-04-10T16:08:36Z
@Tobion could you make a PR to this branch for the named parameters ?
---------------------------------------------------------------------------
by Tobion at 2012-04-10T16:12:34Z
I can include it in #3754 because I'm about the add 2 more fixes to it anyway.
But when I proposed to apply these fixes to 2.0 Fabien rejected it. So not sure what branch you want me to apply this.
---------------------------------------------------------------------------
by vicb at 2012-04-10T16:25:38Z
May be the best is to put it on hold while I am reviewing your PRs. There are already enough changes, we'll make an other PR after all have been sorted out.
What's the difference between 3754 and 3810 ? (3810 + 3763 = 3754 ?)
---------------------------------------------------------------------------
by Tobion at 2012-04-10T16:39:32Z
Lol you forget to link the PR numbers. At first sight I thought it's some sort of mathematical riddle. Haha
#3810 is for 2.0 = #3763 (already merged) + #3754 for master
---------------------------------------------------------------------------
by vicb at 2012-04-10T16:52:18Z
I didn't link on purpose... the question is if '=' means strictly or loosely equal (any diffs - beside master vs 2.0) ?
---------------------------------------------------------------------------
by Tobion at 2012-04-10T17:06:04Z
It just applies my changes to 2.0. Nothing more. So master still differs from 2.0 by the addional features that were already implemented (e.g. `RouteCollection->addCollection` with optional requirements and options). But since my changes are bug fixes (except the performance improvement in #3763 but that doesn't break anything and makes 2.0 easier to maintain) I thought they should go into 2.0 as well.
---------------------------------------------------------------------------
by vicb at 2012-04-10T17:14:27Z
@Tobion only bug fixes mean "only bug fixes". You should re-open a PR for 2.0 with "only bug fixes", you might want to wait for me to review 3754.
---------------------------------------------------------------------------
by Tobion at 2012-04-10T17:21:00Z
Without #3763 it's much harder to apply the bug fixes. And now that I found 2 more bugs which requiresome rewriting of the PhpMatcherDumper, I don't want to apply all the commits by hand again for 2.0...
Commits
-------
c4e68a3 [Form] Moved logic of addXxx()/removeXxx() methods to the PropertyPath class
Discussion
----------
[Form] Moved logic of addXxx()/removeXxx() methods to the PropertyPath class
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3732
Todo: -
The addXxx()/removeXxx() methods should now be called correctly in ChoiceType and CollectionType.
PropertyPath now favors addXxx()/removeXxx() over setXxx() for collections. For example:
```
$propertyPath = new PropertyPath('article.tags');
// Tries to use addTag()/removeTag() and only uses setTags() (et al.)
// if not found
$propertyPath->setValue($article, $tags);
```
For other languages than English or very irregular plurals, a custom singular can be set by separating it with a pipe:
```
$propertyPath = new PropertyPath('article.genera|genus');
```
---------------------------------------------------------------------------
by bschussek at 2012-04-07T12:40:39Z
Again, the failing build is not my fault.
Commits
-------
61d792e [Form] Changed checkboxes in an expanded multiple-choice field to not include the choice index
bc9bc4a [Form] Fixed behavior of expanded multiple-choice field when submitted without ticks
2e07256 [Form] Simplified choice list API
2645120 [Form] Fixed handling of expanded choice lists, checkboxes and radio buttons with empty values ("")
Discussion
----------
[Form] Fixed handling of empty values in checkbox/radio/choice type
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3839, #3366
Todo: -
This PR fixes the processing of checkboxes and radio buttons with empty "value" attributes as well as of expanded choice forms with empty values. Additionally, some unnecessary complexity has been removed of the new ChoiceList API.
---------------------------------------------------------------------------
by stof at 2012-04-10T13:56:12Z
You probably need to change some things in the CHANGELOG file too
---------------------------------------------------------------------------
by bschussek at 2012-04-10T14:39:24Z
No. This is an update of a previous post-2.0 PR, the CHANGELOG is still accurate.
---------------------------------------------------------------------------
by stof at 2012-04-10T14:46:47Z
well, doesn't it require changes to the description of previous changes related to the ChoiceList ? It does in the UPGRADE file so it is weird if the CHANGELOG does not require any change.
---------------------------------------------------------------------------
by bschussek at 2012-04-10T14:56:05Z
Feel free to check yourself :)
- The route compiler does not add extra space or line-feed,
- The generated regex does not use the 'x' modified any more,
- The PHP and apache matchers do not need to strip any chars (vs space and line feed before),
- The space characters are escaped according to the apache format
Commits
-------
55014a6 [Routing] Request methods always return a raw path, fix the matcher to decode only once
d17ba0e Fixed base URL detection when request URI contains encoded chars
Discussion
----------
[RFC] Fix issues with url decoding
Related: #2324, #2963, #2962, #2579
### This PR fixes two issues:
* `+` in paths were turned to " " by `urldecode()`
* `urldecode()` was called a few times (and a different number of times according to which part of the path was handled, see #2962 for details).
### BC Breaks:
* `Request::getPathInfo()`, `Request::getBaseUrl()` and `Request::getBasePath` now return the raw (encoded) path (vs a decoded path before this PR). You should check any calls to these methods in your code and wrap them in `rawurldecode()` if needed.
* The `UrlMatcher` now decodes the URL only once (i.e. variable are no more decoded twice) and use `rawurldecode()` to support `+`.
### Notes:
* @arnaud-lb, the first commit is based on your #2963 so I put your name for the commit,
### Comment history from the original PR:
@vicb
**The state before this PR:**
* getRequestUri() returns an **encoded** value
* getPathInfo() returns a **decoded** value
* getBaseUrl() returns a **decoded** value
* getBasePath() returns a **decoded** value
The decoded value is wrong as `urldecode` is used in place of `rawurldecode` turning `+` into a space character (#2324).
The matcher starts by urldecoing the path (it is already decoded as explained right before) and then urldecodes each variable one more time.
We end up with a path being decoded twice and variables being decoded three times.
`Request::getUri()` calls both `getBaseUrl()` and `getPathInfo()` so that the return URI is **decoded**.
**The state after the PR:**
* getRequestUri() returns an **encoded** value
* getPathInfo() returns an **encoded** value
* getBaseUrl() returns an **encoded** value
* getBasePath() returns an **encoded** value
We are consistent and we have the raw values everywhere - there is no (easy) way to get the encoded value back once it has been decoded as it is done in the current code.
The matcher relies on an encoded value and decode the value only once (using `rawurldecode` to support `+`s).
So basically this PR:
* fix a bug - URL with `+` are now supported,
* makes paths consistent - encoded values everywhere, including `getUri()`
* makes variables consistent: they are decoded only once - the same as query string parameters.
There are some BC breaks:
* getPathInfo() returns an encoded value vs a decoded one before,
* getBaseUrl() returns an encoded value vs a decoded one before.
* getBasePath() returns an encoded value vs a decoded one before.
Any code relying on the output of one of the 2 previous methods should be checked and upgraded if needed. I am interested in the use cases where your code need to be updated.
@Seldaek
I checked a few projects and this is what I found (usage of getPathInfo & getBaseUrl):
- One use case of getPathInfo to check if the url started with `/something/` which is a prefix used for all "overlay" content which had to be treated differently somewhere => most likely unaffected
- One use case for checking path prefixes by regex in our [CorsBundle](https://github.com/nelmio/NelmioCorsBundle/blob/master/EventListener/CorsListener.php#L52-56) => potentially affected depending on the complexity of regexes I'd say
@vicb
Thanks @Seldaek for reporting the use cases. You second case would be solved by `rawurldecode`ing the path info which is a minimal change.
And in general I think we have to expand to doc to specify the url format that should be used.
---------------------------------------------------------------------------
by vicb at 2012-04-04T13:42:21Z
I'll squash the commits before this gets merged but for now it make the review easier.
Setting a property path like "article.tags" will now automatically try to
favor addTag() and removeTag() over setTags(), if found. If you want to
set up a property path with an irregular singular that is not detected,
you can use "|" to separate the plural from the singular form in the
path: "article.genera|genus".
Another consequence of this commit is that the MergeCollectionListener has
been simplified a lot. Forms returning an array or a collection will
always result in adders/removers being called now without having to add
this listener.
Commits
-------
7f92833 [BrowserKit] Fixed cs.
df3da28 [BrowserKit] Using assertNull instead of assertEquals.
87890d3 [BrowserKit] Fixed CookieJar issue being unable to parse multiple cookies from Set-Cookie.
Discussion
----------
[BrowserKit] Fixed CookieJar being unable to parse multiple cookies
Fix proposition for #3109
My fix splits value of *Set-Cookie* header by comma. Than it checks each extracted part if it starts with a cookie-name (token). If check is positive cookie is added to the list. Otherwise it's appended to the previous value. First element is always added to the list.
[rfc6265](http://tools.ietf.org/html/rfc6265) defines cookie-name with token:
cookie-name = token
token = <token, defined in [RFC2616], Section 2.2>
token is defined in [rfc2616](http://tools.ietf.org/html/rfc2616#section-2.2) as follows:
token = 1*<any CHAR except CTLs or separators>
CHAR = <any US-ASCII character (octets 0 - 127)>
separators = "(" | ")" | "<" | ">" | "@"
| "," | ";" | ":" | "\" | <">
| "/" | "[" | "]" | "?" | "="
| "{" | "}" | SP | HT
That means cookie-name can be built out of following set of characters: *! # $ % & ' * + - . ^ _ ` | ~ 0-9 A-Z a-z*
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Commits
-------
100e97e [Filesystem] Fixed warnings in makePathRelative().
f5f5c21 [Filesystem] Fixed typos in the docblocks.
d4243a2 [Filesystem] Fixed a bug in remove being unable to remove symlinks to unexisting file or directory.
11a676d [Filesystem] Added unit tests for mirror method.
8c94069 [Filesystem] Added unit tests for isAbsolutePath method.
2ee4b88 [Filesystem] Added unit tests for makePathRelative method.
21860cb [Filesystem] Added unit tests for symlink method.
a041feb [Filesystem] Added unit tests for rename method.
8071859 [Filesystem] Added unit tests for chmod method.
bba0080 [Filesystem] Added unit tests for remove method.
8e861b7 [Filesystem] Introduced workspace directory to limit complexity of tests.
a91e200 [Filesystem] Added unit tests for touch method.
7e297db [Filesystem] Added unit tests for mkdir method.
6ac5486 [Filesystem] Added unit tests for copy method.
1c833e7 [Filesystem] Added missing docblock comment.
Discussion
----------
[Filesystem] Fixed a bug in remove() being unable to unlink broken symlinks
While working on test coverage for Filesystem class I discovered a bug in remove() method.
Before removing a file a check is made if it exists:
if (!file_exists($file)) {
continue;
}
Problem is [file_exists()](http://php.net/file_exists) returns false if link's target file doesn't exist. Therefore remove() will fail to delete a directory containing a broken link. Solution is to handle links a bit different:
if (!file_exists($file) && !is_link($file)) {
continue;
}
Additionally, this PR improves test coverage of Filesystem component.
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
---------------------------------------------------------------------------
by cordoval at 2012-04-07T00:55:59Z
✌.|•͡˘‿•͡˘|.✌
---------------------------------------------------------------------------
by fabpot at 2012-04-07T06:12:34Z
Tests do not pass for me:
PHPUnit 3.6.10 by Sebastian Bergmann.
Configuration read from /Users/fabien/work/symfony/git/symfony/phpunit.xml.dist
.........................EE.......
Time: 0 seconds, Memory: 5.25Mb
There were 2 errors:
1) Symfony\Component\Filesystem\Tests\FilesystemTest::testMakePathRelative with data set #0 ('/var/lib/symfony/src/Symfony/', '/var/lib/symfony/src/Symfony/Component', '../')
Uninitialized string offset: 29
.../rc/Symfony/Component/Filesystem/Filesystem.php:183
.../rc/Symfony/Component/Filesystem/Tests/FilesystemTest.php:434
2) Symfony\Component\Filesystem\Tests\FilesystemTest::testMakePathRelative with data set #1 ('var/lib/symfony/', 'var/lib/symfony/src/Symfony/Component', '../../../')
Uninitialized string offset: 16
.../rc/Symfony/Component/Filesystem/Filesystem.php:183
.../rc/Symfony/Component/Filesystem/Tests/FilesystemTest.php:434
FAILURES!
Tests: 34, Assertions: 67, Errors: 2.
---------------------------------------------------------------------------
by jakzal at 2012-04-07T07:26:15Z
Sorry for this. For some reason my PHP error reporting level was to low to catch this...
Should be fixed now but I needed to modify the makePathRelative() (this bug existed before).
Commits
-------
6584721 [Form] Improved labels generated by default from form names
6e0b03a [Form] Fixed label of prototype in CollectionType
fc342d1 Merge remote branch 'umpirsky/collection-name' into issue3738
f91660d Added test for prototype label.
Discussion
----------
[Form] Fixed default label generated for the CollectionType prototype
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #3738, #3739
Todo: -
(the fact that the build fails seems to origin from the broken master)
Commits
-------
b6ac1aa [FORM] Give PropertyPath ability to read hassers
Discussion
----------
[Form] Give PropertyPath ability to read hassers
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
Using a `hasser` instead of `isser` for Boolean values is pretty common. I've found myself using `issers` a handful of times just to make an interface play nice with the form component, but the code reads funny now. I don't think we should be accounting for every possible `getter` variation, but I think this one is common enough that it warrants a discussion.
---------------------------------------------------------------------------
by fabpot at 2012-03-11T08:25:31Z
I tend to agree with with. What do you think @bschussek?
---------------------------------------------------------------------------
by kriswallsmith at 2012-03-16T22:42:28Z
I'm not so sure. There are lots of reasons to write a *hasser* that accepts an argument (i.e. `User::hasRole($role)`). Doesn't seem as clean as *issers* and *getters*.
---------------------------------------------------------------------------
by vicb at 2012-03-16T22:49:14Z
> There are lots of reasons to write a hasser that accepts an argument
May be can check for 0 args as we are already using reflexion ?
---------------------------------------------------------------------------
by kriswallsmith at 2012-03-16T22:55:43Z
In that case we should check that there are either 0 arguments or only optional arguments and also consider adding the same logic to the other varieties.
---------------------------------------------------------------------------
by jjbohn at 2012-03-16T23:37:47Z
Passing arguments seems like a pretty big departure for PropertyPath. How would you annotate that? I'm not sure I see a common use case for needing arguments when mapping data to and from forms.
---------------------------------------------------------------------------
by stof at 2012-03-16T23:50:22Z
@jjbohn it is not about passing arguments but about using the hasser only if it does not have required arguments
---------------------------------------------------------------------------
by jjbohn at 2012-03-17T01:54:18Z
Ah. I see. I have a tendency to read @kriswallsmith comments wrong :D. I could see that but iirc, there's not any current check like this on the other accessors. Happy to add it though if there's a consensus.
---------------------------------------------------------------------------
by fabpot at 2012-03-17T11:24:34Z
What's the point is checking the hasser/getter/isser arguments. It's up to the developer to check if he can use them or not. Let's not complexify the code for this.
---------------------------------------------------------------------------
by kriswallsmith at 2012-03-17T15:37:39Z
My concern is that someone writes a hasser method on their model that is not intended for use with the form component but it's called anyway, leading to WTFs.
---------------------------------------------------------------------------
by stof at 2012-04-03T22:28:21Z
@fabpot what's your decision about this ?
@jjbohn you need to rebase your PR. It conflicts with master as tests have been moved
---------------------------------------------------------------------------
by bschussek at 2012-04-05T14:53:55Z
@kriswallsmith is right. The check for 1 === $method->getNumberOfRequiredParameters() can (and should) easily be added to all of the if-clauses here.
Apart from that, I'm okay with adding this.
Commits
-------
8689e9c [WIP] [Locale] Fixes NumberFormatter tests failing when using ICU 4.8 or 4.8.1
Discussion
----------
[WIP] [Locale] Fixes NumberFormatter tests failing when using ICU 4.8 or 4.8.1.1
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: no
Todo: fix DateFormatter
The ICU CLDR 2.0 data has been updated on ICU 4.8 and the same data set is used on version 4.8.1.1. The problem is related to [this commit](http://bugs.icu-project.org/trac/changeset?reponame=&new=31307%40icu%2Ftrunk%2Fsource%2Fdata%2Fcurr%2Fen.txt&old=31074%40icu%2Ftrunk%2Fsource%2Fdata%2Fcurr%2Fen.txt) which has since been updated with new data and subsequently shipped with version 49.
The `DateFormatter` tests are still failing - see this [gist](https://gist.github.com/2004d40e5167286028ea). Suggestions are welcomed on how to handle this part.
Test results with PHP 5.4.0 with ICU 4.8.1.1 on OSX:
````
FAILURES!
Tests: 5917, Assertions: 12749, Failures: 26, Incomplete: 11, Skipped: 47.
```
with this WIP patch:
```
FAILURES!
Tests: 5917, Assertions: 12749, Failures: 13, Incomplete: 11, Skipped: 47.
```
Commits
-------
8ceb569 Fix typo
8702ea5 [Validator] Allow empty keys in the validation config
Discussion
----------
[Validator] Allow empty keys in the validation config
This allows you to just list fields that don't have validation rules (yet), for future reference it's kinda helpful. Right now if they're not commented out a fatal is thrown because null isn't an array.
---------------------------------------------------------------------------
by bschussek at 2012-04-05T15:34:09Z
Could you add a test please?
---------------------------------------------------------------------------
by Seldaek at 2012-04-05T15:34:48Z
The dummy key I added in the test makes it fail without the fix.
---------------------------------------------------------------------------
by bschussek at 2012-04-05T15:46:54Z
Ah, that's perfect, I overlooked that. Thanks!
New checkHost attribute in email constraint will
make the validator check for only one of MX, A or AAAA
DNS resource records to verify it as a valid
email address.
Commits
-------
3c32569 [Routing] Added the possibility to define options for imported resources
Discussion
----------
[Routing] Added the possibility to define options for imported resources
Closes#2772
Commits
-------
2a90871 [Console] Removed previously introduced BC break.
90a2a6e [Console] Undecorated formatter must update style stack too.
bd7e01a [Console] Fixed output formatter test broken by new implementation.
a1add4b [Console] Updated output formatter to use style stack.
4f298dd [Console] Added formatter style stack.
93ffe54 [Console] Added getters to output formatter style (and its interface).
48e6b49 [Console] Updated formatter test to match styles bug fix.
ad334b6 [Console] Fixed empty style appliance.
31d5fe5 [Console] Fixed output formatter docblock.
Discussion
----------
[Console] Fixes formatter nested style appliance.
Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
When outputing styled text in the console, you sometimes face to a confusing behavior: style tags cannot be nested. If tou try something like `<fg=blue>Hello <fg=red>world</fg=red>!</fg=blue>`, the trailing `!` will not be styled.
This PR introduce a new FormatterOutputStyleStack to keep open/closed styles informations up-to-date. It slightly changes OutputFormatter implementation which no longer uses `OutputFormatterStyle::apply()` method, but the new `OutputFormatterStyle::getTerminalSequence()`.
**Question:** I don't une `OutputFormatterStyleInterface` but `OutputFormatterStyle` to type `OutputFormatterStyleStack` methods arguments (to avoid BC break on the interface). Do you think it's right?
**Notice:** I also needed to fix some tests broken by new implementation.
---------------------------------------------------------------------------
by stof at 2012-03-16T10:27:56Z
Adding new methods in an interface is a BC break for people implementing it
---------------------------------------------------------------------------
by jfsimon at 2012-03-16T10:33:21Z
@stof indeed... this is a problem, should I remove them? If I do so, I should use `OutputFormatterStyle` instead of the interface to type arguments in `OutputFormatterStyleStack` right?
Commits
-------
8a0e6d2 [HttpFoundation] Update changelog.
4fc04fa [HttpFoundation] Renamed MetaBag to MetadataBag
2f03b31 [HttpFoundation] Added the ability to change the session cookie lifetime on migrate().
39141e8 [HttpFoundation] Add ability to force the lifetime (allows update of session cookie expiry-time)
ec3f88f [HttpFoundation] Add methods to interface
402254c [HttpFoundation] Changed meta-data responsibility to SessionStorageInterface
d9fd14f [HttpFoundation] Refactored for moved tests location.
29bd787 [HttpFoundation] Added some basic meta-data to Session
Discussion
----------
[2.1][HttpFoundation] Added some basic meta-data to Session
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
References the following tickets: #2171
Todo: -
Session data is stored as an encoded string against a single id. If we want to store meta-data about the session, that data has to be stored as part of the session data to ensure the meta-data can persist using any session save handler.
This patch makes it much easier to determine the logic of session expiration. In general a session expiry can be dealt with by the gc handlers, however, in some applications more specific expiry rules might be required.
Session expiry may also be more complex than a simple, session was idle for x seconds. For example, in Zikula there are three security settings, Low, Medium and High. The rules for session expiry are more complex as under the Medium setting, a session will expire after x minutes idle time, unless the rememberme option was ticked on login. If so, the session will not idle. This gives the user some control over their experience. Under the high security setting, then there is no option, sessions will expire after the idle time is reached and login the UI has the rememberme checkbox removed.
The other advantage is that under this methodology, there can be a UI experience on expiry, like "Sorry, your session expired due to being idle for 10 minutes".
Keeping in the spirit of Symfony2 Components, I am seeking to make session handling flexible enough to accommodate these general requirements without specifically covering expiration rules. It would mean that it would be up to the implementing application to specifcally check and expire session after starting it.
Expiration might look something like this:
$session->start();
if (time() - $session->getMetadataBag()->getLastUpdate() > $maxIdleTime) {
$session->invalidate();
throw new SessionExpired();
}
This commit also brings the ability to change the `cookie_lifetime` when migrating a session. This means one could move from a default of browser only session cookie to long-lived cookie when changing from a anonymous to a logged in user for example.
$session->migrate($destroy, $lifetime);
---------------------------------------------------------------------------
by drak at 2012-03-30T18:18:43Z
@fabpot I have removed [WIP] status.
---------------------------------------------------------------------------
by drak at 2012-03-31T13:34:57Z
NB: This PR has been rebased and the tests relocated as per recent master changes.
---------------------------------------------------------------------------
by drak at 2012-04-03T02:16:43Z
@fabpot - ping
Commits
-------
56c1e31 performance improvement in PhpMatcherDumper
Discussion
----------
performance improvement in PhpMatcherDumper
Tests pass: yes
The code generation uses a string internally instead of an array. The array wasn't used for random access anyway.
I also removed 4 unneeded iterations this way (when imploding, when merging and twice when applying the extra indention). A `preg_replace` could also be saved under certain circumstances by moving it.
And there was a small code errror in line 139.
Commits
-------
f1f1494 Added an exception when passing an invalid object to ApcClassLoader
f5cb167 [ClassLoader] Added a DebugClassLoader using composition
0e54a22 Updated the changelog
eae772e [ClassLoader] Added an ApcClassLoader
4d1333f Changed the test autoloading to use the new autoloader
09850bd [ClassLoader] Added a simplified PSR-0 ClassLoader
Discussion
----------
Autoloader refactoring
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: [](http://travis-ci.org/stof/symfony)
As discussed in #3623, I added a new ClassLoader instead of modifying the UniversalClassLoader, to be able to use the method names without BC concerns. The new class works the same than the composer autoloader regarding the handling of fallbacks, to be able to reuse namespace maps generated by composer.
```php
<?php
// autoload.php
require_once __DIR__.'/vendor/symfony/class-loader/Symfony/Component/ClassLoader/ClassLoader.php';
$loader = new Symfony\Component\ClassLoader\ClassLoader();
$map = require __DIR__.'/vendor/.composer/autoload_namespaces.php';
$loader->addPrefixes($map);
$loader->register();
```
Differences with the composer class loader:
- Composer's ``add`` method is named ``addPrefix`` in the Symfony ClassLoader
- the methods related to the class map are removed as Symfony has a separate laoder for class maps
- the ``addPrefixes`` method is added, accepting a namespace map.
I also added a new ApcClassLoader which uses composition instead of inheriting from a class loader, which makes it far more easier to reuse (we could wrap a Composer autoloader with it for instance).
```php
<?php
$composerLoader = require __DIR__.'/vendor/.composer/autoload.php';
// no need to require the file manually as Composer already registered its autoloader
$cachedLoader = new Symfony\Component\ClassLoader\ApcClassLoader('autoload.my_app', $composerLoader);
$cachedLoader->register();
// unregister the Composer autoloader as we wrapped it in the ApcClassLoader
$composerLoader->unregister();
```
TODO:
- refactor the Debug class loader to use composition too to be able to support different class loaders
---------------------------------------------------------------------------
by fabpot at 2012-04-02T16:31:28Z
Can you update the CHANGELOG and the UPGRADE file accordingly?
---------------------------------------------------------------------------
by stof at 2012-04-02T16:47:43Z
I added a note in the CHANGELOG. There is nothing to add in the UPGRADE file as the change is fully BC (I did not change the UniversalClassLoader at all so it can still be used).
I'm working on the Debug loader right now so please wait a bit before merging
---------------------------------------------------------------------------
by stof at 2012-04-02T17:12:11Z
Here is a new DebugClassLoader using composition too. this way, it is able to support the UniversalClassLoader, the ApcUniversalClassLoader (without dropping the use of APC as done previously), the new ClassLoader, the new ApcClassLoader and even the composer autoloader.
I'm not sure about the use of ``method_exists`` as it could break if an autoloader implements a protected ``findFile`` method (crappy PHP 😢) but hardcoding the supported classes would be a pain and requiring an interface would make the autoloaders more difficult to use (as the interface would need to be required first) and would drop the support of the composer autoloader.
Unlike the ApcUniversalClassLoader, ApcClassLoader uses composition,
meaning it can be used to wrap any object providing a findFile($class)
method. Both the UniversalClassLoader and the new ClassLoader follow
this convention. It can also be used to wrap the Composer autoloader.
The new ClassLoader does not differentiate namespaced classes and
PEAR-like classes like the UniversalClassLoader does as the PEAR
format is a subset of PSR-0.
The new loader registers fallbacks by adding a location for an empty
prefix, as done in Composer. This allows using namespaces map generated
by Composer without any special processing on them.
Commits
-------
dcf82d7 [Finder] Added sortBy options based on accessed, changed and modified times
Discussion
----------
[Finder] Added sortBy options based on accessed, changed and modified times
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Commits
-------
925b65d updated reference to tests
Discussion
----------
updated reference to tests
this is basically the format used in all other components.
however i am not sure if it really makes sense to list ``phpunit -c src/Symfony/Component/HttpFoundation/``, since this relative path could be confusing for anyone using the standalone components. But even if using ``symfony/symfony`` the path is wrong relative to the location of the README.md.
Commits
-------
24a0d0a [DependencyInjection] Support Yaml calls without arguments
Discussion
----------
[DependencyInjection] Support Yaml calls without arguments
This is a very important option which allows the cookie lifetime to be changed on migrate.
For example when a user converts from an anonymous session to a logged in session one might
wish to change from a persistent cookie to browser session (e.g. a banking application).
This commit allows applications to know certain meta-data about the session
Session storage is designed to only store some data against a session ID
so this method is necessary to be compatible with any session handler, including
native handlers.
Commits
-------
8dd2c27 [HttpFoundation] Further micro-optimization.
54c5d5e [HttpFoundation] Micro-optimisation.
Discussion
----------
[HttpFoundation] Micro-optimisation.
Ref #3729
---------------------------------------------------------------------------
by robocoder at 2012-03-30T11:45:02Z
If you pre-flip your $validOptions arrays, you can use isset() instead of in_array() in the loop.
This changes the performance from O(m * n) to O(m).
---------------------------------------------------------------------------
by drak at 2012-03-30T11:53:24Z
@robocoder What is the expense of the array_flip though?
---------------------------------------------------------------------------
by robocoder at 2012-03-30T11:56:21Z
Why would you use array_flip if the array doesn't change? Change $validOptions = array('x', 'y', ...) to $validOptions = array('x' => 0, 'y' => 0, ...), then change the in_array() to use isset().
---------------------------------------------------------------------------
by stof at 2012-03-30T11:57:08Z
@drak a loop. But it will be done only once before the other loop so it will be O(n + m) instead of O(m * n)
---------------------------------------------------------------------------
by drak at 2012-03-30T12:00:47Z
Ok :)
Commits
-------
c73748f [HttpFoundation] Added RFC reference to 308
468ad40 [HttpFoundation] Added support for 308 / Permanent Redirect
Discussion
----------
[HttpFoundation] Added support for 308 / Permanent Redirect
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes (i guess) [](http://travis-ci.org/lyrixx/symfony)
Fixes the following tickets: -
Todo: -
I know this is still a draft, but it is already implemented in Firefox.
See :
- http://tools.ietf.org/html/draft-reschke-http-status-308-07
- https://developer.mozilla.org/en/HTTP/HTTP_response_codes#308
---------------------------------------------------------------------------
by stloyd at 2012-03-29T09:25:20Z
It will be in Firefox... 14!
---------------------------------------------------------------------------
by fabpot at 2012-03-29T09:33:01Z
Like the non RFC 2616 status code, you need to add the RFC number as a comment (or the reference to the draft).
---------------------------------------------------------------------------
by lsmith77 at 2012-03-29T11:58:14Z
can you open a PR for https://github.com/FriendsOfSymfony/FOSRest/blob/master/Util/Codes.php ?
---------------------------------------------------------------------------
by lyrixx at 2012-03-29T12:08:31Z
@lsmith77 : Done. See : https://github.com/FriendsOfSymfony/FOSRest/pull/7 :)
Commits
-------
57de69f added an exception for failed processes
Discussion
----------
added an exception for failed processes
---------------------------------------------------------------------------
by Seldaek at 2012-03-19T07:27:56Z
So this is just there to use if you want to throw an exception when a process call failed in your application? It doesn't seem enabled by default, which I think is good anyway.
---------------------------------------------------------------------------
by stof at 2012-03-19T07:44:43Z
@Seldaek yeah, I guess this is a way to make it easier to reuse what he implemented for Assetic first.
---------------------------------------------------------------------------
by fabpot at 2012-03-23T15:08:26Z
How and when do you use such an exception?
---------------------------------------------------------------------------
by schmittjoh at 2012-03-23T17:22:16Z
It's intended for your own code to give you a nice and meaningful error message without having to repeat the same code whereever you are dealing with a Process:
```php
if (0 !== $proc->run()) {
throw new ProcessFailedException($proc);
}
Commits
-------
5ae76f1 [HttpFoundation] Update documentation.
910b5c7 [HttpFoudation] CS, more tests and some optimization.
b0466e8 [HttpFoundation] Refactored BC Session class methods.
84c2e3c [HttpFoundation] Allow flash messages to have multiple messages per type.
Discussion
----------
[2.1][HttpFoundation] Multiple session flash messages
Bug fix: no
Feature addition: yes
Backwards compatibility break: yes, but this already happened in #2583. BC `Session` methods remain unbroken.
Symfony2 tests pass: yes
Fixes the following tickets: #1863
References the following tickets: #2714, #2753, #2510, #2543, #2853
Todo: -
This PR alters flash messages so that it is possible to store more than one message per flash type using the `add()` method or by passing an array of messages to `set()`.
__NOTES ABOUT BC__
This PR maintains BC behaviour with the `Session` class in that the old Symfony 2.0 methods will continue to work as before.
---------------------------------------------------------------------------
by drak at 2012-02-13T06:28:33Z
I think this is ready for review @fabpot @lsmith77
---------------------------------------------------------------------------
by lsmith77 at 2012-02-14T19:30:39Z
the FlashBag vs. AutoExpireFlashBag behavior and setup difference should probably also be explained in the upgrading log
---------------------------------------------------------------------------
by drak at 2012-02-15T04:43:14Z
@lsmith77 Those differences are explained already in the changelog
* Added `FlashBag`. Flashes expire when retrieved by `get()` or `all()`.
This makes the implementation ESI compatible.
* Added `AutoExpireFlashBag` (default) to replicate Symfony 2.0.x auto expire behaviour of messages auto expiring
after one page page load. Messages must be retrived by `get()` or `all()`.
---------------------------------------------------------------------------
by Crell at 2012-02-19T17:35:34Z
Drak asked me to weigh in here with use cases. Drupal currently has a similar session-stored-messaging system in place that I'd like to be able to replace with Flash messages. We frequently have multiple messages within a single request, however, so this change is critical to our being able to do so.
For instance, when saving an article in Drupal there is, by default, a "yay, you saved an article!" type message that gets displayed. If you also have the site configured to send email when a post is updated, you may see a "email notifications sent" message (depending on your access level). If you have a Solr server setup for search, and you're in debug mode, there will also be a "record ID X added to Solr, it should update in 2 minutes" message. And if there's a bug somewhere, you'll also get, as an error message rather than notice message, a "Oops, E_NOTICE on line 54" message.
Form validation is another case. If you have multiple errors in a single form, we prefer to list all of them. So if you screw up 4 times on a form, you may get 4 different error messages showing what you screwed up so you can fix it in one go instead of several.
Now sure, one could emulate that by building a multi-message layer on top of single-layer messages, but, really, why? "One is a special case of many", and there are many many cases where you'll want to post multiple messages. Like, most of Drupal. :-)
---------------------------------------------------------------------------
by lsmith77 at 2012-03-06T20:55:51Z
@fabpot is there any information you still need before merging this? do you want more discussion in which case you might want to take this to the mailing list ..
---------------------------------------------------------------------------
by drak at 2012-03-08T18:54:13Z
Another plus for this PR is that it requires no extra lines of code in templates etc to display the flashes, see https://github.com/symfony/symfony/pull/3267/files#diff-1
---------------------------------------------------------------------------
by drak at 2012-03-15T06:38:21Z
Rebased against current `master`, should be mergeable again..
---------------------------------------------------------------------------
by evillemez at 2012-03-17T03:08:41Z
+1 to this, I have an extended version of HttpFoundation just for this... would love to get rid of it.
Commits
-------
3f2b917 added a configurable extension base class
Discussion
----------
added a configurable extension base class
This is mostly a convenience class which provides first-class integration with the Config/Definition component.
Commits
-------
0c9b2d4 use SecurityContextInterface instead of SecurityContext
Discussion
----------
[2.0][Security] use SecurityContextInterface instead of SecurityContext
see https://github.com/symfony/symfony/pull/3522 (this is a fix for the 2.0 branch)
---------------------------------------------------------------------------
by pminnieur at 2012-03-21T13:25:59Z
*ping* it still missed the 2.0.12 release ...
---------------------------------------------------------------------------
by stof at 2012-03-21T16:41:28Z
@pminnieur you PR has been merged into master, not into 2.0, so it will only be in 2.1
---------------------------------------------------------------------------
by pminnieur at 2012-03-21T16:43:02Z
I know, and this is a second PR for 2.0 branch.
Commits
-------
bd02554 [HttpFoundation] SPL IteratorAggregate+Countable on *Bags
665fdeb [HttpFoundation] SPL on ParameterBag
Discussion
----------
[HttpFoundation] SPL on ParameterBag
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Added a couple SPL interfaces to ParameterBag, added shortcuts to working with the parameters. For example:
```php
<?php
$post = Request::createFromGlobal()->request;
echo "There are {count($post)} POST variables\n";
foreach ($post as $key => $val) {
echo "{$key}: {$val}\n";
}
```
Thoughts?
---------------------------------------------------------------------------
by stealth35 at 2012-03-07T13:09:11Z
You already have the `all` method
``` php
<?php
$post = Request::createFromGlobals()->request->all();
echo "There are ", count($post), " POST variables\n";
foreach ($post as $key => $val) {
echo "{$key}: {$val}\n";
}
```
---------------------------------------------------------------------------
by cboden at 2012-03-07T13:50:22Z
Yes, but when in the context of working with the Request object (or POST ParamegerBag), it's 1 more call and loose variable to set.
ParameterBag is a container, these common SPL interfaces give standard PHP container methods to it.
---------------------------------------------------------------------------
by lsmith77 at 2012-03-07T18:42:41Z
makes sense to me ..
---------------------------------------------------------------------------
by vicb at 2012-03-09T15:45:40Z
Probably makes sense. Could you check if any other `*Bag.php` needs to be updated so that it could ba an atomic merge.
---------------------------------------------------------------------------
by cboden at 2012-03-09T15:48:40Z
Whoops, good catch @vicb. I made a poor assumption all the *Bags extended ParameterBag, while only some do. I will post an update shortly.
Commits
-------
c4ee947 Native Redis Session Storage update
665f593 NativeRedisSessionStorage added
Discussion
----------
[HttpFoundation] Native Redis Session Storage
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
---------------------------------------------------------------------------
by lstrojny at 2012-03-04T23:15:43Z
Does Symfony (or any of its dependencies) has Redis support in any form whatsoever? If not this might be a good point to decide which clients to support
---------------------------------------------------------------------------
by lsmith77 at 2012-03-04T23:36:11Z
well ideally we just get this cache interface stuff done .. for this use case it would be perfect.
---------------------------------------------------------------------------
by pulzarraider at 2012-03-05T00:35:59Z
There is RedisProfilerStorage available (based on phpredis). I prefer and write code for [phpredis](https://github.com/nicolasff/phpredis).
It's recommended by [official Redis homepage](http://redis.io/clients#PHP). [In this benchmark](http://dev.af83.com/2011/01/01/which-php-library-to-use-with-redis-the-benchmark.html
) is fastest and less memory consumpting.
But if somebody prefer predis (with phpiredis), rediska or something other widely used, there are no limitations to add support of it to Symfony.
My opinion is, that the C extension should be supported at first, because of good performance and native session storage support. Redis is quite young and the process of creating PHP clients is comparable to Memcache.
There were created pure PHP Memcache clients in the past (Google found for example [this](http://www.phpclasses.org/browse/file/20284.html) and [this](http://code.blitzaffe.com/pages/phpclasses/files/memcached_client_52-12)), but they are not being used now. Everyone, who is seriously thinking about performance, is using only the C Redis/Memcache(d)/... extensions.
---------------------------------------------------------------------------
by drak at 2012-03-05T07:40:06Z
+1 on this PR. Needs a test written though.
I don't think there is any need to wait for #3493 imo. I'll deal with it if this is merged before #3493.
Are there any PHP ini settings for this for this driver or is everything via the `session.save_path` directive? (A quick look at the C code seems to indicate there are no explicit ini directives).
---------------------------------------------------------------------------
by lstrojny at 2012-03-05T12:14:34Z
@pulzarraider I don’t necessarily disagree with the usage of phpredis, I just wanted to bring up the issues of various clients and people having different preferences about them.
---------------------------------------------------------------------------
by fabpot at 2012-03-05T14:46:22Z
@pulzarraider Can you add some unit tests before I merge?
---------------------------------------------------------------------------
by pulzarraider at 2012-03-11T20:19:57Z
@drak No there are no php.ini settings. Only RedisArray has some, but it's another feature.
@fabpot I've added simple test based on other session storage tests.
I planned to create a RedisSessionStorage, too, but I have no time for it now. This can be added later in another PR as it's independent from NativeRedisSessionStorage.
---------------------------------------------------------------------------
by drak at 2012-03-12T02:21:25Z
The code looks OK to me.
---------------------------------------------------------------------------
by fabpot at 2012-03-15T06:05:27Z
#3493 has been merged now.
---------------------------------------------------------------------------
by pulzarraider at 2012-03-16T23:21:27Z
Code updated.
Commits
-------
f9f51a5 fixed cs
af65673 [Process] Added support for non-blocking process control Added methods to control long running processes to the Process class: - A non blocking start method to startup a process and return immediately - A blocking waitForTermination method to wait for the processes termination - A stop method to stop a process started with start All status-getters like getOutput were changed to return real-time data
Discussion
----------
[Process] Added support for non-blocking process control
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes [](http://travis-ci.org/drak3/symfony)
Fixes the following tickets: #1049
Todo: -
Added methods to control long running processes (as described in issue #1049) to the Process class:
- A non blocking start method to startup a process and return
immediately
- A blocking waitForTermination method to wait for the processes
termination
- A stop method to stop a process started with start
All status-getters like getOutput were changed to return real-time data
---------------------------------------------------------------------------
by Seldaek at 2012-03-23T10:52:30Z
Overall this seems like a good improvement. I didn't check the code in detail though.
---------------------------------------------------------------------------
by drak3 at 2012-03-23T11:21:45Z
@stof @Seldaek thanks, fixed
Added methods to control long running processes to the Process class:
- A non blocking start method to startup a process and return
immediately
- A blocking waitForTermination method to wait for the processes
termination
- A stop method to stop a process started with start
All status-getters like getOutput were changed to return real-time data
Commits
-------
da3a2c4 [Process] Fix command escaping
Discussion
----------
[Process] Fix command escaping
My bad, I misunderstood what escapeshellcmd was good for. This fixes it by properly escaping all args.
The test suite hangs here but it already did before, so I'm not sure if all tests pass.
Commits
-------
4a43453 remove callback from constructor and create method
601b87c add basic validation of callback name
266f76d rename jsonp to callback, defaults to null
38b79a7 add data and callback setter to JsonResponse
6788224 add JSONP support to JsonResponse
Discussion
----------
add JSONP support to JsonResponse
---------------------------------------------------------------------------
by schmittjoh at 2012-03-19T17:56:24Z
I think a ``setCallback()`` method would be more expressive, and easier to use:
```php
<?php
return JsonResponse::create($myData)->setCallback('foo');
// vs
return new JsonResponse($myData, 200, array(), 'foo');
```
What do you think?
---------------------------------------------------------------------------
by havvg at 2012-03-19T18:07:38Z
Looks good to me, I'll add it.
---------------------------------------------------------------------------
by dlsniper at 2012-03-19T19:38:45Z
+1 for this one :)
---------------------------------------------------------------------------
by vicb at 2012-03-19T23:09:50Z
Sorry for nitpicking but what about:
* some validation on the function name ?
* renamming `jsonp` -> `callback` ?
---------------------------------------------------------------------------
by havvg at 2012-03-20T09:16:32Z
@vicb What do you mean with "some validation on the function name"? I can't follow you there.
---------------------------------------------------------------------------
by vicb at 2012-03-20T09:22:49Z
I mean a valid JS function name
---------------------------------------------------------------------------
by havvg at 2012-03-20T09:34:59Z
Ah I see, I searched for it, and ended up with those results:
* The most complete: http://stackoverflow.com/questions/2008279/validate-a-javascript-function-name#answer-9392578
* and a less accurate one: http://www.geekality.net/2011/08/03/valid-javascript-identifier/
I'm not sure whether to put this into the `JsonResponse` itself, or to add somewhere else (where, if so?).
---------------------------------------------------------------------------
by vicb at 2012-03-20T09:45:20Z
I would go for a regexp only (ignoring reserved words); The idea would be not to use this to run arbitrary JS code.
---------------------------------------------------------------------------
by fabpot at 2012-03-21T21:33:36Z
Now that you have added the `setCallback` method, I would remove the constructor argument as it makes the signature quite long. As we have the `create` method anyway, it's more explicit and clearer to use the `setCallback` .
---------------------------------------------------------------------------
by havvg at 2012-03-21T21:37:51Z
So remove the callback argument from both, the constructor and the `create` method or only from the constructor?
---------------------------------------------------------------------------
by havvg at 2012-03-21T21:38:30Z
Ehr.. never mind :-)
Commits
-------
836d12b Fixing typo.
ac2a187 Improved feedback for Upload Validator to cover all PHP error states. This way we don't get a unclear "upload error" message unless its something completely unexpected.
Discussion
----------
Improved feedback for Upload Validator to cover all PHP error states.
The upload validator only sets individual messages for 2 out of the 7 error states PHP suports for uploading files. Which means when you have any of those 5 stats you get a standard error message and have to really dig into the code to read the error state.
I added messages for every state, so that you will always get a detailed message.
---------------------------------------------------------------------------
by stloyd at 2012-03-19T13:54:04Z
You should probably also extend translations in `FrameworkBundle`.
---------------------------------------------------------------------------
by rdohms at 2012-03-19T14:04:50Z
@stloyd what's the best way to do that? I obviously don't speak all languages. Could you point me to a best practices in this case?
---------------------------------------------------------------------------
by stof at 2012-03-19T15:58:17Z
@rdohms update the translations for the languages you speak. Other people will contribute with update later eventually :)
---------------------------------------------------------------------------
by rdohms at 2012-03-19T22:34:50Z
Fixed the typo, only other language i can update is portuguese, but it needs more work. I'll update it on a separate PR later on.
Anything else for this PR?
---------------------------------------------------------------------------
by mvrhov at 2012-03-20T05:41:00Z
@rdohms: just put English strings into other lanugages
---------------------------------------------------------------------------
by rdohms at 2012-03-20T07:35:56Z
@mvrhov is there a quick way to do this? or is it copying and pasting into every language and adjusting the string IDs?
---------------------------------------------------------------------------
by mvrhov at 2012-03-20T09:02:59Z
AFAIK you'll have to copy paste. String ids and source are the same in all translations so it really is just c/p after you update the first one.
---------------------------------------------------------------------------
by rdohms at 2012-03-20T09:56:48Z
@mvrhov so which is the most updated one you would say? i see a lot of them missing bit and pieces :P
---------------------------------------------------------------------------
by mvrhov at 2012-03-20T14:00:21Z
Sorry no idea.
---------------------------------------------------------------------------
by stof at 2012-03-20T19:09:10Z
@mvrhov Please don't do this. The translation component has a fallback mecanism so putting the EN string in an incomplete translation file is a bad idea as it forbids using a fallback.
---------------------------------------------------------------------------
by rdohms at 2012-03-20T20:14:50Z
@stof i figured as much.
Any other concerns before we push this PR further?
Commits
-------
93d2a4f [Translation] Ignore xliff entries with no "target" node
Discussion
----------
[Translation] Ignore xliff entries with no "target" node
This PR will ignore some entries with no "target" node when a xliff file is loaded.
```xml
<xliff xmlns="urn:oasis:names:tc:xliff:document:1.2" version="1.2">
<file source-language="en" datatype="plaintext" original="file.ext">
<body>
<trans-unit id="1">
<source>foo</source>
<target>bar</target>
</trans-unit>
<trans-unit id="2">
<source>extra</source>
</trans-unit>
</body>
</file>
</xliff>
```
Before:
```php
array(
'foo' => 'bar',
'extra' => ''
);
```
After:
```php
array(
'foo' => 'bar'
);
```
---------------------------------------------------------------------------
by fabpot at 2012-03-21T17:35:51Z
Wouldn't it be better to throw an exception for such cases? A `trans-unit` without a `target` is useless anyway, no?
---------------------------------------------------------------------------
by aerialls at 2012-03-21T20:25:19Z
I'm using transifex (https://www.transifex.net/home/) and when a user doesn't translate an entry, transifex fills up a `trans-unit` node without a `target` children.
Commits
-------
fc7c7f6 [Form] Fix min/max length guessing for numeric types (fix#3091)
Discussion
----------
[Form] Fix min/max length guessing for numeric types (fix#3091)
Before this PR, the length was guessed from `strlen(min/max)`.
This is obviously false for float: `strlen("1.123") > strlen ("5")` then this guess is now low confidence only and is masked by a `null` medium confidence guess for floats (implemented in both doctrine ORM & validator).
This PR also includes some code reorg in order to improve readability.
I'll update Propel & Mongo if needed once this is merged.
_note: `5.000` did neither work because of `5e3`_
---------------------------------------------------------------------------
by Koc at 2012-03-19T23:42:01Z
Will `strlen` works correctly with multibyte strings?
---------------------------------------------------------------------------
by vicb at 2012-03-19T23:58:33Z
could numeric types be multibyte strings ?
---------------------------------------------------------------------------
by Koc at 2012-03-20T00:07:24Z
I thought it somehow concerns `Symfony\Component\Validator\Constraints\MaxLengthValidator` too.
---------------------------------------------------------------------------
by vicb at 2012-03-20T00:20:33Z
This PR is about numeric types only and the MaxLengthValidator is [multibyte safe:](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Constraints/MaxLengthValidator.php#L45)
Commits
-------
4d4ef24 [Console] Stop parsing options after encountering "--" token
Discussion
----------
[Console] Stop parsing options after encountering "--" token
This enables support for arguments with leading dashes (e.g. "-1"), as supported by getopt in other languages.
[](http://travis-ci.org/jmikola/symfony)
The test suite currently fails due to 7a54fe41ca. ArgvInputTest passes, and these changes don't appear to break anything else.
Aside: This got me thinking about how one would pass an option value of "-1". I suppose for input options with `VALUE_OPTIONAL`, it would be ambiguous if "-1" followed; however, `VALUE_REQUIRED` should probably require that the next token is captured as the option value. In my tests, a required option value with a leading dash was interpreted as another option. The workaround for all of this is to use the space-less syntax (e.g. `-f=-1`).
---------------------------------------------------------------------------
by fabpot at 2012-03-17T08:43:15Z
AFAIK, the `--` should disable both option and argument parsing, no?
---------------------------------------------------------------------------
by jmikola at 2012-03-18T02:13:51Z
If that were the case, what would be the point of using `--` at all? :)
* http://wiki.bash-hackers.org/dict/terms/end_of_options
* http://perldoc.perl.org/Getopt/Long.html#Mixing-command-line-option-with-other-arguments
Commits
-------
e6577de Added a 'post validation' event to the form component.
Discussion
----------
[Form] Add post-validate event
Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: n/a
Fixes the following tickets: n/a
Todo: n/a
---------------------------------------------------------------------------
by fabpot at 2012-03-02T20:34:18Z
ping @bschussek
---------------------------------------------------------------------------
by vicb at 2012-03-04T09:19:53Z
I think this is a good idea (It was something missing to properly handle PersistentFile i.e. you should not persist invalid files)
---------------------------------------------------------------------------
by vicb at 2012-03-09T22:35:26Z
@jankramer please remove the second commit from this PR (see http://symfony.com/doc/current/contributing/code/patches.html) in order to make this mergeable.
---------------------------------------------------------------------------
by jankramer at 2012-03-10T09:26:04Z
@vicb done, sorry about that commit: overlooked the fact that it was on the same branch...
Commits
-------
5fa1c70 [json-response] Add a JsonResponse class for convenient JSON encoding
Discussion
----------
[json-response] Add a JsonResponse class for convenient JSON encoding
Usage example:
$data = array(user => $user->toArray());
return new JsonResponse($data);
---------------------------------------------------------------------------
by drak at 2012-02-16T11:51:11Z
@fabpot - maybe we could benefit with a bit more sub-namespacing in this component. One for Response for example and probably one for Request.
---------------------------------------------------------------------------
by Seldaek at 2012-02-16T15:07:31Z
@drak Please no. Moving the session was already a pain IMO since it was type-hinted in a few places (lack of interface, and interface doesn't include flash stuff still). Creating BC breaks just for fun like that is annoying for interop of bundles. It doesn't matter whether we have 10 or 15 classes in one directory.
---------------------------------------------------------------------------
by drak at 2012-02-17T08:33:46Z
@francodacosta The most optimal place is `__toString()`.
@Saldaek It just looks like the whole namespace is getting more cluttered. I suggest it because things like Request/Response objects are surely only going to grow over time. There is always the possibility to make BC for moved and renamed classes so there doesn't have to be any extra complications for making things look cleaner. Anyway, just a thought :-)
---------------------------------------------------------------------------
by stof at 2012-02-17T14:47:40Z
@drak Changing the namespace of a class is a BC break. The request and the response are used in many more places than the Session so it would be a real pain to update this. And the component is tagged with ``@api`` so BC breaks are forbidden without a good reason. The session refactoring was one as it was really an issue in the implementation, but simply renaming the class is not.
---------------------------------------------------------------------------
by fabpot at 2012-03-05T15:03:53Z
I'm -1 for adding this to the core. It does not add much value and why add a special response for JSON and not other formats?
---------------------------------------------------------------------------
by Seldaek at 2012-03-05T18:38:05Z
I think it's useful because it's a class we need in almost every project, and I don't think we're alone. It's super simple but makes me wonder every time why I have to recreate it. I don't want an additional bundle just for 3lines of code. Similarly I would say a JsonpResponse would be great, or maybe just an optional $callback arg to the json response to enable jsonp mode.
I just had someone ask me on irc how to do JSONP so while I think it's obvious and I'm sure you'd think that too, it obviously isn't to newcomers. The Response stuff is hidden behind those render methods & such and people don't realize they can simply subclass. If a few examples were in core it would be both helpful for learning and useful on a day to day basis.
As for other formats, well JSON is typically used nowadays, except when you want more fancy XML APIs, but for that the JMSSerializerBundle + FOSRestBundle are superior and we can't achieve such things in a few lines of code. I could also see a BinaryResponse or DownloadResponse or such that has proper "force-download" headers and accepts any binary stream, but that's another debate.
---------------------------------------------------------------------------
by dragoonis at 2012-03-05T19:43:05Z
I'm +1 for the concept but not commenting on how it should be implemented I'll leave that to other people.
Typically when you want to force a download you have to do ``content-disposition: attachment; filename="filehere.pdf"``
Modifying some response headers and the likes automatically for the user by returning a DownloadResponse object would be very handy..
I'm +1 for @Seldaek's point about examples of sub-classing for specific use cases. It will help with demonstrating how to do custom stuff the right way rather than people coming up with their own contraptions.
---------------------------------------------------------------------------
by stof at 2012-03-05T20:14:39Z
btw, regarding the BinaryResponse, there is a pending PR about it: #2606
---------------------------------------------------------------------------
by simensen at 2012-03-05T21:07:33Z
I'm +1 for providing reference implementations fo custom Response cases. I wanted to find best practices for handling JSONP requests/responses and couldn't find anything at all on the topic. I thought maybe extending Response might be useful but wasn't sure if that could be done safely or should be done at all.
---------------------------------------------------------------------------
by lsmith77 at 2012-03-05T22:28:01Z
@stof i think @drak was suggesting moving the class, but leaving an empty class extending from the new class in the old location to maintain BC
---------------------------------------------------------------------------
by stof at 2012-03-05T23:55:36Z
@lsmith77 This would force Symfony to use the BC class so that it does not break all typehints in existing code
---------------------------------------------------------------------------
by lsmith77 at 2012-03-06T00:22:15Z
BC hacks are never nice .. the goal would just be to eventually have all those classes and more importantly all new ones in a subnamespace. actually it might be easier to just leave all the classes in the old location and create new ones extending from the old ones. anyway .. personally i am also not such a big fan of these specialized responses .. but i guess i see FOSRestBundle as the alternative answer which makes me biased.
---------------------------------------------------------------------------
by Seldaek at 2012-03-06T07:57:36Z
I'm using FOSRestBundle when it's needed, but when you just have a small scale app that needs one or two json responses for specialized stuff it is slightly overkill. And again, newcomers probably won't know about it, and encouraging using it for simple use cases isn't exactly the best learning curve we can provide.
---------------------------------------------------------------------------
by COil at 2012-03-06T23:12:15Z
+1 for this. I have implemented such a function in all my sf1 projects, it will be the same for sf2.
---------------------------------------------------------------------------
by fabpot at 2012-03-15T13:22:27Z
Closing this PR in favor of a cookbook that explains how a developer can override the default Response class (this JSON class being a good example). see symfony/symfony-docs#1159
---------------------------------------------------------------------------
by Seldaek at 2012-03-15T13:25:08Z
Meh. Forcing people to copy paste code from the cookbook in every second project isn't exactly a step forward with regard to ease of use and user-friendliness.
---------------------------------------------------------------------------
by Seldaek at 2012-03-15T13:26:48Z
I mean following this logic, things like the X509 authentication should just be put in cookbooks too because almost nobody needs that. We have tons of code in the framework, I don't get the resistance with adding such a simple class which makes code more expressive.
---------------------------------------------------------------------------
by fabpot at 2012-03-15T13:53:07Z
because X509 authentication is not easy to get it right. Sending a JSON response is as simple as it can get:
new Response(json_encode($data), 200, array('Content-Type' => 'application/json'));
---------------------------------------------------------------------------
by marijn at 2012-03-15T13:54:25Z
Perhaps we need a `Symfony\Extensions\{Component}` namespace for things that don't necessarily belong in the core but are truly useful...
---------------------------------------------------------------------------
by Seldaek at 2012-03-15T14:03:40Z
I still fail to see why it doesn't belong in core.. There are tons of little helpers here and there, a base controller class made only of proxies, and then this gets turned down because it is simple to do it yourself? Sure it is simple, but it's repetitive and boring too. And while it's simple when you know your way around, some people aren't really sure how to do it.
The whole point of a framework is to avoid repetitive bullshit and be more productive. @fabpot do you have any real arguments against? I can see that you don't see a big use to it, fair enough, but do you see any downside at all?
Commits
-------
0e4f789 changed test config
a98d554 [SecurityBundle] Allow switching to the user that is already impersonated (fix#2554)
Discussion
----------
[Security] Disabled exception when switching to the user that is already impersonated
Bug fix: yes-ish
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #2554
Todo: -
---------------------------------------------------------------------------
by vicb at 2012-03-13T14:31:45Z
@meandmymonkey thank you for your work on this issue. Would you have time to add functional tests ?
---------------------------------------------------------------------------
by meandmymonkey at 2012-03-13T14:49:52Z
Probably not today, but during the next few days, yes, of course.
---------------------------------------------------------------------------
by meandmymonkey at 2012-03-14T18:05:19Z
@vicb @schmittjoh Writing the tests I noticed switching to an non-existent user will not raise an exception. While it's not a security issue, it should raise an error for completeness sake, shouldn't it?
---------------------------------------------------------------------------
by vicb at 2012-03-14T20:28:52Z
I think it should (throw an `AuthenticationCredentialsNotFoundException`). _btw there is an extra `sprintf` in the original code that could be remove when attempting to exit_
---------------------------------------------------------------------------
by meandmymonkey at 2012-03-14T21:13:16Z
The problem with throwing an `AuthenticationCredentialsNotFoundException` (or any other security exception for that matter) is that it derives from `AuthenticationException`, which means it gets caught by the framework and redirects to the login form, which is not what we want in this case.
We need to throw something 500-ish at [L89](d40b3376ec/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php (L89)), either a generic or a (new) custom Exception.
---------------------------------------------------------------------------
by meandmymonkey at 2012-03-14T21:43:57Z
IMHO a `LogicException`would be fine, like the one used at [L117](d40b3376ec/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php (L117)), as the error is not really about a failed authentication.
---------------------------------------------------------------------------
by vicb at 2012-03-14T21:49:04Z
I agree and btw very good job on the tests !
---------------------------------------------------------------------------
by meandmymonkey at 2012-03-14T22:12:43Z
Thanks :)
---------------------------------------------------------------------------
by vicb at 2012-03-15T08:01:13Z
Could you squash the commits, prefix the commit message with `[SecurityBundle]` and add `(fix#2554)` at the end ?
---------------------------------------------------------------------------
by meandmymonkey at 2012-03-15T08:53:12Z
Done.
---------------------------------------------------------------------------
by vicb at 2012-03-15T09:19:09Z
@fabpot this PR looks good to me.
---------------------------------------------------------------------------
by fabpot at 2012-03-15T12:50:50Z
Tests do not pass when you run them all.
---------------------------------------------------------------------------
by meandmymonkey at 2012-03-15T13:41:45Z
@fabpot @vicb With this config change, they pass when run together.
What is weird though is that the reason seems to be that the config for the profiler gets overwritten when running all tests together, while being used correctly when run alone. Any idea what can cause this? They should be isolated from each other.
The new config from 0e4f789 works, but enables the profiler for all SecurityBundle Tests... which is not strictly necessary.
Disabled exception when switching to the user that is already impersonated, exception is now only thrown when trying to switch to a new user.
Added an Excption exception when switching fails because target user does not exist.
Added funtional tests for switching users.
