NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Validator] Reset constraint options#20122

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:2.7fromro0NL:validator/reset-options

Oct 2, 2016

Merged

[Validator] Reset constraint options#20122

fabpot merged 1 commit intosymfony:2.7fromro0NL:validator/reset-options

Oct 2, 2016

Conversation

Copy link

Contributor

ro0NL commentedOct 1, 2016•
edited
Loading

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#20106
License	MIT
Doc PR	reference to the documentation PR, if any

I think it's good to reset the internal pointer of the options array if we depend onkey($options). I've added tests to clarify we intentionally check for the first key.

The current behavior actually differs for hhvm/php7 -https://3v4l.org/e6r6E

carsonbot added Status: Needs Review Validator Bug labels

Oct 1, 2016

reset constraint options

4c6ddd4

lemoinem approved these changes

Oct 1, 2016

View reviewed changes

Copy link

Contributor

lemoinem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks for the contribution. Looks good to me.

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Oct 1, 2016

Copy link

Member

fabpot commentedOct 2, 2016

Have you check if this is the only place where we need that?

Copy link

Contributor

lemoinem commentedOct 2, 2016•
edited
Loading

@fabpot I grep'd through the Validator Component forkey() orcurrent().

The only other time they are used is in theYamlFileLoader. And I don't think it's possible for this array to have a misplaced internal pointer (single element array, generated by the Yaml Parser).

Copy link

Member

fabpot commentedOct 2, 2016

I was thinking about the other components and bundles as well.

Copy link

Contributor

@fabpot I double checked the rest of symfony's code. As far as I can tell, all other uses ofcurrent() andkey() are safe (most are on newly built arrays, the others are on arrays being iterated on).

Copy link

ContributorAuthor

ro0NL commentedOct 2, 2016

We should be careful though, it needs to be looked at on a per case basis.

I've double checked all subclasses, and overridden constructors are good to go.

Copy link

Member

fabpot commentedOct 2, 2016

Thank you@ro0NL.

fabpot merged commit4c6ddd4 intosymfony:2.7

Oct 2, 2016

fabpot added a commit that referenced this pull request

Oct 2, 2016

bug#20122[Validator] Reset constraint options (ro0NL)

80577f9

This PR was merged into the 2.7 branch.Discussion----------[Validator] Reset constraint options| Q             | A| ------------- | ---| Branch?       | 2.7| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#20106| License       | MIT| Doc PR        | reference to the documentation PR, if anyI think it's good to reset the internal pointer of the options array if we depend on `key($options)`. I've added tests to clarify we intentionally check for the first key.The current behavior actually differs for hhvm/php7 -https://3v4l.org/e6r6ECommits-------4c6ddd4 reset constraint options