minor #12201 [Validator] Fixed: The state of the XML/YAML loaders was changed even if an exception was thrown upon loading (webmozart)

This PR was merged into the 2.3 branch.

Discussion
----------

[Validator] Fixed: The state of the XML/YAML loaders was changed even if an exception was thrown upon loading

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

Commits
-------

85d464a [Validator] Fixed: The state of the XML/YAML loaders was changed even if an exception was thrown upon loading
This commit is contained in:
Fabien Potencier 2015-02-11 14:32:27 +01:00
commit 5ce1dc383c
13 changed files with 332 additions and 145 deletions

View File

@ -14,11 +14,25 @@ namespace Symfony\Component\Validator\Mapping\Loader;
use Symfony\Component\Validator\Exception\MappingException;
use Symfony\Component\Validator\Constraint;
/**
* Base loader for validation metadata.
*
* This loader supports the loading of constraints from Symfony's default
* namespace (see {@link DEFAULT_NAMESPACE}) using the short class names of
* those constraints. Constraints can also be loaded using their fully
* qualified class names. At last, namespace aliases can be defined to load
* constraints with the syntax "alias:ShortName".
*
* @author Bernhard Schussek <bschussek@gmail.com>
*/
abstract class AbstractLoader implements LoaderInterface
{
/**
* Contains all known namespaces indexed by their prefix.
*
* The namespace to load constraints from by default.
*/
const DEFAULT_NAMESPACE = '\\Symfony\\Component\\Validator\\Constraints\\';
/**
* @var array
*/
protected $namespaces;
@ -26,6 +40,13 @@ abstract class AbstractLoader implements LoaderInterface
/**
* Adds a namespace alias.
*
* The namespace alias can be used to reference constraints from specific
* namespaces in {@link newConstraint()}:
*
* $this->addNamespaceAlias('mynamespace', '\\Acme\\Package\\Constraints\\');
*
* $constraint = $this->newConstraint('mynamespace:NotNull');
*
* @param string $alias The alias
* @param string $namespace The PHP namespace
*/
@ -37,16 +58,19 @@ abstract class AbstractLoader implements LoaderInterface
/**
* Creates a new constraint instance for the given constraint name.
*
* @param string $name The constraint name. Either a constraint relative
* to the default constraint namespace, or a fully
* qualified class name
* @param mixed $options The constraint options
* @param string $name The constraint name. Either a constraint relative
* to the default constraint namespace, or a fully
* qualified class name. Alternatively, the constraint
* may be preceded by a namespace alias and a colon.
* The namespace alias must have been defined using
* {@link addNamespaceAlias()}.
* @param mixed $options The constraint options
*
* @return Constraint
*
* @throws MappingException If the namespace prefix is undefined
*/
protected function newConstraint($name, $options)
protected function newConstraint($name, $options = null)
{
if (strpos($name, '\\') !== false && class_exists($name)) {
$className = (string) $name;
@ -59,7 +83,7 @@ abstract class AbstractLoader implements LoaderInterface
$className = $this->namespaces[$prefix].$className;
} else {
$className = 'Symfony\\Component\\Validator\\Constraints\\'.$name;
$className = self::DEFAULT_NAMESPACE.$name;
}
return new $className($options);

View File

@ -18,23 +18,40 @@ use Symfony\Component\Validator\Constraints\GroupSequence;
use Symfony\Component\Validator\Constraints\GroupSequenceProvider;
use Symfony\Component\Validator\Constraint;
/**
* Loads validation metadata using a Doctrine annotation {@link Reader}.
*
* @author Bernhard Schussek <bschussek@gmail.com>
*/
class AnnotationLoader implements LoaderInterface
{
/**
* @var Reader
*/
protected $reader;
/**
* Creates a new loader.
*
* @param Reader $reader The annotation reader to use.
*/
public function __construct(Reader $reader)
{
$this->reader = $reader;
}
/**
* {@inheritdoc}
* Loads the metadata using annotations defined in the class.
*
* @param ClassMetadata $metadata The class metadata to load
*
* @return bool Whether the loader succeeded
*/
public function loadClassMetadata(ClassMetadata $metadata)
{
$reflClass = $metadata->getReflectionClass();
$className = $reflClass->name;
$loaded = false;
$success = false;
foreach ($this->reader->getClassAnnotations($reflClass) as $constraint) {
if ($constraint instanceof GroupSequence) {
@ -45,7 +62,7 @@ class AnnotationLoader implements LoaderInterface
$metadata->addConstraint($constraint);
}
$loaded = true;
$success = true;
}
foreach ($reflClass->getProperties() as $property) {
@ -55,7 +72,7 @@ class AnnotationLoader implements LoaderInterface
$metadata->addPropertyConstraint($property->name, $constraint);
}
$loaded = true;
$success = true;
}
}
}
@ -71,11 +88,11 @@ class AnnotationLoader implements LoaderInterface
}
}
$loaded = true;
$success = true;
}
}
}
return $loaded;
return $success;
}
}

