[Validator] Refactored the Validator for use in Drupal #6096
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| */ | ||
| public function validateValue($value, $constraints, $groups = null, $subPath = '') | ||
| { | ||
| $constraints = is_array($constraints) ? $constraints : array($constraints); |
There was a problem hiding this comment.
$constraints = (array) $constraints; ?
There was a problem hiding this comment.
The (array) cast converts objects to arrays of their properties.
There was a problem hiding this comment.
Ah right, forgot this method accepts also objects... =)
…Visitor design pattern. With this refactoring comes a decoupling of the validator from the structure of the underlying metadata. This way it is possible for Drupal to use the validator for validating their Entity API by using their own metadata layer, which is not modeled as classes and properties/getter methods.
fabpot
added a commit
that referenced
this pull request
Nov 24, 2012
This PR was merged into the master branch. Commits ------- 1858b96 [Form] Adapted FormValidator to latest changes in the Validator 1f752e8 [DoctrineBridge] Adapted UniqueValidator to latest changes in the Validator efe42cb [Validator] Refactored the GraphWalker into an implementation of the Visitor design pattern. Discussion ---------- [Validator] Refactored the Validator for use in Drupal Bug fix: no Feature addition: no Backwards compatibility break: yes Symfony2 tests pass: yes Fixes the following tickets: - Todo: - License of the code: MIT Documentation PR: TODO Drupal wants to use the Symfony Validator component in their next version. I was talking to @fago recently about the changes that we'd need to make and implemented these changes in this PR. I don't want to rush this, but the deadline is tight, since Drupal feature freeze is on December 1st and @fago needs at least a couple of days to integrate the Validator into Drupal. This PR introduces two significant changes: * Interfaces were created for all classes that constitute the Validator's API. This is were the PR breaks BC, because `ConstraintValidatorInterface::initialize()` is now type hinted against `ExecutionContextInterface` instead of `ExecutionContext`. * The graph walker was refactored into an implementation of the Visitor pattern. This way, the validator was decoupled from the structure of the metadata (class → properties and getter methods) and makes it possible to implement a different metadata structure, as is required by the Drupal Entity API. As a consequence of the API change, custom validation code is now much easier to write, because `ValidatorInterface` and `ExecutionContextInterface` share the following set of methods: ```php interface ValidatorInterface { public function validate($value, $groups = null, $traverse = false, $deep = false); public function validateValue($value, $constraints, $groups = null); public function getMetadataFor($value); } interface ExecutionContextInterface { public function validate($value, $subPath = '', $groups = null, $traverse = false, $deep = false); public function validateValue($value, $constraints, $subPath = '', $groups = null); public function getMetadataFor($value); } ``` No more juggling with property paths, no more fiddling with the graph walker. Just call on the execution context what you'd call on the validator and you're done. There are two controversial things to discuss and decide (cc @fabpot): * I moved the `@api` tags of all implementations to the respective interfaces. Is this ok? * I would like to deprecate `ValidatorInterface::getMetadataFactory()` (tagged as `@api`) in favor of the added `ValidatorInterface::getMetadataFor()`, which offers the exact same functionality, but with a different API and better encapsulation, which makes it easier to maintain for us. We can tag `getMetadataFor()` as `@api`, as I don't expect it to change. Can we do this or should we leave the old method in? I would like to decide the major issues of this PR until **Sunday November 25th** in order to give @fago enough room for his implementation. Let me hear your thoughts.
|
This pr breaks validation if used Asserts in Entity annotations. namespace Millwright\UserBundle\Entity;
use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Validator\Constraints as Assert;
/**
* @ORM\Table(name="user")
* @ORM\Entity
*/
class User {
/**
* @ORM\Column(name="id", type="integer")
* @ORM\Id
* @ORM\GeneratedValue(strategy="AUTO")
*/
protected $id;
/**
* @ORM\Column(name="email", type="string", length=255)
* @Assert\Email()
*/
protected $email;
...
}in controller: use Symfony\Bundle\FrameworkBundle\Controller\Controller;
class UserAdminController extends Controller
{
public function createAction()
{
$form = $this->createForm('user_create');
$form->bind($this->getRequest());
$result = $form->isValid();
// Always true, if email is invalid
...
}
} |
|
I can confirm this. |
fabpot
added a commit
that referenced
this pull request
Nov 28, 2013
This PR was merged into the 2.3 branch. Discussion ---------- [Validator] Replaced inexistent interface | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | #9631 | License | MIT | Doc PR | - `ClassMetadataFactoryInterface` was removed in 2.3 (#6096) and replaced with `MetadataFactoryInterface`. Commits ------- 8fd3256 [Validator] Replaced inexistent interface.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
License of the code: MIT
Documentation PR: TODO
Drupal wants to use the Symfony Validator component in their next version. I was talking to @fago recently about the changes that we'd need to make and implemented these changes in this PR. I don't want to rush this, but the deadline is tight, since Drupal feature freeze is on December 1st and @fago needs at least a couple of days to integrate the Validator into Drupal.
This PR introduces two significant changes:
ConstraintValidatorInterface::initialize()is now type hinted againstExecutionContextInterfaceinstead ofExecutionContext.As a consequence of the API change, custom validation code is now much easier to write, because
ValidatorInterfaceandExecutionContextInterfaceshare the following set of methods:No more juggling with property paths, no more fiddling with the graph walker. Just call on the execution context what you'd call on the validator and you're done.
There are two controversial things to discuss and decide (cc @fabpot):
@apitags of all implementations to the respective interfaces. Is this ok?ValidatorInterface::getMetadataFactory()(tagged as@api) in favor of the addedValidatorInterface::getMetadataFor(), which offers the exact same functionality, but with a different API and better encapsulation, which makes it easier to maintain for us. We can taggetMetadataFor()as@api, as I don't expect it to change. Can we do this or should we leave the old method in?I would like to decide the major issues of this PR until Sunday November 25th in order to give @fago enough room for his implementation.
Let me hear your thoughts.