Skip to content

OptionsResolver: normalizer fix #5104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 24, 2012
Merged

OptionsResolver: normalizer fix #5104

merged 2 commits into from
Nov 24, 2012

Conversation

Tobion
Copy link
Contributor

@Tobion Tobion commented Jul 29, 2012

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

@webmozart
Copy link
Contributor

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

@Tobion
Copy link
Contributor Author

Tobion commented Jul 30, 2012

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

@Tobion
Copy link
Contributor Author

Tobion commented Jul 30, 2012

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).

@stof
Copy link
Member

stof commented Jul 30, 2012

@Tobion according to github, this need to be rebased

@webmozart
Copy link
Contributor

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.

@webmozart
Copy link
Contributor

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.

@Tobion
Copy link
Contributor Author

Tobion commented Jul 30, 2012

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.

@Tobion
Copy link
Contributor Author

Tobion commented Jul 30, 2012

Added the fix. If you disagree tell me.

@webmozart
Copy link
Contributor

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.

@Tobion
Copy link
Contributor Author

Tobion commented Aug 4, 2012

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?

@Tobion
Copy link
Contributor Author

Tobion commented Aug 17, 2012

ping @bschussek

@travisbot
Copy link

This pull request passes (merged e2a50ef into 2c0e851).

@Tobion
Copy link
Contributor Author

Tobion commented Oct 7, 2012

@bschussek ping

1 similar comment
@stof
Copy link
Member

stof commented Oct 13, 2012

@bschussek ping

@Tobion
Copy link
Contributor Author

Tobion commented Nov 8, 2012

@bschussek please let's get this finished.

fabpot added a commit that referenced this pull request Nov 24, 2012
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.
@fabpot fabpot merged commit e2a50ef into symfony:master Nov 24, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants