[Form] Some code cleanup

src/Symfony/Component/Form/Extension/Core/Type/FormType.php

@@ -137,11 +137,7 @@ class FormType extends AbstractType
     {
         // Derive "data_class" option from passed "data" object
         $dataClass = function (Options $options) {
-            if (is_object($options['data'])) {
+            return is_object($options['data']) ? get_class($options['data']) : null;
-                return get_class($options['data']);
-            }
-
-            return null;
         };
 
         // Derive "empty_data" closure from "data_class" option
@@ -150,20 +146,12 @@ class FormType extends AbstractType
 
             if (null !== $class) {
                 return function (FormInterface $form) use ($class) {
-                    if ($form->isEmpty() && !$form->isRequired()) {
+                    return $form->isEmpty() && !$form->isRequired() ? null : new $class();
-                        return null;
-                    }
-
-                    return new $class();
                 };
             }
 
             return function (FormInterface $form) {
-                if (count($form) > 0) {
+                return count($form) > 0 ? array() : '';
-                    return array();
-                }
-
-                return '';
             };
         };
 

src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php

@@ -34,11 +34,7 @@ class FormValidator extends ConstraintValidator
      */
     public function __construct(ServerParams $params = null)
     {
-        if (null === $params) {
+        $this->serverParams = $params ?: new ServerParams();
-            $params = new ServerParams();
-        }
-
-        $this->serverParams = $params;
     }
 
     /**
@@ -107,34 +103,17 @@ class FormValidator extends ConstraintValidator
         $length = $this->serverParams->getContentLength();
 
         if ($form->isRoot() && null !== $length) {
-            $max = strtoupper(trim($this->serverParams->getPostMaxSize()));
+            $max = $this->serverParams->getPostMaxSize();
 
-            if ('' !== $max) {
+            if (null !== $max && $length > $max) {
-                $maxLength = (int) $max;
-
-                switch (substr($max, -1)) {
-                    // The 'G' modifier is available since PHP 5.1.0
-                    case 'G':
-                        $maxLength *= pow(1024, 3);
-                        break;
-                    case 'M':
-                        $maxLength *= pow(1024, 2);
-                        break;
-                    case 'K':
-                        $maxLength *= 1024;
-                        break;
-                }
-
-                if ($length > $maxLength) {
                 $this->context->addViolation(
                     $config->getOption('post_max_size_message'),
-                        array('{{ max }}' => $max),
+                    array('{{ max }}' => $this->serverParams->getNormalizedIniPostMaxSize()),
                     $length
                 );
             }
         }
     }
-    }
 
     /**
      * Returns whether the data of a form may be walked.
@@ -159,14 +138,10 @@ class FormValidator extends ConstraintValidator
 
         // Non-root forms are validated if validation cascading
         // is enabled in all ancestor forms
-        $parent = $form->getParent();
+        while (null !== ($form = $form->getParent())) {
-
+            if (!$form->getConfig()->getOption('cascade_validation')) {
-        while (null !== $parent) {
-            if (!$parent->getConfig()->getOption('cascade_validation')) {
                 return false;
             }
-
-            $parent = $parent->getParent();
         }
 
         return true;
@@ -181,22 +156,20 @@ class FormValidator extends ConstraintValidator
      */
     private function getValidationGroups(FormInterface $form)
     {
-        $groups = null;
+        do {
-
-        while (null !== $form && null === $groups) {
             $groups = $form->getConfig()->getOption('validation_groups');
 
+            if (null !== $groups) {
                 if (is_callable($groups)) {
-                $groups = (array) call_user_func($groups, $form);
+                    $groups = call_user_func($groups, $form);
-            }
-
-            $form = $form->getParent();
-        }
-
-        if (null === $groups) {
-            $groups = array(Constraint::DEFAULT_GROUP);
                 }
 
                 return (array) $groups;
             }
+
+            $form = $form->getParent();
+        } while (null !== $form);
+
+        return array(Constraint::DEFAULT_GROUP);
+    }
 }

src/Symfony/Component/Form/Extension/Validator/EventListener/ValidationListener.php

