Skip to content

Commit

Permalink
[Form] Fixed: errors are not mapped to unsynchronized forms anymore
Browse files Browse the repository at this point in the history
  • Loading branch information
webmozart committed May 22, 2012
1 parent c8b61d5 commit c9c4900
Show file tree
Hide file tree
Showing 9 changed files with 103 additions and 29 deletions.
1 change: 1 addition & 0 deletions src/Symfony/Component/Form/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ CHANGELOG
* added option "mapped" which should be used instead of setting "property_path" to false
* "data_class" now *must* be set if a form maps to an object and should be left empty otherwise
* improved error mapping on forms
* errors are not mapped to unsynchronized forms anymore
* changed Form constructor to accept a single `FormConfigInterface` object
* changed argument order in the FormBuilder constructor
* deprecated Form methods
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public function buildForm(FormBuilder $builder, array $options)
// BC compatibility, when "property_path" could be false
->setPropertyPath(is_string($options['property_path']) ? $options['property_path'] : null)
->setMapped($options['mapped'])
->setVirtual($options['virtual'])
->setAttribute('read_only', $options['read_only'])
->setAttribute('by_reference', $options['by_reference'])
->setAttribute('error_mapping', $options['error_mapping'])
Expand All @@ -58,7 +59,6 @@ public function buildForm(FormBuilder $builder, array $options)
->setAttribute('invalid_message', $options['invalid_message'])
->setAttribute('invalid_message_parameters', $options['invalid_message_parameters'])
->setAttribute('translation_domain', $options['translation_domain'])
->setAttribute('virtual', $options['virtual'])
->setAttribute('single_control', $options['single_control'])
->setData($options['data'])
->setDataMapper(new PropertyPathMapper())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
use Symfony\Component\Form\FormEvents;
use Symfony\Component\Form\Event\DataEvent;
use Symfony\Component\Form\Exception\FormException;
use Symfony\Component\Form\Util\VirtualFormAwareIterator;
use Symfony\Component\Form\Util\PropertyPath;
use Symfony\Component\Validator\Constraint;
use Symfony\Component\Validator\ValidatorInterface;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,18 @@ public function mapViolation(ConstraintViolation $violation, FormInterface $form
}
}

$this->scope->addError(new FormError(
$violation->getMessageTemplate(),
$violation->getMessageParameters(),
$violation->getMessagePluralization()
));
// Only add the error if the form is synchronized
// If the form is not synchronized, it already contains an
// error about being invalid. Further errors could be a result
// of the failed transformation and thus should not be
// displayed.
if ($this->scope->isSynchronized()) {
$this->scope->addError(new FormError(
$violation->getMessageTemplate(),
$violation->getMessageParameters(),
$violation->getMessagePluralization()
));
}
}

/**
Expand Down Expand Up @@ -204,7 +211,7 @@ private function reconstructPath(ViolationPath $violationPath, FormInterface $or
// Process child form
$scope = $scope->get($it->current());

if ($scope->getAttribute('virtual')) {
if ($scope->getConfig()->getVirtual()) {
// Form is virtual
// Cut the piece out of the property path and proceed
$propertyPathBuilder->remove($i);
Expand Down
27 changes: 27 additions & 0 deletions src/Symfony/Component/Form/FormConfig.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ class FormConfig implements FormConfigInterface
*/
private $mapped;

/**
* @var Boolean
*/
private $virtual;

/**
* @var array
*/
Expand Down Expand Up @@ -289,6 +294,14 @@ public function getMapped()
return $this->mapped;
}

/**
* {@inheritdoc}
*/
public function getVirtual()
{
return $this->virtual;
}

/**
* {@inheritdoc}
*/
Expand Down Expand Up @@ -537,6 +550,20 @@ public function setMapped($mapped)
return $this;
}

/**
* Sets whether the form should be virtual.
*
* @param Boolean $virtual Whether the form should be virtual.
*
* @return self The configuration object.
*/
public function setVirtual($virtual)
{
$this->virtual = $virtual;

return $this;
}

/**
* Set the types.
*
Expand Down
11 changes: 11 additions & 0 deletions src/Symfony/Component/Form/FormConfigInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,17 @@ function getPropertyPath();
*/
function getMapped();

/**
* Returns whether the form should be virtual.
*
* When mapping data to the children of a form, the data mapper
* should ignore virtual forms and map to the children of the
* virtual form instead.
*
* @return Boolean Whether the form is virtual.
*/
function getVirtual();

/**
* Returns the form types used to construct the form.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
namespace Symfony\Component\Form\Tests\Extension\Validator\ViolationMapper;

use Symfony\Component\Form\Extension\Validator\ViolationMapper\ViolationMapper;
use Symfony\Component\Form\Exception\TransformationFailedException;
use Symfony\Component\Form\CallbackTransformer;
use Symfony\Component\Form\Form;
use Symfony\Component\Form\FormConfig;
use Symfony\Component\Form\FormError;
use Symfony\Component\Form\Util\PropertyPath;
use Symfony\Component\Form\FormBuilder;
Expand All @@ -35,11 +39,6 @@ class ViolationMapperTest extends \PHPUnit_Framework_TestCase
*/
private $dispatcher;

/**
* @var \PHPUnit_Framework_MockObject_MockObject
*/
private $factory;

/**
* @var ViolationMapper
*/
Expand All @@ -62,27 +61,27 @@ protected function setUp()
}

$this->dispatcher = $this->getMock('Symfony\Component\EventDispatcher\EventDispatcherInterface');
$this->factory = $this->getMock('Symfony\Component\Form\FormFactoryInterface');
$this->mapper = new ViolationMapper();
$this->message = 'Message';
$this->params = array('foo' => 'bar');
}

protected function getBuilder($name = 'name', $propertyPath = null, $dataClass = null, $errorMapping = array(), $virtual = false)
protected function getForm($name = 'name', $propertyPath = null, $dataClass = null, $errorMapping = array(), $virtual = false, $synchronized = true)
{
$builder = new FormBuilder($name, $dataClass, $this->dispatcher, $this->factory);
$builder->setPropertyPath(new PropertyPath($propertyPath ?: $name));
$builder->setAttribute('error_mapping', $errorMapping);
$builder->setAttribute('virtual', $virtual);
$builder->setErrorBubbling(false);
$builder->setMapped(true);

return $builder;
}
$config = new FormConfig($name, $dataClass, $this->dispatcher);
$config->setMapped(true);
$config->setVirtual($virtual);
$config->setPropertyPath($propertyPath);
$config->setAttribute('error_mapping', $errorMapping);

if (!$synchronized) {
$config->appendClientTransformer(new CallbackTransformer(
function ($normData) { return $normData; },
function () { throw new TransformationFailedException(); }
));
}

protected function getForm($name = 'name', $propertyPath = null, $dataClass = null, $errorMapping = array(), $virtual = false)
{
return $this->getBuilder($name, $propertyPath, $dataClass, $errorMapping, $virtual)->getForm();
return new Form($config);
}

/**
Expand Down Expand Up @@ -1338,4 +1337,21 @@ public function testVirtualFormErrorMapping($target, $childName, $childPath, $gr
$this->assertEquals(array($this->getFormError()), $grandChild->getErrors(), $grandChildName. ' should have an error, but has none');
}
}

public function testDontMapToUnsynchronizedForms()
{
$violation = $this->getConstraintViolation('children[address].data.street');
$parent = $this->getForm('parent');
$child = $this->getForm('address', 'address', null, array(), false, false);

$parent->add($child);

// bind to invoke the transformer and mark the form unsynchronized
$parent->bind(array());

$this->mapper->mapViolation($violation, $parent);

$this->assertFalse($parent->hasErrors(), $parent->getName() . ' should not have an error, but has one');
$this->assertFalse($child->hasErrors(), $child->getName() . ' should not have an error, but has one');
}
}
14 changes: 14 additions & 0 deletions src/Symfony/Component/Form/UnmodifiableFormConfig.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ class UnmodifiableFormConfig implements FormConfigInterface
*/
private $mapped;

/**
* @var Boolean
*/
private $virtual;

/**
* @var array
*/
Expand Down Expand Up @@ -118,6 +123,7 @@ public function __construct(FormConfigInterface $config)
$this->name = $config->getName();
$this->propertyPath = $config->getPropertyPath();
$this->mapped = $config->getMapped();
$this->virtual = $config->getVirtual();
$this->types = $config->getTypes();
$this->clientTransformers = $config->getClientTransformers();
$this->normTransformers = $config->getNormTransformers();
Expand Down Expand Up @@ -164,6 +170,14 @@ public function getMapped()
return $this->mapped;
}

/**
* {@inheritdoc}
*/
public function getVirtual()
{
return $this->virtual;
}

/**
* {@inheritdoc}
*/
Expand Down
3 changes: 1 addition & 2 deletions src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ public function getChildren()

public function hasChildren()
{
return $this->current()->hasAttribute('virtual')
&& $this->current()->getAttribute('virtual');
return $this->current()->getConfig()->getVirtual();
}
}

0 comments on commit c9c4900

Please sign in to comment.