From be7f055237bf877f72e1bafdd17ac4a2f9ad14b4 Mon Sep 17 00:00:00 2001 From: Bernhard Schussek Date: Fri, 21 Feb 2014 16:07:33 +0100 Subject: [PATCH] [Validator] Visitors may now abort the traversal by returning false from beforeTraversal() --- .../NodeTraverser/NodeTraverserInterface.php | 11 ++- .../NonRecursiveNodeTraverser.php | 84 ++++++++++++----- .../Validator/NodeVisitor/AbstractVisitor.php | 4 +- .../NodeVisitor/NodeValidationVisitor.php | 89 +++++++++++------- .../NodeVisitor/NodeVisitorInterface.php | 26 +++++- .../ObjectInitializationVisitor.php | 43 +++++++-- .../NonRecursiveNodeTraverserTest.php | 91 +++++++++++++++++++ 7 files changed, 280 insertions(+), 68 deletions(-) create mode 100644 src/Symfony/Component/Validator/Tests/NodeTraverser/NonRecursiveNodeTraverserTest.php diff --git a/src/Symfony/Component/Validator/NodeTraverser/NodeTraverserInterface.php b/src/Symfony/Component/Validator/NodeTraverser/NodeTraverserInterface.php index 37c7c8c22c..36e559ba68 100644 --- a/src/Symfony/Component/Validator/NodeTraverser/NodeTraverserInterface.php +++ b/src/Symfony/Component/Validator/NodeTraverser/NodeTraverserInterface.php @@ -25,10 +25,13 @@ use Symfony\Component\Validator\NodeVisitor\NodeVisitorInterface; * {@link \Symfony\Component\Validator\NodeVisitor\NodeVisitorInterface::visit()} * of each visitor is called. At the end of the traversal, the traverser invokes * {@link \Symfony\Component\Validator\NodeVisitor\NodeVisitorInterface::afterTraversal()} - * on each visitor. + * on each visitor. The visitors are called in the same order in which they are + * added to the traverser. * - * The visitors should be called in the same order in which they are added to - * the traverser. + * If the {@link traverse()} method is called recursively, the + * {@link \Symfony\Component\Validator\NodeVisitor\NodeVisitorInterface::beforeTraversal()} + * and {@link \Symfony\Component\Validator\NodeVisitor\NodeVisitorInterface::afterTraversal()} + * methods of the visitors will be invoked for each call. * * The validation graph typically contains nodes of the following types: * @@ -92,5 +95,5 @@ interface NodeTraverserInterface * @param Node[] $nodes The nodes to traverse * @param ExecutionContextInterface $context The validation context */ - public function traverse(array $nodes, ExecutionContextInterface $context); + public function traverse($nodes, ExecutionContextInterface $context); } diff --git a/src/Symfony/Component/Validator/NodeTraverser/NonRecursiveNodeTraverser.php b/src/Symfony/Component/Validator/NodeTraverser/NonRecursiveNodeTraverser.php index b931b96943..811849929b 100644 --- a/src/Symfony/Component/Validator/NodeTraverser/NonRecursiveNodeTraverser.php +++ b/src/Symfony/Component/Validator/NodeTraverser/NonRecursiveNodeTraverser.php @@ -68,11 +68,6 @@ class NonRecursiveNodeTraverser implements NodeTraverserInterface */ private $metadataFactory; - /** - * @var Boolean - */ - private $traversalStarted = false; - /** * Creates a new traverser. * @@ -104,20 +99,20 @@ class NonRecursiveNodeTraverser implements NodeTraverserInterface /** * {@inheritdoc} */ - public function traverse(array $nodes, ExecutionContextInterface $context) + public function traverse($nodes, ExecutionContextInterface $context) { - // beforeTraversal() and afterTraversal() are only executed for the - // top-level call of traverse() - $isTopLevelCall = !$this->traversalStarted; + if (!is_array($nodes)) { + $nodes = array($nodes); + } - if ($isTopLevelCall) { - // Remember that the traversal was already started for the case of - // recursive calls to traverse() - $this->traversalStarted = true; + $numberOfInitializedVisitors = $this->beforeTraversal($nodes, $context); - foreach ($this->visitors as $visitor) { - $visitor->beforeTraversal($nodes, $context); - } + // If any of the visitors requested to abort the traversal, do so, but + // clean up before + if ($numberOfInitializedVisitors < count($this->visitors)) { + $this->afterTraversal($nodes, $context, $numberOfInitializedVisitors); + + return; } // This stack contains all the nodes that should be traversed @@ -148,13 +143,60 @@ class NonRecursiveNodeTraverser implements NodeTraverserInterface } } - if ($isTopLevelCall) { - foreach ($this->visitors as $visitor) { - $visitor->afterTraversal($nodes, $context); + $this->afterTraversal($nodes, $context); + } + + /** + * Executes the {@link NodeVisitorInterface::beforeTraversal()} method of + * each visitor. + * + * @param Node[] $nodes The traversed nodes + * @param ExecutionContextInterface $context The current execution context + * + * @return integer The number of successful calls. This is lower than + * the number of visitors if any of the visitors' + * beforeTraversal() methods returned false + */ + private function beforeTraversal($nodes, ExecutionContextInterface $context) + { + $numberOfCalls = 1; + + foreach ($this->visitors as $visitor) { + if (false === $visitor->beforeTraversal($nodes, $context)) { + break; } - // Put the traverser back into its initial state - $this->traversalStarted = false; + ++$numberOfCalls; + } + + return $numberOfCalls; + } + + /** + * Executes the {@link NodeVisitorInterface::beforeTraversal()} method of + * each visitor. + * + * @param Node[] $nodes The traversed nodes + * @param ExecutionContextInterface $context The current execution context + * @param integer|null $limit Limits the number of visitors + * on which beforeTraversal() + * should be called. All visitors + * will be called by default + */ + private function afterTraversal($nodes, ExecutionContextInterface $context, $limit = null) + { + if (null === $limit) { + $limit = count($this->visitors); + } + + $numberOfCalls = 0; + + foreach ($this->visitors as $visitor) { + $visitor->afterTraversal($nodes, $context); + + if (++$numberOfCalls === $limit) { + return; + } } } diff --git a/src/Symfony/Component/Validator/NodeVisitor/AbstractVisitor.php b/src/Symfony/Component/Validator/NodeVisitor/AbstractVisitor.php index 3f15359878..c516f4a8d2 100644 --- a/src/Symfony/Component/Validator/NodeVisitor/AbstractVisitor.php +++ b/src/Symfony/Component/Validator/NodeVisitor/AbstractVisitor.php @@ -27,14 +27,14 @@ abstract class AbstractVisitor implements NodeVisitorInterface /** * {@inheritdoc} */ - public function beforeTraversal(array $nodes, ExecutionContextInterface $context) + public function beforeTraversal($nodes, ExecutionContextInterface $context) { } /** * {@inheritdoc} */ - public function afterTraversal(array $nodes, ExecutionContextInterface $context) + public function afterTraversal($nodes, ExecutionContextInterface $context) { } diff --git a/src/Symfony/Component/Validator/NodeVisitor/NodeValidationVisitor.php b/src/Symfony/Component/Validator/NodeVisitor/NodeValidationVisitor.php index e36f7880c8..cc6f578078 100644 --- a/src/Symfony/Component/Validator/NodeVisitor/NodeValidationVisitor.php +++ b/src/Symfony/Component/Validator/NodeVisitor/NodeValidationVisitor.php @@ -29,16 +29,6 @@ use Symfony\Component\Validator\NodeTraverser\NodeTraverserInterface; */ class NodeValidationVisitor extends AbstractVisitor implements GroupManagerInterface { - /** - * Stores the hashes of each validated object together with the groups - * in which that object was already validated. - * - * @var array - */ - private $validatedObjects = array(); - - private $validatedConstraints = array(); - /** * @var ConstraintValidatorFactoryInterface */ @@ -49,20 +39,37 @@ class NodeValidationVisitor extends AbstractVisitor implements GroupManagerInter */ private $nodeTraverser; + /** + * The currently validated group. + * + * @var string + */ private $currentGroup; + /** + * Creates a new visitor. + * + * @param NodeTraverserInterface $nodeTraverser The node traverser + * @param ConstraintValidatorFactoryInterface $validatorFactory The validator factory + */ public function __construct(NodeTraverserInterface $nodeTraverser, ConstraintValidatorFactoryInterface $validatorFactory) { $this->validatorFactory = $validatorFactory; $this->nodeTraverser = $nodeTraverser; } - public function afterTraversal(array $nodes, ExecutionContextInterface $context) - { - $this->validatedObjects = array(); - $this->validatedConstraints = array(); - } - + /** + * Validates a node's value against the constraints defined in the node's + * metadata. + * + * Objects and constraints that were validated before in the same context + * will be skipped. + * + * @param Node $node The current node + * @param ExecutionContextInterface $context The execution context + * + * @return Boolean Whether to traverse the successor nodes + */ public function visit(Node $node, ExecutionContextInterface $context) { if ($node instanceof CollectionNode) { @@ -84,21 +91,24 @@ class NodeValidationVisitor extends AbstractVisitor implements GroupManagerInter // simply continue traversal (if possible) foreach ($node->groups as $key => $group) { - // Remember which object was validated for which group - // Skip validation if the object was already validated for this - // group + // Even if we remove the following clause, the constraints on an + // object won't be validated again due to the measures taken in + // validateNodeForGroup(). + // The following shortcut, however, prevents validatedNodeForGroup() + // from being called at all and enhances performance a bit. if ($node instanceof ClassNode) { // Use the object hash for group sequences $groupHash = is_object($group) ? spl_object_hash($group) : $group; if ($context->isObjectValidatedForGroup($objectHash, $groupHash)) { - // Skip this group when validating properties + // Skip this group when validating the successor nodes + // (property and/or collection nodes) unset($node->groups[$key]); continue; } - //$context->markObjectAsValidatedForGroup($objectHash, $groupHash); + $context->markObjectAsValidatedForGroup($objectHash, $groupHash); } // Validate normal group @@ -108,27 +118,34 @@ class NodeValidationVisitor extends AbstractVisitor implements GroupManagerInter continue; } - // Skip the group sequence when validating properties - unset($node->groups[$key]); - // Traverse group sequence until a violation is generated $this->traverseGroupSequence($node, $group, $context); - // Optimization: If the groups only contain the group sequence, - // we can skip the traversal for the properties of the object - if (1 === count($node->groups)) { - return false; - } + // Skip the group sequence when validating successor nodes + unset($node->groups[$key]); } return true; } + /** + * {@inheritdoc} + */ public function getCurrentGroup() { return $this->currentGroup; } + /** + * Validates a node's value in each group of a group sequence. + * + * If any of the groups' constraints generates a violation, subsequent + * groups are not validated anymore. + * + * @param Node $node The validated node + * @param GroupSequence $groupSequence The group sequence + * @param ExecutionContextInterface $context The execution context + */ private function traverseGroupSequence(Node $node, GroupSequence $groupSequence, ExecutionContextInterface $context) { $violationCount = count($context->getViolations()); @@ -150,6 +167,17 @@ class NodeValidationVisitor extends AbstractVisitor implements GroupManagerInter } } + /** + * Validates a node's value against all constraints in the given group. + * + * @param Node $node The validated node + * @param string $group The group to validate + * @param ExecutionContextInterface $context The execution context + * @param string $objectHash The hash of the node's + * object (if any) + * + * @throws \Exception + */ private function validateNodeForGroup(Node $node, $group, ExecutionContextInterface $context, $objectHash) { try { @@ -176,8 +204,6 @@ class NodeValidationVisitor extends AbstractVisitor implements GroupManagerInter $context->markPropertyConstraintAsValidated($objectHash, $propertyName, $constraintHash); } - - $this->validatedConstraints[$objectHash][$constraintHash] = true; } $validator = $this->validatorFactory->getInstance($constraint); @@ -187,6 +213,7 @@ class NodeValidationVisitor extends AbstractVisitor implements GroupManagerInter $this->currentGroup = null; } catch (\Exception $e) { + // Should be put into a finally block once we switch to PHP 5.5 $this->currentGroup = null; throw $e; diff --git a/src/Symfony/Component/Validator/NodeVisitor/NodeVisitorInterface.php b/src/Symfony/Component/Validator/NodeVisitor/NodeVisitorInterface.php index fe22a1f219..ec05923f1b 100644 --- a/src/Symfony/Component/Validator/NodeVisitor/NodeVisitorInterface.php +++ b/src/Symfony/Component/Validator/NodeVisitor/NodeVisitorInterface.php @@ -29,9 +29,31 @@ use Symfony\Component\Validator\Node\Node; */ interface NodeVisitorInterface { - public function beforeTraversal(array $nodes, ExecutionContextInterface $context); + /** + * Called at the beginning of a traversal. + * + * @param Node[] $nodes A list of Node instances + * @param ExecutionContextInterface $context The execution context + * + * @return Boolean Whether to continue the traversal + */ + public function beforeTraversal($nodes, ExecutionContextInterface $context); - public function afterTraversal(array $nodes, ExecutionContextInterface $context); + /** + * Called at the end of a traversal. + * + * @param Node[] $nodes A list of Node instances + * @param ExecutionContextInterface $context The execution context + */ + public function afterTraversal($nodes, ExecutionContextInterface $context); + /** + * Called for each node during a traversal. + * + * @param Node $node The current node + * @param ExecutionContextInterface $context The execution context + * + * @return Boolean Whether to traverse the node's successor nodes + */ public function visit(Node $node, ExecutionContextInterface $context); } diff --git a/src/Symfony/Component/Validator/NodeVisitor/ObjectInitializationVisitor.php b/src/Symfony/Component/Validator/NodeVisitor/ObjectInitializationVisitor.php index 05226ac6d3..18fe19a955 100644 --- a/src/Symfony/Component/Validator/NodeVisitor/ObjectInitializationVisitor.php +++ b/src/Symfony/Component/Validator/NodeVisitor/ObjectInitializationVisitor.php @@ -12,12 +12,18 @@ namespace Symfony\Component\Validator\NodeVisitor; use Symfony\Component\Validator\Context\ExecutionContextInterface; +use Symfony\Component\Validator\Exception\InvalidArgumentException; use Symfony\Component\Validator\Node\ClassNode; use Symfony\Component\Validator\Node\Node; use Symfony\Component\Validator\ObjectInitializerInterface; /** - * @since %%NextVersion%% + * Initializes the objects of all class nodes. + * + * You have to pass at least one instance of {@link ObjectInitializerInterface} + * to the constructor of this visitor. + * + * @since 2.5 * @author Bernhard Schussek */ class ObjectInitializationVisitor extends AbstractVisitor @@ -27,30 +33,51 @@ class ObjectInitializationVisitor extends AbstractVisitor */ private $initializers; + /** + * Creates a new visitor. + * + * @param ObjectInitializerInterface[] $initializers The object initializers + * + * @throws InvalidArgumentException + */ public function __construct(array $initializers) { foreach ($initializers as $initializer) { if (!$initializer instanceof ObjectInitializerInterface) { - throw new \InvalidArgumentException('Validator initializers must implement ObjectInitializerInterface.'); + throw new InvalidArgumentException(sprintf( + 'Validator initializers must implement '. + '"Symfony\Component\Validator\ObjectInitializerInterface". '. + 'Got: "%s"', + is_object($initializer) ? get_class($initializer) : gettype($initializer) + )); } } // If no initializer is present, this visitor should not even be created if (0 === count($initializers)) { - throw new \InvalidArgumentException('Please pass at least one initializer.'); + throw new InvalidArgumentException('Please pass at least one initializer.'); } $this->initializers = $initializers; } + /** + * Calls the {@link ObjectInitializerInterface::initialize()} method for + * the object of each class node. + * + * @param Node $node The current node + * @param ExecutionContextInterface $context The execution context + * + * @return Boolean Always returns true + */ public function visit(Node $node, ExecutionContextInterface $context) { - if (!$node instanceof ClassNode) { - return; + if ($node instanceof ClassNode) { + foreach ($this->initializers as $initializer) { + $initializer->initialize($node->value); + } } - foreach ($this->initializers as $initializer) { - $initializer->initialize($node->value); - } + return true; } } diff --git a/src/Symfony/Component/Validator/Tests/NodeTraverser/NonRecursiveNodeTraverserTest.php b/src/Symfony/Component/Validator/Tests/NodeTraverser/NonRecursiveNodeTraverserTest.php new file mode 100644 index 0000000000..4dfc707124 --- /dev/null +++ b/src/Symfony/Component/Validator/Tests/NodeTraverser/NonRecursiveNodeTraverserTest.php @@ -0,0 +1,91 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Validator\Tests\NodeTraverser; + +use Symfony\Component\Validator\Mapping\GenericMetadata; +use Symfony\Component\Validator\Node\GenericNode; +use Symfony\Component\Validator\NodeTraverser\NonRecursiveNodeTraverser; +use Symfony\Component\Validator\Tests\Fixtures\FakeMetadataFactory; + +/** + * @since 2.5 + * @author Bernhard Schussek + */ +class NonRecursiveNodeTraverserTest extends \PHPUnit_Framework_TestCase +{ + /** + * @var FakeMetadataFactory + */ + private $metadataFactory; + + /** + * @var NonRecursiveNodeTraverser + */ + private $traverser; + + protected function setUp() + { + $this->metadataFactory = new FakeMetadataFactory(); + $this->traverser = new NonRecursiveNodeTraverser($this->metadataFactory); + } + + public function testVisitorsMayPreventTraversal() + { + $nodes = array(new GenericNode('value', new GenericMetadata(), '', array('Default'))); + $context = $this->getMock('Symfony\Component\Validator\Context\ExecutionContextInterface'); + + $visitor1 = $this->getMock('Symfony\Component\Validator\NodeVisitor\NodeVisitorInterface'); + $visitor2 = $this->getMock('Symfony\Component\Validator\NodeVisitor\NodeVisitorInterface'); + $visitor3 = $this->getMock('Symfony\Component\Validator\NodeVisitor\NodeVisitorInterface'); + + $visitor1->expects($this->once()) + ->method('beforeTraversal') + ->with($nodes, $context); + + // abort traversal + $visitor2->expects($this->once()) + ->method('beforeTraversal') + ->with($nodes, $context) + ->will($this->returnValue(false)); + + // never called + $visitor3->expects($this->never()) + ->method('beforeTraversal'); + + $visitor1->expects($this->never()) + ->method('visit'); + $visitor2->expects($this->never()) + ->method('visit'); + $visitor2->expects($this->never()) + ->method('visit'); + + // called in order to clean up + $visitor1->expects($this->once()) + ->method('afterTraversal') + ->with($nodes, $context); + + // abort traversal + $visitor2->expects($this->once()) + ->method('afterTraversal') + ->with($nodes, $context); + + // never called, because beforeTraversal() wasn't called either + $visitor3->expects($this->never()) + ->method('afterTraversal'); + + $this->traverser->addVisitor($visitor1); + $this->traverser->addVisitor($visitor2); + $this->traverser->addVisitor($visitor3); + + $this->traverser->traverse($nodes, $context); + } +}