Skip to content

[Validator] Simplified testing of violations #12015

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 1 commit into from
Sep 25, 2014

Conversation

webmozart
Copy link
Contributor

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

I simplified the assertion of violations in preparation of a replacement PR for #7276.

@webmozart webmozart force-pushed the 2.3-violation-tests branch 4 times, most recently from 7264f61 to 4d59ccf Compare September 24, 2014 11:18
use Symfony\Component\Validator\Constraints\Collection;
use Symfony\Component\Validator\Constraints\NotNull;
use Symfony\Component\Validator\Constraints\Range;
use Symfony\Component\Validator\Constraints\Choice;
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid moving use statements around. It does not change anything and make merging harder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, this happened automatically. Thanks.

@stof
Copy link
Member

stof commented Sep 24, 2014

IMO, instead of building a class with an assertNow method, it would be better to implement a PHPUnit constraint and then passing it to assertThat (assertViolation could be kept as a shortcut for it btw).

@webmozart webmozart force-pushed the 2.3-violation-tests branch 4 times, most recently from 3b9356b to 53b5033 Compare September 24, 2014 12:05
@webmozart
Copy link
Contributor Author

@stof assertThat() is usually not called directly, but through a facade method such as assertEquals(). The facade makes it impossible to use the builder pattern though.

However, let's give better names to assertViolation() and assertNow(), if you have any suggestions.

@@ -71,8 +71,7 @@ protected function createRepositoryMock()
{
$repository = $this->getMockBuilder('Doctrine\Common\Persistence\ObjectRepository')
->setMethods(array('findByCustom', 'find', 'findAll', 'findOneBy', 'findBy', 'getClassName'))
->getMock()
;
->getMock();
Copy link
Member

Choose a reason for hiding this comment

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

Sorry about that, but that's a wrong change coming from fabbot/PHP-CS-Fixer. That should be reverted.

@fabpot
Copy link
Member

fabpot commented Sep 24, 2014

What about:

$this->withViolation('invalid_message_key')
            ->assertParameter('{{ value }}', 'foo')
            ->assertParameter('{{ foo }}', 'bar')
            ->assertInvalidValue('foo')
            ->assertCode(Form::ERR_INVALID)
            ->done();

@webmozart
Copy link
Contributor Author

I changed it to

$this->buildViolation('invalid_message_key')
    ->setParameter('{{ value }}', 'foo')
    ->setParameter('{{ foo }}', 'bar')
    ->setInvalidValue('foo')
    ->setCode(Form::ERR_INVALID)
    ->assertRaised();

This way the API is equivalent to the violation builder API. Also, "raise" is the proper verb to go with violations.

@webmozart
Copy link
Contributor Author

The fabbot issues are due to a bug in fabbot. Unless Travis fails, this is good to merge from my side.

ping @symfony/deciders

@fabpot
Copy link
Member

fabpot commented Sep 25, 2014

👍

1 similar comment
@stof
Copy link
Member

stof commented Sep 25, 2014

👍

@fabpot
Copy link
Member

fabpot commented Sep 25, 2014

Thank you @webmozart.

@fabpot fabpot merged commit 8e5537b into symfony:2.3 Sep 25, 2014
fabpot added a commit that referenced this pull request Sep 25, 2014
This PR was merged into the 2.3 branch.

Discussion
----------

[Validator] Simplified testing of violations

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

I simplified the assertion of violations in preparation of a replacement PR for #7276.

Commits
-------

8e5537b [Validator] Simplified testing of violations
webmozart added a commit that referenced this pull request Sep 30, 2014
…multiple error causes (webmozart)

This PR was merged into the 2.6-dev branch.

Discussion
----------

[Validator] Added error codes to all constraints with multiple error causes

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

This PR depends on #12015 and #12016 being merged first. However, a few changes in 52cb7df first must be backported to #12016.

This PR introduces error codes for all constraints with multiple error paths. This lets you determine what exactly the reason was that a constraint failed:

```php
$validator = Validation::createValidator();

$violations = $validator->validate('0-4X19-92619812', new Isbn());

foreach ($violations as $violation) {
    var_dump($violation->getCode());
    // => int(3)
    var_dump(Isbn::getErrorName($violation->getCode()));
    // => string(24) "ERROR_INVALID_CHARACTERS"
    var_dump($violation->getConstraint()->getErrorName($violation->getCode()));
    // => string(24) "ERROR_INVALID_CHARACTERS"
}
```

The `getErrorName()` method is especially helpful for REST APIs, where you can return both an error code and a description of that error now.

**Todos**

- [x] Backport a few structural changes to #12016
- [x] Update constraints outside of the Validator component
- [x] Rebase on master (after merging #12015 and #12016)

Commits
-------

3b50bf2 [Validator] Added error codes to all constraints with multiple error causes
@webmozart webmozart deleted the 2.3-violation-tests branch October 22, 2014 17:20
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.

3 participants