diogo/symfony
Archived
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

merged branch bschussek/issue8111 (PR #8638)

Browse Source 
This PR was merged into the 2.3 branch.

Discussion
----------

[Form] Fixed patched forms to be valid even if children are not submitted

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

#8362 was reverted because it introduces a failing test that is caused by a regression. This PR takes the alternative approach that

* unsubmitted fields in the PATCH request remain unsubmitted
* `isValid()` ignores unsubmitted children
* `mapFormsToData()` ignores unsubmitted children

In my opinion this is a more proper solution than #8362.

Commits
-------

85330a6 [Form] Fixed patched forms to be valid even if children are not submitted
50f201e Revert "[Form] Fix of "PATCH'ed forms are never valid""
This commit is contained in:
Fabien Potencier 2013-08-02 15:20:14 +02:00
commit ef69619614
5 changed files with 76 additions and 162 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php
View File

@ -78,9 +78,9 @@ class PropertyPathMapper implements DataMapperInterface
$propertyPath = $form->getPropertyPath();
$config = $form->getConfig();
// Write-back is disabled if the form is not synchronized (transformation failed)
// and if the form is disabled (modification not allowed)
if (null !== $propertyPath && $config->getMapped() && $form->isSynchronized() && !$form->isDisabled()) {
// Write-back is disabled if the form is not synchronized (transformation failed),
// if the form was not submitted and if the form is disabled (modification not allowed)
if (null !== $propertyPath && $config->getMapped() && $form->isSubmitted() && $form->isSynchronized() && !$form->isDisabled()) {
// If the data is identical to the value in $data, we are
// dealing with a reference
if (!is_object($data) || !$config->getByReference() || $form->getData() !== $this->propertyAccessor->getValue($data, $propertyPath)) {

26
src/Symfony/Component/Form/Form.php
View File

@ -500,14 +500,6 @@ class Form implements \IteratorAggregate, FormInterface
return $this;
}
// In order to process patch requests we need to "submit" null values
// Later this value will be mapped via DataMapper and InheritDataAwareIterator
if ($submittedData === null && !$clearMissing) {
$this->submitted = true;
return $this;
}
// The data must be initialized if it was not initialized yet.
// This is necessary to guarantee that the *_SET_DATA listeners
// are always invoked before submit() takes place.
@ -545,12 +537,10 @@ class Form implements \IteratorAggregate, FormInterface
}
foreach ($this->children as $name => $child) {
$fieldValue = null;
if (array_key_exists($name, $submittedData)) {
$fieldValue = $submittedData[$name];
if (array_key_exists($name, $submittedData) || $clearMissing) {
$child->submit(isset($submittedData[$name]) ? $submittedData[$name] : null, $clearMissing);
unset($submittedData[$name]);
}
$child->submit($fieldValue, $clearMissing);
}
$this->extraData = $submittedData;
@ -720,11 +710,13 @@ class Form implements \IteratorAggregate, FormInterface
return false;
}
if (!$this->isDisabled()) {
foreach ($this->children as $child) {
if (!$child->isValid()) {
return false;
}
if ($this->isDisabled()) {
return true;
}
foreach ($this->children as $child) {
if ($child->isSubmitted() && !$child->isValid()) {
return false;
}
}

32
src/Symfony/Component/Form/Tests/AbstractFormTest.php
View File

@ -89,38 +89,6 @@ abstract class AbstractFormTest extends \PHPUnit_Framework_TestCase
return $form;
}
/**
* @param string $name
*
* @return \PHPUnit_Framework_MockObject_MockObject
*/
protected function getValidForm($name)
{
$form = $this->getMockForm($name);
$form->expects($this->any())
->method('isValid')
->will($this->returnValue(true));
return $form;
}
/**
* @param string $name
*
* @return \PHPUnit_Framework_MockObject_MockObject
*/
protected function getInvalidForm($name)
{
$form = $this->getMockForm($name);
$form->expects($this->any())
->method('isValid')
->will($this->returnValue(false));
return $form;
}
/**
* @return \PHPUnit_Framework_MockObject_MockObject
*/

148
src/Symfony/Component/Form/Tests/CompoundFormTest.php
View File

@ -22,8 +22,8 @@ class CompoundFormTest extends AbstractFormTest
{
public function testValidIfAllChildrenAreValid()
{
$this->form->add($this->getValidForm('firstName'));
$this->form->add($this->getValidForm('lastName'));
$this->form->add($this->getBuilder('firstName')->getForm());
$this->form->add($this->getBuilder('lastName')->getForm());
$this->form->submit(array(
'firstName' => 'Bernhard',
@ -35,17 +35,51 @@ class CompoundFormTest extends AbstractFormTest
public function testInvalidIfChildIsInvalid()
{
$this->form->add($this->getValidForm('firstName'));
$this->form->add($this->getInvalidForm('lastName'));
$this->form->add($this->getBuilder('firstName')->getForm());
$this->form->add($this->getBuilder('lastName')->getForm());
$this->form->submit(array(
'firstName' => 'Bernhard',
'lastName' => 'Schussek',
));
$this->form->get('lastName')->addError(new FormError('Invalid'));
$this->assertFalse($this->form->isValid());
}
public function testValidIfChildIsNotSubmitted()
{
$this->form->add($this->getBuilder('firstName')->getForm());
$this->form->add($this->getBuilder('lastName')->getForm());
$this->form->submit(array(
'firstName' => 'Bernhard',
));
// "lastName" is not "valid" because it was not submitted. This happens
// for example in PATCH requests. The parent form should still be
// considered valid.
$this->assertTrue($this->form->isValid());
}
public function testDisabledFormsValidEvenIfChildrenInvalid()
{
$form = $this->getBuilder('person')
->setDisabled(true)
->setCompound(true)
->setDataMapper($this->getDataMapper())
->add($this->getBuilder('name'))
->getForm();
$form->submit(array('name' => 'Jacques Doe'));
$form->get('name')->addError(new FormError('Invalid'));
$this->assertTrue($form->isValid());
}
public function testSubmitForwardsNullIfValueIsMissing()
{
$child = $this->getMockForm('firstName');
@ -59,17 +93,14 @@ class CompoundFormTest extends AbstractFormTest
$this->form->submit(array());
}
public function testSubmitDoesNotSaveNullIfNotClearMissing()
public function testSubmitDoesNotForwardNullIfNotClearMissing()
{
$child = $this->getMockForm('firstName');
$this->form->add($child);
$child->expects($this->once())
->method('submit');
$child->expects($this->never())
->method('setData');
->method('submit');
$this->form->submit(array(), false);
}
@ -124,37 +155,6 @@ class CompoundFormTest extends AbstractFormTest
$this->assertFalse($this->form->isEmpty());
}
public function testValidIfSubmittedAndDisabledWithChildren()
{
$this->factory->expects($this->once())
->method('createNamedBuilder')
->with('name', 'text', null, array())
->will($this->returnValue($this->getBuilder('name')));
$form = $this->getBuilder('person')
->setDisabled(true)
->setCompound(true)
->setDataMapper($this->getDataMapper())
->add('name', 'text')
->getForm();
$form->submit(array('name' => 'Jacques Doe'));
$this->assertTrue($form->isValid());
}
public function testNotValidIfChildNotValid()
{
$child = $this->getMockForm();
$child->expects($this->once())
->method('isValid')
->will($this->returnValue(false));
$this->form->add($child);
$this->form->submit(array());
$this->assertFalse($this->form->isValid());
}
public function testAdd()
{
$child = $this->getBuilder('foo')->getForm();
@ -564,74 +564,6 @@ class CompoundFormTest extends AbstractFormTest
unlink($path);
}
public function testSubmitPatchRequest()
{
if (!class_exists('Symfony\Component\HttpFoundation\Request')) {
$this->markTestSkipped('The "HttpFoundation" component is not available');
}
$object = new \stdClass();
$object->name = 'Bernhard';
$object->name2 = 'Me';
//$values = array();
$values = array(
'author' => array(
'name2' => 'Artem',
),
);
$request = new Request(array(), $values, array(), array(), array(), array(
'REQUEST_METHOD' => 'PATCH',
));
$mapper = $this->getDataMapper();
$mapper->expects($this->any())
->method('mapDataToForms')
->will($this->returnCallback(function($data, $forms){
foreach ($forms as $form) {
$propertyPath = $form->getPropertyPath();
if (null !== $propertyPath) {
$form->setData($data->{$propertyPath});
}
}
}));
$mapper->expects($this->any())
->method('mapFormsToData')
->will($this->returnCallback(function($forms, &$data){
foreach ($forms as $form) {
$propertyPath = $form->getPropertyPath();
if (null !== $propertyPath) {
if (!is_object($data) || $form->getData() !== $data->{$propertyPath}) {
$data->{$propertyPath} = $form->getData();
}
}
}
}));
/** @var Form $form */
$form = $this->getBuilder('author', null, 'stdClass')
->setMethod('PATCH')
->setData($object)
->setCompound(true)
->setDataMapper($mapper)
->setRequestHandler(new HttpFoundationRequestHandler())
->getForm();
$form->add($this->getBuilder('name')->getForm());
$form->add($this->getBuilder('name2')->getForm());
$form->handleRequest($request);
$this->assertTrue($form->isValid());
$this->assertEquals('Bernhard', $form['name']->getData());
$this->assertEquals('Artem', $form['name2']->getData());
}
/**
* @dataProvider requestMethodProvider
*/

26
src/Symfony/Component/Form/Tests/Extension/Core/DataMapper/PropertyPathMapperTest.php
View File

@ -64,17 +64,21 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase
* @param Boolean $synchronized
* @return \PHPUnit_Framework_MockObject_MockObject
*/
private function getForm(FormConfigInterface $config, $synchronized = true)
private function getForm(FormConfigInterface $config, $synchronized = true, $submitted = true)
{
$form = $this->getMockBuilder('Symfony\Component\Form\Form')
->setConstructorArgs(array($config))
->setMethods(array('isSynchronized'))
->setMethods(array('isSynchronized', 'isSubmitted'))
->getMock();
$form->expects($this->any())
->method('isSynchronized')
->will($this->returnValue($synchronized));
$form->expects($this->any())
->method('isSubmitted')
->will($this->returnValue($submitted));
return $form;
}
@ -263,6 +267,24 @@ class PropertyPathMapperTest extends \PHPUnit_Framework_TestCase
$this->mapper->mapFormsToData(array($form), $car);
}
public function testMapFormsToDataIgnoresUnsubmittedForms()
{
$car = new \stdClass();
$engine = new \stdClass();
$propertyPath = $this->getPropertyPath('engine');
$this->propertyAccessor->expects($this->never())
->method('setValue');
$config = new FormConfigBuilder('name', '\stdClass', $this->dispatcher);
$config->setByReference(true);
$config->setPropertyPath($propertyPath);
$config->setData($engine);
$form = $this->getForm($config, true, false);
$this->mapper->mapFormsToData(array($form), $car);
}
public function testMapFormsToDataIgnoresEmptyData()
{
$car = new \stdClass();