@@ -12,18 +12,11 @@
 namespace Symfony\Component\Form\Extension\Validator\EventListener;
 
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
-use Symfony\Component\Validator\ConstraintViolation;
-use Symfony\Component\Form\Extension\Validator\Constraints\Form;
 use Symfony\Component\Form\Extension\Validator\ViolationMapper\ViolationMapperInterface;
-use Symfony\Component\Form\FormInterface;
+use Symfony\Component\Validator\ValidatorInterface;
-use Symfony\Component\Form\FormError;
 use Symfony\Component\Form\FormEvents;
 use Symfony\Component\Form\Event\DataEvent;
-use Symfony\Component\Form\Exception\FormException;
+use Symfony\Component\Form\Extension\Validator\Constraints\Form;
-use Symfony\Component\Form\Util\PropertyPath;
-use Symfony\Component\Validator\Constraint;
-use Symfony\Component\Validator\ValidatorInterface;
-use Symfony\Component\Validator\ExecutionContext;
 
 /**
  * @author Bernhard Schussek <bschussek@gmail.com>

src/Symfony/Component/Form/Extension/Validator/Util/ServerParams.php

@@ -17,13 +17,40 @@ namespace Symfony\Component\Form\Extension\Validator\Util;
 class ServerParams
 {
     /**
-     * Returns the "post_max_size" ini setting.
+     * Returns maximum post size in bytes.
      *
-     * @return string The value of the ini setting.
+     * @return null|integer The maximum post size in bytes
      */
     public function getPostMaxSize()
     {
-        return ini_get('post_max_size');
+        $iniMax = $this->getNormalizedIniPostMaxSize();
+
+        if ('' === $iniMax) {
+            return null;
+        }
+
+        $max = (int) $iniMax;
+
+        switch (substr($iniMax, -1)) {
+            case 'G':
+                $max *= 1024;
+            case 'M':
+                $max *= 1024;
+            case 'K':
+                $max *= 1024;
+        }
+
+        return $max;
+    }
+
+    /**
+     * Returns the normalized "post_max_size" ini setting.
+     *
+     * @return string
+     */
+    public function getNormalizedIniPostMaxSize()
+    {
+        return strtoupper(trim(ini_get('post_max_size')));
     }
 
     /**

src/Symfony/Component/Form/Tests/Extension/Validator/Constraints/FormValidatorTest.php

@@ -55,7 +55,10 @@ class FormValidatorTest extends \PHPUnit_Framework_TestCase
 
         $this->dispatcher = $this->getMock('Symfony\Component\EventDispatcher\EventDispatcherInterface');
         $this->factory = $this->getMock('Symfony\Component\Form\FormFactoryInterface');
-        $this->serverParams = $this->getMock('Symfony\Component\Form\Extension\Validator\Util\ServerParams');
+        $this->serverParams = $this->getMock(
+            'Symfony\Component\Form\Extension\Validator\Util\ServerParams',
+            array('getNormalizedIniPostMaxSize', 'getContentLength')
+        );
         $this->validator = new FormValidator($this->serverParams);
     }
 
@@ -408,14 +411,18 @@ class FormValidatorTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals('Extra!', $context->getViolations()->get(0)->getMessage());
     }
 
-    public function testViolationIfPostMaxSizeExceeded_GigaUpper()
+
+    /**
+     * @dataProvider getPostMaxSizeFixtures
+     */
+    public function testPostMaxSizeViolation($contentLength, $iniMax, $nbViolation, $msg)
     {
-        $this->serverParams->expects($this->any())
+        $this->serverParams->expects($this->once())
             ->method('getContentLength')
-            ->will($this->returnValue(pow(1024, 3) + 1));
+            ->will($this->returnValue($contentLength));
         $this->serverParams->expects($this->any())
-            ->method('getPostMaxSize')
+            ->method('getNormalizedIniPostMaxSize')
-            ->will($this->returnValue('1G'));
+            ->will($this->returnValue($iniMax));
 
         $context = $this->getExecutionContext();
         $options = array('post_max_size_message' => 'Max {{ max }}!');
@@ -424,132 +431,33 @@ class FormValidatorTest extends \PHPUnit_Framework_TestCase
         $this->validator->initialize($context);
         $this->validator->validate($form, new Form());
 
-        $this->assertCount(1, $context->getViolations());
+        $this->assertCount($nbViolation, $context->getViolations());
-        $this->assertEquals('Max 1G!', $context->getViolations()->get(0)->getMessage());
+        if (null !== $msg) {
+            $this->assertEquals($msg, $context->getViolations()->get(0)->getMessage());
+        }
     }
 