View File

@ -13,26 +13,42 @@ namespace Symfony\Component\Validator\Mapping\Loader;
use Symfony\Component\Validator\Exception\MappingException;
/**
* Base loader for loading validation metadata from a file.
*
* @author Bernhard Schussek <bschussek@gmail.com>
*
* @see YamlFileLoader
* @see XmlFileLoader
*/
abstract class FileLoader extends AbstractLoader
{
/**
* The file to load.
*
* @var string
*/
protected $file;
/**
* Constructor.
* Creates a new loader.
*
* @param string $file The mapping file to load
*
* @throws MappingException if the mapping file does not exist
* @throws MappingException if the mapping file is not readable
* @throws MappingException If the file does not exist or is not readable
*/
public function __construct($file)
{
if (!is_file($file)) {
throw new MappingException(sprintf('The mapping file %s does not exist', $file));
throw new MappingException(sprintf('The mapping file "%s" does not exist', $file));
}
if (!is_readable($file)) {
throw new MappingException(sprintf('The mapping file %s is not readable', $file));
throw new MappingException(sprintf('The mapping file "%s" is not readable', $file));
}
if (!stream_is_local($this->file)) {
throw new MappingException(sprintf('The mapping file "%s" is not a local file', $file));
}
$this->file = $file;

View File

@ -12,21 +12,25 @@
namespace Symfony\Component\Validator\Mapping\Loader;
/**
* Creates mapping loaders for array of files.
*
* Abstract class, used by
* Base loader for loading validation metadata from a list of files.
*
* @author Bulat Shakirzyanov <mallluhuct@gmail.com>
* @author Bernhard Schussek <bschussek@gmail.com>
*
<<<<<<< HEAD
* @see YamlFileLoader
* @see XmlFileLoader
=======
* @see YamlFilesLoader
* @see XmlFilesLoader
>>>>>>> pull/12201
*/
abstract class FilesLoader extends LoaderChain
{
/**
* Array of mapping files.
* Creates a new loader.
*
* @param array $paths Array of file paths
* @param array $paths An array of file paths
*/
public function __construct(array $paths)
{
@ -34,15 +38,16 @@ abstract class FilesLoader extends LoaderChain
}
/**
* Array of mapping files.
* Returns an array of file loaders for the given file paths.
*
* @param array $paths Array of file paths
* @param array $paths An array of file paths
*
* @return LoaderInterface[] Array of metadata loaders
* @return LoaderInterface[] The metadata loaders
*/
protected function getFileLoaders($paths)
{
$loaders = array();
foreach ($paths as $path) {
$loaders[] = $this->getFileLoaderInstance($path);
}
@ -51,11 +56,11 @@ abstract class FilesLoader extends LoaderChain
}
/**
* Takes mapping file path.
* Creates a loader for the given file path.
*
* @param string $file
* @param string $path The file path
*
* @return LoaderInterface
* @return LoaderInterface The created loader
*/
abstract protected function getFileLoaderInstance($file);
abstract protected function getFileLoaderInstance($path);
}

View File

@ -15,25 +15,25 @@ use Symfony\Component\Validator\Exception\MappingException;
use Symfony\Component\Validator\Mapping\ClassMetadata;
/**
* Calls multiple LoaderInterface instances in a chain.
* Loads validation metadata from multiple {@link LoaderInterface} instances.
*
* This class accepts multiple instances of LoaderInterface to be passed to the
* constructor. When loadClassMetadata() is called, the same method is called
* in <em>all</em> of these loaders, regardless of whether any of them was
* successful or not.
* Pass the loaders when constructing the chain. Once
* {@link loadClassMetadata()} is called, that method will be called on all
* loaders in the chain.
*
* @author Bernhard Schussek <bschussek@gmail.com>
*/
class LoaderChain implements LoaderInterface
{
/**
* @var LoaderInterface[]
*/
protected $loaders;
/**
* Accepts a list of LoaderInterface instances.
* @param LoaderInterface[] $loaders The metadata loaders to use
*
* @param LoaderInterface[] $loaders An array of LoaderInterface instances
*
* @throws MappingException If any of the loaders does not implement LoaderInterface
* @throws MappingException If any of the loaders has an invalid type
*/
public function __construct(array $loaders)
{
@ -47,7 +47,12 @@ class LoaderChain implements LoaderInterface
}
/**
* {@inheritdoc}
* Calls {@link LoaderInterface::loadClassMetadata()} on all loaders in
* the chain.
*
* @param ClassMetadata $metadata The metadata to load
*
* @return bool Whether the loader succeeded
*/
public function loadClassMetadata(ClassMetadata $metadata)
{

View File

@ -13,14 +13,19 @@ namespace Symfony\Component\Validator\Mapping\Loader;
use Symfony\Component\Validator\Mapping\ClassMetadata;
/**
* Loads validation metadata into {@link ClassMetadata} instances.
*
* @author Bernhard Schussek <bschussek@gmail.com>
*/
interface LoaderInterface
{
/**
* Load a Class Metadata.
* Loads validation metadata into a {@link ClassMetadata} instance.
*
* @param ClassMetadata $metadata A metadata
* @param ClassMetadata $metadata The metadata to load
*
* @return bool
* @return bool Whether the loader succeeded
*/
public function loadClassMetadata(ClassMetadata $metadata);
}

View File

@ -14,17 +14,38 @@ namespace Symfony\Component\Validator\Mapping\Loader;
use Symfony\Component\Validator\Exception\MappingException;
use Symfony\Component\Validator\Mapping\ClassMetadata;
/**
* Loads validation metadata by calling a static method on the loaded class.
*
* @author Bernhard Schussek <bschussek@gmail.com>
*/
class StaticMethodLoader implements LoaderInterface
{
/**
* The name of the method to call.
*
* @var string
*/
protected $methodName;
/**
* Creates a new loader.
*
* @param string $methodName The name of the static method to call
*/
public function __construct($methodName = 'loadValidatorMetadata')
{
$this->methodName = $methodName;
}
/**
* {@inheritdoc}
* Loads validation metadata by calling a static method in the class.
*
* The name of the static method is passed to {@link __construct()}.
*
* @param ClassMetadata $metadata The metadata to load
*
* @return bool Whether the loader succeeded
*/
public function loadClassMetadata(ClassMetadata $metadata)
{

View File

@ -15,24 +15,36 @@ use Symfony\Component\Validator\Exception\MappingException;
use Symfony\Component\Validator\Mapping\ClassMetadata;
use Symfony\Component\Config\Util\XmlUtils;
/**
* Loads validation metadata from an XML file.
*
* @author Bernhard Schussek <bschussek@gmail.com>
*/
class XmlFileLoader extends FileLoader
{
/**
* An array of SimpleXMLElement instances.
* The XML nodes of the mapping file.
*
* @var \SimpleXMLElement[]
*/
protected $classes = null;
/**
* {@inheritdoc}
* Loads validation metadata using the metadata defined in the XML file.
*
* @param ClassMetadata $metadata The metadata to load
*
* @return bool Whether the loader succeeded
*/
public function loadClassMetadata(ClassMetadata $metadata)
{
if (null === $this->classes) {
$this->classes = array();
// This method may throw an exception. Do not modify the class'
// state before it completes
$xml = $this->parseFile($this->file);
$this->classes = array();
foreach ($xml->namespace as $namespace) {
$this->addNamespaceAlias((string) $namespace['prefix'], trim((string) $namespace));
}
@ -43,33 +55,9 @@ class XmlFileLoader extends FileLoader
}
if (isset($this->classes[$metadata->getClassName()])) {
$xml = $this->classes[$metadata->getClassName()];
$classDescription = $this->classes[$metadata->getClassName()];
foreach ($xml->{'group-sequence-provider'} as $provider) {
$metadata->setGroupSequenceProvider(true);
}
foreach ($xml->{'group-sequence'} as $groupSequence) {
if (count($groupSequence->value) > 0) {
$metadata->setGroupSequence($this->parseValues($groupSequence[0]->value));
}
}
foreach ($this->parseConstraints($xml->constraint) as $constraint) {
$metadata->addConstraint($constraint);
}
foreach ($xml->property as $property) {
foreach ($this->parseConstraints($property->constraint) as $constraint) {
$metadata->addPropertyConstraint((string) $property['name'], $constraint);
}
}
foreach ($xml->getter as $getter) {
foreach ($this->parseConstraints($getter->constraint) as $constraint) {
$metadata->addGetterConstraint((string) $getter['property'], $constraint);
}
}
$this->loadClassMetadataFromXml($metadata, $classDescription);
return true;
}
@ -179,22 +167,57 @@ class XmlFileLoader extends FileLoader
}
/**
* Parse a XML File.
* Loads the XML class descriptions from the given file.
*
* @param string $file Path of file
* @param string $path The path of the XML file
*
* @return \SimpleXMLElement
* @return \SimpleXMLElement The class descriptions
*
* @throws MappingException
* @throws MappingException If the file could not be loaded
*/
protected function parseFile($file)
protected function parseFile($path)
{
try {
$dom = XmlUtils::loadFile($file, __DIR__.'/schema/dic/constraint-mapping/constraint-mapping-1.0.xsd');
$dom = XmlUtils::loadFile($path, __DIR__.'/schema/dic/constraint-mapping/constraint-mapping-1.0.xsd');
} catch (\Exception $e) {
throw new MappingException($e->getMessage(), $e->getCode(), $e);
}
return simplexml_import_dom($dom);
}
/**
* Loads the validation metadata from the given XML class description.
*
* @param ClassMetadata $metadata The metadata to load
* @param array $classDescription The XML class description
*/
private function loadClassMetadataFromXml(ClassMetadata $metadata, $classDescription)
{
foreach ($classDescription->{'group-sequence-provider'} as $_) {
$metadata->setGroupSequenceProvider(true);
}
foreach ($classDescription->{'group-sequence'} as $groupSequence) {
if (count($groupSequence->value) > 0) {
$metadata->setGroupSequence($this->parseValues($groupSequence[0]->value));
}
}
foreach ($this->parseConstraints($classDescription->constraint) as $constraint) {
$metadata->addConstraint($constraint);
}
foreach ($classDescription->property as $property) {
foreach ($this->parseConstraints($property->constraint) as $constraint) {
$metadata->addPropertyConstraint((string) $property['name'], $constraint);
}
}
foreach ($classDescription->getter as $getter) {
foreach ($this->parseConstraints($getter->constraint) as $constraint) {
$metadata->addGetterConstraint((string) $getter['property'], $constraint);
}
}
}
}

View File

@ -12,9 +12,10 @@
namespace Symfony\Component\Validator\Mapping\Loader;
/**
* Loads multiple xml mapping files.
* Loads validation metadata from a list of XML files.
*
* @author Bulat Shakirzyanov <mallluhuct@gmail.com>
* @author Bernhard Schussek <bschussek@gmail.com>
*
* @see FilesLoader
*/

View File

@ -14,10 +14,13 @@ namespace Symfony\Component\Validator\Mapping\Loader;
use Symfony\Component\Validator\Mapping\ClassMetadata;
use Symfony\Component\Yaml\Parser as YamlParser;
/**
* Loads validation metadata from a YAML file.
*
* @author Bernhard Schussek <bschussek@gmail.com>
*/
class YamlFileLoader extends FileLoader
{
private $yamlParser;
/**
* An array of YAML class descriptions.
*
@ -26,34 +29,33 @@ class YamlFileLoader extends FileLoader
protected $classes = null;
/**
* {@inheritdoc}
* Caches the used YAML parser.
*
* @var YamlParser
*/
private $yamlParser;
/**
* Loads validation metadata using the metadata defined in the YAML file.
*
* @param ClassMetadata $metadata The metadata to load
*
* @return bool Whether the loader succeeded
*/
public function loadClassMetadata(ClassMetadata $metadata)
{
if (null === $this->classes) {
if (!stream_is_local($this->file)) {
throw new \InvalidArgumentException(sprintf('This is not a local file "%s".', $this->file));
}
if (!file_exists($this->file)) {
throw new \InvalidArgumentException(sprintf('File "%s" not found.', $this->file));
}
if (null === $this->yamlParser) {
$this->yamlParser = new YamlParser();
}
$this->classes = $this->yamlParser->parse(file_get_contents($this->file));
// empty file
if (null === $this->classes) {
// This method may throw an exception. Do not modify the class'
// state before it completes
if (false === ($classes = $this->parseFile($this->file))) {
return false;
}
// not an array
if (!is_array($this->classes)) {
throw new \InvalidArgumentException(sprintf('The file "%s" must contain a YAML array.', $this->file));
}
$this->classes = $classes;
if (isset($this->classes['namespaces'])) {
foreach ($this->classes['namespaces'] as $alias => $namespace) {
@ -64,44 +66,10 @@ class YamlFileLoader extends FileLoader
}
}
// TODO validation
if (isset($this->classes[$metadata->getClassName()])) {
$yaml = $this->classes[$metadata->getClassName()];
$classDescription = $this->classes[$metadata->getClassName()];
if (isset($yaml['group_sequence_provider'])) {
$metadata->setGroupSequenceProvider((bool) $yaml['group_sequence_provider']);
}
if (isset($yaml['group_sequence'])) {
$metadata->setGroupSequence($yaml['group_sequence']);
}
if (isset($yaml['constraints']) && is_array($yaml['constraints'])) {
foreach ($this->parseNodes($yaml['constraints']) as $constraint) {
$metadata->addConstraint($constraint);
}
}
if (isset($yaml['properties']) && is_array($yaml['properties'])) {
foreach ($yaml['properties'] as $property => $constraints) {
if (null !== $constraints) {
foreach ($this->parseNodes($constraints) as $constraint) {
$metadata->addPropertyConstraint($property, $constraint);
}
}
}
}
if (isset($yaml['getters']) && is_array($yaml['getters'])) {
foreach ($yaml['getters'] as $getter => $constraints) {
if (null !== $constraints) {
foreach ($this->parseNodes($constraints) as $constraint) {
$metadata->addGetterConstraint($getter, $constraint);
}
}
}
}
$this->loadClassMetadataFromYaml($metadata, $classDescription);
return true;
}
@ -140,4 +108,76 @@ class YamlFileLoader extends FileLoader
return $values;
}
/**
* Loads the YAML class descriptions from the given file.
*
* @param string $path The path of the YAML file
*
* @return array|null The class descriptions or null, if the file was empty
*
* @throws \InvalidArgumentException If the file could not be loaded or did
* not contain a YAML array
*/
private function parseFile($path)
{
$classes = $this->yamlParser->parse(file_get_contents($path));
// empty file
if (null === $classes) {
return;
}
// not an array
if (!is_array($classes)) {
throw new \InvalidArgumentException(sprintf('The file "%s" must contain a YAML array.', $this->file));
}
return $classes;
}
/**
* Loads the validation metadata from the given YAML class description.
*
* @param ClassMetadata $metadata The metadata to load
* @param array $classDescription The YAML class description
*/
private function loadClassMetadataFromYaml(ClassMetadata $metadata, array $classDescription)
{
if (isset($classDescription['group_sequence_provider'])) {
$metadata->setGroupSequenceProvider(
(bool) $classDescription['group_sequence_provider']
);
}
if (isset($classDescription['group_sequence'])) {
$metadata->setGroupSequence($classDescription['group_sequence']);
}
if (isset($classDescription['constraints']) && is_array($classDescription['constraints'])) {
foreach ($this->parseNodes($classDescription['constraints']) as $constraint) {
$metadata->addConstraint($constraint);
}
}
if (isset($classDescription['properties']) && is_array($classDescription['properties'])) {
foreach ($classDescription['properties'] as $property => $constraints) {
if (null !== $constraints) {
foreach ($this->parseNodes($constraints) as $constraint) {
$metadata->addPropertyConstraint($property, $constraint);
}
}
}
}
if (isset($classDescription['getters']) && is_array($classDescription['getters'])) {
foreach ($classDescription['getters'] as $getter => $constraints) {
if (null !== $constraints) {
foreach ($this->parseNodes($constraints) as $constraint) {
$metadata->addGetterConstraint($getter, $constraint);
}
}
}
}
}
}

View File

@ -12,9 +12,10 @@
namespace Symfony\Component\Validator\Mapping\Loader;
/**
* Loads multiple yaml mapping files.
* Loads validation metadata from a list of YAML files.
*
* @author Bulat Shakirzyanov <mallluhuct@gmail.com>
* @author Bernhard Schussek <bschussek@gmail.com>
*
* @see FilesLoader
*/

View File

@ -17,6 +17,7 @@ use Symfony\Component\Validator\Constraints\NotNull;
use Symfony\Component\Validator\Constraints\Range;
use Symfony\Component\Validator\Constraints\Choice;
use Symfony\Component\Validator\Constraints\Regex;
use Symfony\Component\Validator\Exception\MappingException;
use Symfony\Component\Validator\Mapping\ClassMetadata;
use Symfony\Component\Validator\Mapping\Loader\XmlFileLoader;
use Symfony\Component\Validator\Tests\Fixtures\ConstraintA;
@ -98,15 +99,28 @@ class XmlFileLoaderTest extends \PHPUnit_Framework_TestCase
$this->assertEquals($expected, $metadata);
}
/**
* @expectedException \Symfony\Component\Validator\Exception\MappingException
* @expectedExceptionMessage Document types are not allowed.
*/
public function testDocTypeIsNotAllowed()
public function testThrowExceptionIfDocTypeIsSet()
{
$loader = new XmlFileLoader(__DIR__.'/withdoctype.xml');
$metadata = new ClassMetadata('Symfony\Component\Validator\Tests\Fixtures\Entity');
$this->setExpectedException('\Symfony\Component\Validator\Exception\MappingException');
$loader->loadClassMetadata($metadata);
}
/**
* @see https://github.com/symfony/symfony/pull/12158
*/
public function testDoNotModifyStateIfExceptionIsThrown()
{
$loader = new XmlFileLoader(__DIR__.'/withdoctype.xml');
$metadata = new ClassMetadata('Symfony\Component\Validator\Tests\Fixtures\Entity');
try {
$loader->loadClassMetadata($metadata);
} catch (MappingException $e) {
$this->setExpectedException('\Symfony\Component\Validator\Exception\MappingException');
$loader->loadClassMetadata($metadata);
}
}
}

View File

@ -31,16 +31,31 @@ class YamlFileLoaderTest extends \PHPUnit_Framework_TestCase
$this->assertFalse($loader->loadClassMetadata($metadata));
}
/**
* @expectedException \InvalidArgumentException
*/
public function testLoadClassMetadataThrowsExceptionIfNotAnArray()
{
$loader = new YamlFileLoader(__DIR__.'/nonvalid-mapping.yml');
$metadata = new ClassMetadata('Symfony\Component\Validator\Tests\Fixtures\Entity');
$this->setExpectedException('\InvalidArgumentException');
$loader->loadClassMetadata($metadata);
}
/**
* @see https://github.com/symfony/symfony/pull/12158
*/
public function testDoNotModifyStateIfExceptionIsThrown()
{
$loader = new YamlFileLoader(__DIR__.'/nonvalid-mapping.yml');
$metadata = new ClassMetadata('Symfony\Component\Validator\Tests\Fixtures\Entity');
try {
$loader->loadClassMetadata($metadata);
} catch (\InvalidArgumentException $e) {
// Call again. Again an exception should be thrown
$this->setExpectedException('\InvalidArgumentException');
$loader->loadClassMetadata($metadata);
}
}
public function testLoadClassMetadataReturnsTrueIfSuccessful()
{
$loader = new YamlFileLoader(__DIR__.'/constraint-mapping.yml');