[OptionsResolver] fix removing normalizers
[OptionsResolver] fix normalizer without corresponding option
Try to make sure cookies get deleted from the TokenProvider when no longer in use
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.
Handle case of static controller method and controllers using magic __call() method
merged branch sli-systems/dc-1 (PR #6080)

This PR was squashed before being merged into the master branch (closes #6080).

Commits
-------

e477a2e Handle case of static controller method and controllers using magic __call() method

Discussion
----------

Handle case of static controller method and controllers using magic __call() method

Improve collecting controller details for edge cases where:
- controller is array, but contains class name and static method
- method doesn't exist, but is handled by magic __call() method

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

by fabpot at 2012-11-21T08:12:08Z

Can you add some unit tests?

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

by sli-systems at 2012-11-21T22:19:17Z

@pierredup
I disagree with the your comment about is_callable() only working with objects. The PHP docs state that the first  argument is a callable, so it can be a string, array, closure, and perhaps more.

The test I added also shows that the code works as is :)

I've thought about your suggestion of adding reflection to look up the location of __call(). However, I think this doesn't really  add a lot and only complicates matters. Also, as you can see in the new test, there is also __callStatic() to consider.

The fact that file/line are n/a is  correct, because the most typical case will be that __call() and __callStatic() will delegate to some other method that might not even be in the same class/file (a subclass I would expect), IMHO.

@fabpot
Good catch  about the '/'. I hope the test is complete enough. Looks more like an exercise on PHP callables than anything else, tho ;)

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

by pierredup at 2012-11-22T04:56:18Z

True that ````is_callable```` takes any callable argument, except in the one specific case where you have a ````__call()```` method, and pass an array with the first paramater as a string.

Take the following example:

    class Controller {
        public function __call($method, $arguments) {}
    }

    $controller = array('Controller', 'action');

    var_dump(is_callable($controller));

Here ````is_callable($controller)```` will actually return ````false````, where if you have ````$controller = array(new Controller, 'action');```` it would return true.

Of course if you have a ````__callStatic```` method, then it would always return true.

Your tests doesn't seem to cover this use case

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

by sli-systems at 2012-11-22T20:27:05Z

Hmm, maybe. I have to admin that I do not know about this case. OTOH, if is_callable returns false is it really callable then? I would think this more of a PHP bug then?

I think I might have come across this case during coding, but then dismissed it because in that case FilterControllerEvent failed already before the data collector code is reached.

In FilterControllerEvent there is a check on is_callable and a LogicException is thrown if $controller is not callable.

So, is FilterControllerEvent wrong  too then?

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

by pierredup at 2012-11-22T20:41:14Z

One would think that if is_callable returns false, then the controller isn't callable, but in the case I mentioned above, the controller is in fact callable. I also thought it was a bug with php, but the php-internals don't seem to think so.

The problem is, if you specify the class as a string, php looks for a static method, even if you have a __call method, it won't be registered.

I will have a look at the FilterControllerEvent to see if this use case applies there as well.

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

by sli-systems at 2012-11-22T20:50:32Z

Rather strange - if that is the case then using is_callable seems pretty pointless and the only way would be to try to execute the controller to find out if it is, in fact, callable...

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

by pierredup at 2012-11-22T20:51:07Z

Okay so it actually seems that the case above isn't callable after all. If the controller is specified as a string, then a static method need to exist. Hence why it works with __callStatic. Only when an instance of the class is specified, will it handle the __call method.

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

by sli-systems at 2012-11-22T20:57:55Z

So the tests are sufficient then?

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

by pierredup at 2012-11-22T20:59:22Z

Yes it is.
This happens when you just assume something without actually testing it :)

Sorry for the hassle
merged branch TerjeBr/persistent-token-provider (PR #6055)

This PR was merged into the master branch.

Commits
-------

d1b5093 Try to make sure cookies get deleted from the TokenProvider when no longer in use

Discussion
----------

Delete cookies from the TokenProvider that is no longer in use

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Todo: -
License of the code: MIT

When the user logs in, or login fails for some reason, the old "remember me" cookie should be deleted from the TokenProvider if you are using the PersistentTokenBasedRememberMeServices.

As the code is now, the token is only deleted on logout.

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

by TerjeBr at 2012-11-20T13:45:54Z

So, anything else that needs to be done before this is merged?

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

by TerjeBr at 2012-11-21T10:30:53Z

Ok, I have corrected the typo in the comment and squashed the commit.

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

by schmittjoh at 2012-11-21T10:36:29Z

btw, ``canceled`` (more American) and ``cancelled`` (more British) are both
correct English forms.

On Wed, Nov 21, 2012 at 11:30 AM, Terje Bråten <notifications@github.com>wrote:

> Ok, I have corrected the typo in the comment and squashed the commit.
>
> —
> Reply to this email directly or view it on GitHub<https://github.com/symfony/symfony/pull/6055#issuecomment-10592112>.
>
>

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

by schmittjoh at 2012-11-21T10:40:24Z

As a side-note have you verified that this does not break the cookie theft protection?

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

by TerjeBr at 2012-11-21T10:51:10Z

Yes, cookie theft protection is still there and is functioning well.

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

by TerjeBr at 2012-11-21T11:14:04Z

I am using this together with the DoctrineTokenProvider in issue #6057 in my own project and done some extensive testing on it.

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

by TerjeBr at 2012-11-23T10:30:34Z

Is this ready to be merged now?