-    public function testViolationIfPostMaxSizeExceeded_GigaLower()
+    public function getPostMaxSizeFixtures()
     {
-        $this->serverParams->expects($this->any())
+        return array(
-            ->method('getContentLength')
+            array(pow(1024, 3) + 1, '1G', 1, 'Max 1G!'),
-            ->will($this->returnValue(pow(1024, 3) + 1));
+            array(pow(1024, 3), '1G', 0, null),
-        $this->serverParams->expects($this->any())
+            array(pow(1024, 2) + 1, '1M', 1, 'Max 1M!'),
-            ->method('getPostMaxSize')
+            array(pow(1024, 2), '1M', 0, null),
-            ->will($this->returnValue('1g'));
+            array(1024 + 1, '1K', 1, 'Max 1K!'),
-
+            array(1024, '1K', 0, null),
-        $context = $this->getExecutionContext();
+            array(null, '1K', 0, null),
-        $options = array('post_max_size_message' => 'Max {{ max }}!');
+            array(1024, '', 0, null),
-        $form = $this->getBuilder('name', null, $options)->getForm();
+        );
-
-        $this->validator->initialize($context);
-        $this->validator->validate($form, new Form());
-
-        $this->assertCount(1, $context->getViolations());
-        $this->assertEquals('Max 1G!', $context->getViolations()->get(0)->getMessage());
-    }
-
-    public function testNoViolationIfPostMaxSizeNotExceeded_Giga()
-    {
-        $this->serverParams->expects($this->any())
-            ->method('getContentLength')
-            ->will($this->returnValue(pow(1024, 3)));
-        $this->serverParams->expects($this->any())
-            ->method('getPostMaxSize')
-            ->will($this->returnValue('1G'));
-
-        $context = $this->getExecutionContext();
-        $form = $this->getForm();
-
-        $this->validator->initialize($context);
-        $this->validator->validate($form, new Form());
-
-        $this->assertCount(0, $context->getViolations());
-    }
-
-    public function testViolationIfPostMaxSizeExceeded_Mega()
-    {
-        $this->serverParams->expects($this->any())
-            ->method('getContentLength')
-            ->will($this->returnValue(pow(1024, 2) + 1));
-        $this->serverParams->expects($this->any())
-            ->method('getPostMaxSize')
-            ->will($this->returnValue('1M'));
-
-        $context = $this->getExecutionContext();
-        $options = array('post_max_size_message' => 'Max {{ max }}!');
-        $form = $this->getBuilder('name', null, $options)->getForm();
-
-        $this->validator->initialize($context);
-        $this->validator->validate($form, new Form());
-
-        $this->assertCount(1, $context->getViolations());
-        $this->assertEquals('Max 1M!', $context->getViolations()->get(0)->getMessage());
-    }
-
-    public function testNoViolationIfPostMaxSizeNotExceeded_Mega()
-    {
-        $this->serverParams->expects($this->any())
-            ->method('getContentLength')
-            ->will($this->returnValue(pow(1024, 2)));
-        $this->serverParams->expects($this->any())
-            ->method('getPostMaxSize')
-            ->will($this->returnValue('1M'));
-
-        $context = $this->getExecutionContext();
-        $form = $this->getForm();
-
-        $this->validator->initialize($context);
-        $this->validator->validate($form, new Form());
-
-        $this->assertCount(0, $context->getViolations());
-    }
-
-    public function testViolationIfPostMaxSizeExceeded_Kilo()
-    {
-        $this->serverParams->expects($this->any())
-            ->method('getContentLength')
-            ->will($this->returnValue(1025));
-        $this->serverParams->expects($this->any())
-            ->method('getPostMaxSize')
-            ->will($this->returnValue('1K'));
-
-        $context = $this->getExecutionContext();
-        $options = array('post_max_size_message' => 'Max {{ max }}!');
-        $form = $this->getBuilder('name', null, $options)->getForm();
-
-        $this->validator->initialize($context);
-        $this->validator->validate($form, new Form());
-
-        $this->assertCount(1, $context->getViolations());
-        $this->assertEquals('Max 1K!', $context->getViolations()->get(0)->getMessage());
-    }
-
-    public function testNoViolationIfPostMaxSizeNotExceeded_Kilo()
-    {
-        $this->serverParams->expects($this->any())
-            ->method('getContentLength')
-            ->will($this->returnValue(1024));
-        $this->serverParams->expects($this->any())
-            ->method('getPostMaxSize')
-            ->will($this->returnValue('1K'));
-
-        $context = $this->getExecutionContext();
-        $form = $this->getForm();
-
-        $this->validator->initialize($context);
-        $this->validator->validate($form, new Form());
-
-        $this->assertCount(0, $context->getViolations());
     }
 
     public function testNoViolationIfNotRoot()
     {
-        $this->serverParams->expects($this->any())
+        $this->serverParams->expects($this->once())
             ->method('getContentLength')
             ->will($this->returnValue(1025));
-        $this->serverParams->expects($this->any())
+        $this->serverParams->expects($this->never())
-            ->method('getPostMaxSize')
+            ->method('getNormalizedIniPostMaxSize');
-            ->will($this->returnValue('1K'));
 
         $context = $this->getExecutionContext();
         $parent = $this->getForm();
@@ -562,80 +470,6 @@ class FormValidatorTest extends \PHPUnit_Framework_TestCase
         $this->assertCount(0, $context->getViolations());
     }
 
