Skip to content
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

[Form] Fixed check of violation constraint #12792 #13198

Merged
merged 1 commit into from
Jan 9, 2015

Conversation

xelaris
Copy link
Contributor

@xelaris xelaris commented Jan 2, 2015

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #12792
License MIT
Doc PR

ConstraintViolation::getConstraint() must not expect to provide a constraint as long as Symfony\Component\Validator\ExecutionContext exists (before 3.0)

@Tobion
Copy link
Member

Tobion commented Jan 2, 2015

Please provide a test case

@xelaris
Copy link
Contributor Author

xelaris commented Jan 2, 2015

tests added

@fabpot fabpot added the Form label Jan 2, 2015
@Tobion
Copy link
Member

Tobion commented Jan 2, 2015

ping @webmozart

@@ -66,7 +66,7 @@ protected function setUp()

private function getConstraintViolation($code = null)
{
return new ConstraintViolation($this->message, $this->messageTemplate, $this->params, null, 'prop.path', null, null, $code, new Form());
return $this->getMock('Symfony\Component\Validator\ConstraintViolation', array('getConstraint'), array($this->message, $this->messageTemplate, $this->params, null, 'prop.path', null, null, $code));
Copy link
Member

Choose a reason for hiding this comment

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

no need to use a mock IMO. Just add an argument in the getter to decide whether the last argument should be the constraint or null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was my first step. Than I realized that it is probably a good idea to test, if getConstraint is called by validateForm. Otherwise it could be simply removed without breaking the tests (which would also fix the bug). So I changed it to build a mock and added $violation->expects($this->atLeastOnce())->method('getConstraint') to the test cases.

Copy link
Member

Choose a reason for hiding this comment

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

if getConstraint is not called, then other tests should fail. don't mix both things.

ConstraintViolation::getConstraint() must not expect to provide a
constraint as long as Symfony\Component\Validator\ExecutionContext
exists (before 3.0)
@xelaris
Copy link
Contributor Author

xelaris commented Jan 9, 2015

As @Tobion said, I have changed the commit so that it is discrete and only test the particular case.

@stof
Copy link
Member

stof commented Jan 9, 2015

👍

@fabpot
Copy link
Member

fabpot commented Jan 9, 2015

Thank you @xelaris.

@fabpot fabpot merged commit aedabc7 into symfony:2.6 Jan 9, 2015
fabpot added a commit that referenced this pull request Jan 9, 2015
This PR was merged into the 2.6 branch.

Discussion
----------

[Form] Fixed check of violation constraint #12792

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #12792
| License       | MIT
| Doc PR        |

ConstraintViolation::getConstraint() must not expect to provide a constraint as long as Symfony\Component\Validator\ExecutionContext exists (before 3.0)

Commits
-------

aedabc7 [Form] Fixed check of violation constraint #12792
fabpot added a commit that referenced this pull request Apr 12, 2020
…hDude)

This PR was merged into the 3.4 branch.

Discussion
----------

[Form] Removed legacy check in `ValidationListener`

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | ~
| License       | MIT
| Doc PR        | ~

A left over of #13198, should have been removed in 3.0. The tests don't use `null` anymore, no update needed here, this is just about removing dead code.

Commits
-------

e479e51 [Form] Removed legacy check in `ValidationListener`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants