Added ability to set controller result in the kernel.view event
merged branch ltouroumov/2.1 (PR #5885)

This PR was submitted for the 2.1 branch but it was merged into the master branch instead (closes #5885).

Commits
-------

efc22a8 [HttpKernel] Added ability to set controller result in the kernel.view event

Discussion
----------

[HttpKernel] Added ability to set controller result in the kernel.view event

Added the method `GetResponseForControllerResultEvent:setControllerResult(array $controllerResult)` to allow kernel.view events to inject data into the result.

---------------------------------------------------------------------------

by stof at 2012-11-01T13:48:51Z

new features canot be added in the 2.1 branch, which is a maintenance branch and so receives only bugfixes.

---------------------------------------------------------------------------

by fabpot at 2012-11-02T07:13:56Z

I'm not opposed to the change but can you share your use case with us?

---------------------------------------------------------------------------

by ltouroumov at 2012-11-02T07:55:55Z

I have custom annotations that allow controller methods to be invoked before the actions is run (`@BeforeFilter`) and after the action is run (`@ViewFilter`) and after the response is sent (`@ResponseFilter`).
The view filter allows to alter the controller result for example to inject parent entities into the template.

If you need more details I suppose I could post the code but I would have to ask the boss first.

---------------------------------------------------------------------------

by ltouroumov at 2012-11-02T07:57:53Z

Also I fixed the missing dollar sign in the [`6ffea1dc1cfa23844c6d90a1bfa9a282d1807a61`](https://github.com/ltouroumov/symfony/commit/6ffea1dc1cfa23844c6d90a1bfa9a282d1807a61) commit. But I can't seem to attach it to the pull request.

---------------------------------------------------------------------------

by henrikbjorn at 2012-11-02T08:12:20Z

Needs tests.

---------------------------------------------------------------------------

by ltouroumov at 2012-11-02T08:38:03Z

Is it really necessary to write a test for a simple setter ? I'm not opposed to testing, just looking for a justification. And should I create a new test case or is there one ?

---------------------------------------------------------------------------

by henrikbjorn at 2012-11-02T08:42:49Z

Since your code had a clear syntax error i think that a test is in order ;)

---------------------------------------------------------------------------

by ltouroumov at 2012-11-02T08:45:03Z

Can't argue with that !

Would putting it in `Symfony\Component\HttpKernel\Tests\EventListener\ControllerResponseTest` do the trick ?

---------------------------------------------------------------------------

by henrikbjorn at 2012-11-02T09:16:55Z

Sounds fair enough.

---------------------------------------------------------------------------

by ltouroumov at 2012-11-02T09:31:09Z

This tests covers my use case(s), but there might be others I didn't think of.

---------------------------------------------------------------------------

by ludekstepan at 2012-11-20T17:15:45Z

+1
Please merge into master, I have a use case for it, now I'm relying on a Reflection to set the private property, but that's really a dirty solution. The setter would be perfect!
tweaked previous merge
Try to make sure cookies get deleted from the TokenProvider when no longer in use
small fix of #5984 when the container param is not set

this can happen when the config for the router is unset, but this method
does not need to depend on routing. reading an unset config would raise an exception.
Xliff with other node than source or target are ignored
merged branch xkobal/2.1 (PR #6079)

This PR was squashed before being merged into the 2.1 branch (closes #6079).

Commits
-------

bbeff54 Xliff with other node than source or target are ignored

Discussion
----------

Xliff with other node than source or target are ignored

Referring to the xliff XSD, the format can allow other nodes like <note>. Check only count is dangerous if others nodes are present, and we aren't sure that nodes are the two we wan't (source and target)

And the real problem if that if there is other node in translation, the translation is silently ignored.

---------------------------------------------------------------------------

by stloyd at 2012-11-20T17:44:47Z

You should add test that covers this, as well as #6078 should be closed (branch `2.1` is merged from time to time to `master`).

---------------------------------------------------------------------------

by xkobal at 2012-11-21T11:13:32Z

I have added a new node to xliff fixtures and edit test to be sure node isn't ignored.
Add Route::hasOption() and Route::hasRequirement() methods.
merged branch Crell/route-requirements (PR #6098)

This PR was merged into the master branch.

Commits
-------

b930066 Add Route::hasOption() and Route::hasRequirement() methods.

Discussion
----------

Add Route::hasOption() and Route::hasRequirement() methods.

It seems odd that there's a hasDefault() method but not for options and requirements.  Drupal has logic that depends on the existence of a given requirement.  So let's add that.

I think I got the coding standards right the first time for once... :-)
merged branch Tobion/patch-8 (PR #6071)

This PR was merged into the 2.0 branch.

Commits
-------

29bfa13 small fix of #5984 when the container param is not set

Discussion
----------

small fix of #5984 when the container param is not set

 this can happen when the config for the router is unset, but this method does not need to depend on routing. reading an unset config would raise an exception.

---------------------------------------------------------------------------

by Tobion at 2012-11-19T20:44:31Z

Ops, I guess it's wrong. Travis will probably confirm this in a moment.
I will correct that.

---------------------------------------------------------------------------

by flojon at 2012-11-20T22:40:07Z

Yeah you changed the logic...

---------------------------------------------------------------------------

by Tobion at 2012-11-21T14:42:48Z

ok it's fixed.
fixed CS
merged branch Tobion/patch-1 (PR #5104)

This PR was merged into the master branch.

Commits
-------

e2a50ef [OptionsResolver] fix normalizer without corresponding option
5a53821 [OptionsResolver] fix removing normalizers

Discussion
----------

OptionsResolver: normalizer fix

setNormalizer() -> replace() -> all() would generate an error.

---------------------------------------------------------------------------

by bschussek at 2012-07-29T16:09:20Z

Thank you for the fix! Could you please add a test case?

---------------------------------------------------------------------------

by Tobion at 2012-07-30T15:42:26Z

There is another problem: setNormalizer() (without setting an option) -> all()
I suggest to simply ignore normalizers that have no corresponding option. Do you agree?

---------------------------------------------------------------------------

by Tobion at 2012-07-30T16:19:24Z

On the other hand, one could argue that a normalizer without option should also work like this:
```
$this->options->setNormalizer('foo', function (Options $options) {
        return '';
});
$this->assertEquals(array('foo' => ''), $this->options->all());
```

But when having a normalizer that wants a previous value as param, it does not work (because there is none).

---------------------------------------------------------------------------

by stof at 2012-07-30T16:30:34Z

@Tobion according to github, this need to be rebased

---------------------------------------------------------------------------

by bschussek at 2012-07-30T19:16:48Z

I guess setNormalizer() should check whether the option is set and fail otherwise. The second possibility, as you say, is to ignore them in all(). I'd prefer whatever is more efficient.

---------------------------------------------------------------------------

by bschussek at 2012-07-30T19:17:27Z

But setting a normalizer without setting an option, and having that option appear in the final options, does not make sense if you ask me.

---------------------------------------------------------------------------

by Tobion at 2012-07-30T21:23:46Z

Well it could make sense. If you want to override/normalize an option to a given value however it has been overloaded by others or just not overloaded at all. This is what normalizers do. I think its more consistent than the other solutions.
Raising exception in setNormalizer would make the Class dependent on the order you call the methods, e.g. `setNormalizer(); set()` would not work. But the other way round would be ok.
Ignoring some normalizers in `all` would be strange because they are there but not applied under some circumstances.

---------------------------------------------------------------------------

by Tobion at 2012-07-30T21:42:40Z

Added the fix. If you disagree tell me.

---------------------------------------------------------------------------

by bschussek at 2012-08-04T09:30:18Z

> Raising exception in setNormalizer would make the Class dependent on the order you call the methods, e.g. `setNormalizer(); set()` would not work. But the other way round would be ok.

I think this would be a better solution. I dislike if the normalizer magically adds an option that does not exist. This could hide implementation error, e.g. when a refactoring removes an option, but the normalizer is forgotten. Can you throw an exception in this case?

Should we find use cases that rely on this to work, we can soften the behavior and remove the exception.

---------------------------------------------------------------------------

by Tobion at 2012-08-04T15:02:51Z

Well, that would also make it impossible to set a normalizer for on optional option in OptionsResolver.
So `setOptional` + `setNormalizers` would throw an exception which sounds counter-intuitive. Are you sure about that?

---------------------------------------------------------------------------

by Tobion at 2012-08-17T11:47:58Z

ping @bschussek

---------------------------------------------------------------------------

by Tobion at 2012-10-07T22:31:44Z

@bschussek ping

---------------------------------------------------------------------------

by stof at 2012-10-13T18:04:30Z

@bschussek ping

---------------------------------------------------------------------------

by Tobion at 2012-11-08T09:55:15Z

@bschussek please let's get this finished.
merged branch bamarni/preloaded-extension (PR #5479)

This PR was merged into the 2.1 branch.

Commits
-------

84635bd [Form] allowed no type guesser to be registered

Discussion
----------

[Form] made the factory builder pass null when no type guesser registered

reopened #5422 against 2.1 as it's a bug fix

---------------------------------------------------------------------------

by stof at 2012-10-13T21:23:34Z

@fabpot anything left for this PR ?

---------------------------------------------------------------------------

by fabpot at 2012-10-14T09:41:29Z

@bamarni Can you add some unit tests and also update the FormExtensionInterface interface phpdoc as `getTypeGuesser` can now return `null`? Thanks. ping @bschussek

---------------------------------------------------------------------------

by bamarni at 2012-10-14T17:10:27Z

I've added a few tests covering this.

@fabpot : the phpdoc is already correct, it currently can return null, this only occurs with this convenient class.

---------------------------------------------------------------------------

by bschussek at 2012-10-16T07:43:41Z

This PR breaks FormFactory::createBuilderForProperty(), which expects a guesser to be present. Can you check the component for other uses of the guesser and add a null-check there?

---------------------------------------------------------------------------

by bamarni at 2012-10-16T10:57:54Z

I cannot find other places than the factory (searching for 'getTypeGuesser').

---------------------------------------------------------------------------

by bschussek at 2012-11-08T16:58:37Z

You should also adapt `FormRegistry::getTypeGuesser()` not to build a `FormTypeGuesserChain` if the array of guessers is empty. In that case it will return now `null` (adapt the doc block). We also need a different was of checking if the type guessers have already been parsed in FormRegistry. Otherwise the first if condition in `FormRegistry::getTypeGuesser()` will never become false. You could for example initialize the property `$guesser` to `false` and only set it to `null` after the first run of `getTypeGuesser()`.

---------------------------------------------------------------------------

by bamarni at 2012-11-08T18:40:00Z

good catch I had missed it! I've applied your suggestion in the latest commit. Do you see anything else before I squash?

---------------------------------------------------------------------------

by bschussek at 2012-11-08T18:45:15Z

A test for `FormRegistry::getTypeGuesser()` would of course be awesome.

---------------------------------------------------------------------------

by bamarni at 2012-11-08T18:52:13Z

Then it was already awesome! (see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Tests/FormRegistryTest.php#L252)

I've also added one for the null case if it's what you meant.
merged branch sterrien/patch-1 (PR #5842)

This PR was merged into the 2.0 branch.

Commits
-------

2d9a6fc Use Norm Data instead of Data

Discussion
----------

[Form] Use Norm Data instead of App Data

This listener is triggered when normalized data are binded.

We have to use $event->getForm()->getNormData() instead of $event->getForm()->getData().

I have made a new FormType having 'entity' as parent and having a NormTransformer. I encountered a problem in MergeCollectionListener when the request is binded.

My commit fix it.
merged branch armetiz/patch-9 (PR #6021)

This PR was merged into the 2.1 branch.

Commits
-------

82334d2 Force loader to be null or a EntityLoaderInterface

Discussion
----------

Force loader to be null or a EntityLoaderInterface
Update src/Symfony/Component/Form/Extension/Core/Type/FileType.php
merged branch stfalcon/patch-1 (PR #5028)

This PR was submitted for the master branch but it was merged into the 2.1 branch instead (closes #5028).

Commits
-------

5bfe757 Update src/Symfony/Component/Form/Extension/Core/Type/FileType.php

Discussion
----------

Update src/Symfony/Component/Form/Extension/Core/Type/FileType.php

fixed https://github.com/dustin10/VichUploaderBundle/issues/27

---------------------------------------------------------------------------

by bschussek at 2012-07-24T12:44:11Z

Thank you for the PR! Could you please add a test case?

---------------------------------------------------------------------------

by stfalcon at 2012-07-25T13:53:24Z

> Could you please add a test case?

And what to check? I added one static option :)

---------------------------------------------------------------------------

by bschussek at 2012-07-25T14:22:40Z

Whatever was the reason for adding the option ;) The test should fail when the option is not added and succeed when the option is there.

Probably it is sufficient to create a new field of type "file" in the test which comes prefilled with a `File` object.

```
$file = $this->getMock('Symfony\Component\HttpFoundation\File\File');
$this->factory->create('file', $file)
```

---------------------------------------------------------------------------

by stfalcon at 2012-11-15T12:32:01Z

sorry, it's bug in VichUploaderBundle

---------------------------------------------------------------------------

by stfalcon at 2012-11-21T17:00:59Z

or not :)

---------------------------------------------------------------------------

by stfalcon at 2012-11-22T19:47:34Z

@bschussek done! it was really a bug with FileType

---------------------------------------------------------------------------

by stfalcon at 2012-11-22T22:15:18Z

@stof who can merge it? I want close this bug https://github.com/dustin10/VichUploaderBundle/issues/27 :)

---------------------------------------------------------------------------

by stof at 2012-11-23T02:15:46Z

@stfalcon the rule is that only @fabpot merges PRs on symfony.

---------------------------------------------------------------------------

by stfalcon at 2012-11-23T10:12:05Z

@fabpot do you have a minute :)? it's simple PR but many people wait for it
[Validator] Refactored the GraphWalker into an implementation of the Visitor design pattern.

With this refactoring comes a decoupling of the validator from the structure of
the underlying metadata. This way it is possible for Drupal to use the validator
for validating their Entity API by using their own metadata layer, which is not
modeled as classes and properties/getter methods.
[DoctrineBridge] Adapted UniqueValidator to latest changes in the Validator
[Form] Adapted FormValidator to latest changes in the Validator