-    public function testNoViolationIfContentLengthNull()
-    {
-        $this->serverParams->expects($this->any())
-            ->method('getContentLength')
-            ->will($this->returnValue(null));
-        $this->serverParams->expects($this->any())
-            ->method('getPostMaxSize')
-            ->will($this->returnValue('1K'));
-
-        $context = $this->getExecutionContext();
-        $form = $this->getForm();
-
-        $this->validator->initialize($context);
-        $this->validator->validate($form, new Form());
-
-        $this->assertCount(0, $context->getViolations());
-    }
-
-    public function testTrimPostMaxSize()
-    {
-        $this->serverParams->expects($this->any())
-            ->method('getContentLength')
-            ->will($this->returnValue(1025));
-        $this->serverParams->expects($this->any())
-            ->method('getPostMaxSize')
-            ->will($this->returnValue('   1K    '));
-
-        $context = $this->getExecutionContext();
-        $options = array('post_max_size_message' => 'Max {{ max }}!');
-        $form = $this->getBuilder('name', null, $options)->getForm();
-
-        $this->validator->initialize($context);
-        $this->validator->validate($form, new Form());
-
-        $this->assertCount(1, $context->getViolations());
-        $this->assertEquals('Max 1K!', $context->getViolations()->get(0)->getMessage());
-    }
-
-    public function testNoViolationIfPostMaxSizeEmpty()
-    {
-        $this->serverParams->expects($this->any())
-            ->method('getContentLength')
-            ->will($this->returnValue(1025));
-        $this->serverParams->expects($this->any())
-            ->method('getPostMaxSize')
-            ->will($this->returnValue('     '));
-
-        $context = $this->getExecutionContext();
-        $form = $this->getForm();
-
-        $this->validator->initialize($context);
-        $this->validator->validate($form, new Form());
-
-        $this->assertCount(0, $context->getViolations());
-    }
-
-    public function testNoViolationIfPostMaxSizeNull()
-    {
-        $this->serverParams->expects($this->any())
-            ->method('getContentLength')
-            ->will($this->returnValue(1025));
-        $this->serverParams->expects($this->any())
-            ->method('getPostMaxSize')
-            ->will($this->returnValue(null));
-
-        $context = $this->getExecutionContext();
-        $form = $this->getForm();
-
-        $this->validator->initialize($context);
-        $this->validator->validate($form, new Form());
-
-        $this->assertCount(0, $context->getViolations());
-    }
-
     /**
      * Access has to be public, as this method is called via callback array
      * in {@link testValidateFormDataCanHandleCallbackValidationGroups()}

src/Symfony/Component/Form/Tests/Extension/Validator/Type/FormTypeValidatorExtensionTest.php

@@ -22,7 +22,7 @@ class FormTypeValidatorExtensionTest extends TypeTestCase
         $this->assertNull($form->getConfig()->getOption('validation_groups'));
     }
 
-    public function testValidationGroupsCanBeSetToString()
+    public function testValidationGroupsTransformedToArray()
     {
         $form = $this->factory->create('form', null, array(
             'validation_groups' => 'group',

src/Symfony/Component/Form/Util/PropertyPath.php

@@ -355,7 +355,7 @@ class PropertyPath implements \IteratorAggregate, PropertyPathInterface
      *
      * @param object|array $objectOrArray The object or array to read from.
      * @param string       $property      The property to read.
-     * @param  integer      $isIndex         Whether to interpret the property as index.
+     * @param Boolean      $isIndex       Whether to interpret the property as index.
      *
      * @return mixed The value of the read property
      *
@@ -428,7 +428,7 @@ class PropertyPath implements \IteratorAggregate, PropertyPathInterface
      * @param object|array $objectOrArray The object or array to write to.
      * @param string       $property      The property to write.
      * @param string       $singular      The singular form of the property name or null.
-     * @param  integer      $isIndex         Whether to interpret the property as index.
+     * @param Boolean      $isIndex       Whether to interpret the property as index.
      * @param mixed        $value         The value to write.
      *
      * @throws InvalidPropertyException      If the property does not exist.
@@ -445,78 +445,11 @@ class PropertyPath implements \IteratorAggregate, PropertyPathInterface
             $objectOrArray[$property] = $value;
         } elseif (is_object($objectOrArray)) {
             $reflClass = new ReflectionClass($objectOrArray);
-            $setter = 'set'.$this->camelize($property);
-            $addMethod = null;
-            $removeMethod = null;
-            $plural = null;
 
-            // Check if the parent has matching methods to add/remove items
             if (is_array($value) || $value instanceof Traversable) {
-                if (null !== $singular) {
+                $methods = $this->findAdderAndRemover($reflClass, $singular);
-                    $addMethod = 'add' . ucfirst($singular);
+                if (null !== $methods) {
-                    $removeMethod = 'remove' . ucfirst($singular);
+                    // At this point the add and remove methods have been found
-
-                    if (!$this->isAccessible($reflClass, $addMethod, 1)) {
-                        throw new InvalidPropertyException(sprintf(
-                            'The public method "%s" with exactly one required parameter was not found on class %s',
-                            $addMethod,
-                            $reflClass->getName()
-                        ));
-                    }
-
-                    if (!$this->isAccessible($reflClass, $removeMethod, 1)) {
-                        throw new InvalidPropertyException(sprintf(
-                            'The public method "%s" with exactly one required parameter was not found on class %s',
-                            $removeMethod,
-                            $reflClass->getName()
-                        ));
-                    }
-                } else {
-                    // The plural form is the last element of the property path
-                    $plural = ucfirst($this->elements[$this->length - 1]);
-
-                    // Any of the two methods is required, but not yet known
-                    $singulars = (array) FormUtil::singularify($plural);
-
-                    foreach ($singulars as $singular) {
-                        $addMethodName = 'add' . $singular;
-                        $removeMethodName = 'remove' . $singular;
-
-                        if ($this->isAccessible($reflClass, $addMethodName, 1)) {
-                            $addMethod = $addMethodName;
-                        }
-
-                        if ($this->isAccessible($reflClass, $removeMethodName, 1)) {
-                            $removeMethod = $removeMethodName;
-                        }
-
-                        if ($addMethod && !$removeMethod) {
-                            throw new InvalidPropertyException(sprintf(
-                                'Found the public method "%s", but did not find a public "%s" on class %s',
-                                $addMethodName,
-                                $removeMethodName,
-                                $reflClass->getName()
-                            ));
-                        }
-
-                        if ($removeMethod && !$addMethod) {
-                            throw new InvalidPropertyException(sprintf(
-                                'Found the public method "%s", but did not find a public "%s" on class %s',
-                                $removeMethodName,
-                                $addMethodName,
-                                $reflClass->getName()
-                            ));
-                        }
-
-                        if ($addMethod && $removeMethod) {
-                            break;
-                        }
-                    }
-                }
-            }
-
-            // Collection with matching adder/remover in $objectOrArray
-            if ($addMethod && $removeMethod) {
                     $itemsToAdd = is_object($value) ? clone $value : $value;
                     $itemToRemove = array();
                     $previousValue = $this->readProperty($objectOrArray, $property, $isIndex);
@@ -539,13 +472,19 @@ class PropertyPath implements \IteratorAggregate, PropertyPathInterface
                     }
 
                     foreach ($itemToRemove as $item) {
-                    $objectOrArray->$removeMethod($item);
+                        call_user_func(array($objectOrArray, $methods[1]), $item);
                     }
 
                     foreach ($itemsToAdd as $item) {
-                    $objectOrArray->$addMethod($item);
+                        call_user_func(array($objectOrArray, $methods[0]), $item);
                     }
-            } elseif ($reflClass->hasMethod($setter)) {
+
+                    return;
+                }
+            }
+
+            $setter = 'set'.$this->camelize($property);
+            if ($reflClass->hasMethod($setter)) {
                 if (!$reflClass->getMethod($setter)->isPublic()) {
                     throw new PropertyAccessDeniedException(sprintf('Method "%s()" is not public in class "%s"', $setter, $reflClass->getName()));
                 }
@@ -583,6 +522,81 @@ class PropertyPath implements \IteratorAggregate, PropertyPathInterface
         return preg_replace_callback('/(^|_|\.)+(.)/', function ($match) { return ('.' === $match[1] ? '_' : '').strtoupper($match[2]); }, $string);
     }
 
+    /**
+     * Searches for add and remove methods.
+     *
+     * @param \ReflectionClass $reflClass The reflection class for the given object
+     * @param string           $singular  The singular form of the property name or null.
+     *
+     * @return array|null An array containin the adder and remover when found, null otherwise.
+     *
+     * @throws InvalidPropertyException      If the property does not exist.
+     */
+    private function findAdderAndRemover(\ReflectionClass $reflClass, $singular)
+    {
+        if (null !== $singular) {
+            $addMethod = 'add' . ucfirst($singular);
+            $removeMethod = 'remove' . ucfirst($singular);
+
+            if (!$this->isAccessible($reflClass, $addMethod, 1)) {
+                throw new InvalidPropertyException(sprintf(
+                    'The public method "%s" with exactly one required parameter was not found on class %s',
+                    $addMethod,
+                    $reflClass->getName()
+                ));
+            }
+
+            if (!$this->isAccessible($reflClass, $removeMethod, 1)) {
+                throw new InvalidPropertyException(sprintf(
+                    'The public method "%s" with exactly one required parameter was not found on class %s',
+                    $removeMethod,
+                    $reflClass->getName()
+                ));
+            }
+
+            return array($addMethod, $removeMethod);
+        } else {
+            // The plural form is the last element of the property path
+            $plural = ucfirst($this->elements[$this->length - 1]);
+
+            // Any of the two methods is required, but not yet known
+            $singulars = (array) FormUtil::singularify($plural);
+
+            foreach ($singulars as $singular) {
+                $methodsFound = 0;
+                $addMethodFound = false;
+                $addMethodName = 'add' . $singular;
+                $removeMethodName = 'remove' . $singular;
+
+                if ($this->isAccessible($reflClass, $addMethodName, 1)) {
+                    $addMethod = $addMethodName;
+                    $addMethodFound = true;
+                    $methodsFound++;
+                }
+
+                if ($this->isAccessible($reflClass, $removeMethodName, 1)) {
+                    $removeMethod = $removeMethodName;
+                    $methodsFound++;
+                }
+
+                if (2 == $methodsFound) {
+                    return array($addMethod, $removeMethod);
+                }
+
+                if (1 == $methodsFound) {
+                    throw new InvalidPropertyException(sprintf(
+                        'Found the public method "%s", but did not find a public "%s" on class %s',
+                        $addMethodFound ? $addMethodName : $removeMethodName,
+                        $addMethodFound ? $removeMethodName : $addMethodName,
+                        $reflClass->getName()
+                    ));
+                }
+            }
+        }
+
+        return null;
+    }
+
     /**
      * Returns whether a method is public and has a specific number of required parameters.
      *