[Form] Fixed issues mentioned in the PR comments

This commit is contained in:
Bernhard Schussek 2012-05-20 11:18:31 +02:00
parent 7a4ba521c4
commit ea5ff77121
30 changed files with 242 additions and 200 deletions

View File

@ -89,11 +89,11 @@ class ViolationMapper
}
}
$template = $violation->getMessageTemplate();
$parameters = $violation->getMessageParameters();
$pluralization = $violation->getMessagePluralization();
$this->scope->addError(new FormError($template, $parameters, $pluralization));
$this->scope->addError(new FormError(
$violation->getMessageTemplate(),
$violation->getMessageParameters(),
$violation->getMessagePluralization()
));
}
/**
@ -220,12 +220,12 @@ class ViolationMapper
// Property path of a mapped form is null
// Should not happen, bail out
break;
} else {
}
$propertyPathBuilder->replace($i, 1, $propertyPath);
$i += $propertyPath->getLength();
}
}
}
$finalPath = $propertyPathBuilder->getPropertyPath();

View File

@ -43,7 +43,7 @@ class ViolationPath implements \IteratorAggregate, PropertyPathInterface
/**
* @var string
*/
private $string = '';
private $pathAsString = '';
/**
* @var integer
@ -59,32 +59,28 @@ class ViolationPath implements \IteratorAggregate, PropertyPathInterface
public function __construct($violationPath)
{
$path = new PropertyPath($violationPath);
$pathElements = $path->getElements();
$pathPositions = $path->getPositions();
$elements = array();
$positions = array();
$isIndex = array();
$mapsForm = array();
$elements = $path->getElements();
$positions = $path->getPositions();
$data = false;
for ($i = 0, $l = count($pathElements); $i < $l; ++$i) {
for ($i = 0, $l = count($elements); $i < $l; ++$i) {
if (!$data) {
// The element "data" has not yet been passed
if ('children' === $pathElements[$i] && $path->isProperty($i)) {
if ('children' === $elements[$i] && $path->isProperty($i)) {
// Skip element "children"
++$i;
// Next element must exist and must be an index
// Otherwise not a valid path
// Otherwise consider this the end of the path
if ($i >= $l || !$path->isIndex($i)) {
return;
break;
}
$elements[] = $pathElements[$i];
$positions[] = $pathPositions[$i];
$isIndex[] = true;
$mapsForm[] = true;
} elseif ('data' === $pathElements[$i] && $path->isProperty($i)) {
$this->elements[] = $elements[$i];
$this->positions[] = $positions[$i];
$this->isIndex[] = true;
$this->mapsForm[] = true;
} elseif ('data' === $elements[$i] && $path->isProperty($i)) {
// Skip element "data"
++$i;
@ -93,32 +89,28 @@ class ViolationPath implements \IteratorAggregate, PropertyPathInterface
break;
}
$elements[] = $pathElements[$i];
$positions[] = $pathPositions[$i];
$isIndex[] = $path->isIndex($i);
$mapsForm[] = false;
$this->elements[] = $elements[$i];
$this->positions[] = $positions[$i];
$this->isIndex[] = $path->isIndex($i);
$this->mapsForm[] = false;
$data = true;
} else {
// Neither "children" nor "data" property found
// Be nice and consider this the end of the path
// Consider this the end of the path
break;
}
} else {
// Already after the "data" element
// Pick everything as is
$elements[] = $pathElements[$i];
$positions[] = $pathPositions[$i];
$isIndex[] = $path->isIndex($i);
$mapsForm[] = false;
$this->elements[] = $elements[$i];
$this->positions[] = $positions[$i];
$this->isIndex[] = $path->isIndex($i);
$this->mapsForm[] = false;
}
}
$this->elements = $elements;
$this->positions = $positions;
$this->isIndex = $isIndex;
$this->mapsForm = $mapsForm;
$this->length = count($elements);
$this->string = $violationPath;
$this->length = count($this->elements);
$this->pathAsString = $violationPath;
$this->resizeString();
}
@ -128,7 +120,7 @@ class ViolationPath implements \IteratorAggregate, PropertyPathInterface
*/
public function __toString()
{
return $this->string;
return $this->pathAsString;
}
/**
@ -241,7 +233,7 @@ class ViolationPath implements \IteratorAggregate, PropertyPathInterface
$lastIndex = $this->length - 1;
if ($lastIndex < 0) {
$this->string = '';
$this->pathAsString = '';
} else {
// +1 for the dot/opening bracket
$length = $this->positions[$lastIndex] + strlen($this->elements[$lastIndex]) + 1;
@ -251,7 +243,7 @@ class ViolationPath implements \IteratorAggregate, PropertyPathInterface
++$length;
}
$this->string = substr($this->string, 0, $length);
$this->pathAsString = substr($this->pathAsString, 0, $length);
}
}
}

View File

@ -127,8 +127,8 @@ class Form implements \IteratorAggregate, FormInterface
*/
public function __construct(FormConfigInterface $config)
{
if (!$config instanceof ImmutableFormConfig) {
$config = new ImmutableFormConfig($config);
if (!$config instanceof UnmodifiableFormConfig) {
$config = new UnmodifiableFormConfig($config);
}
$this->config = $config;
@ -146,7 +146,7 @@ class Form implements \IteratorAggregate, FormInterface
/**
* Returns the configuration of the form.
*
* @return ImmutableFormConfig The form's immutable configuration.
* @return UnmodifiableFormConfig The form's immutable configuration.
*/
public function getConfig()
{
@ -338,20 +338,32 @@ class Form implements \IteratorAggregate, FormInterface
// Validate if client data matches data class (unless empty)
if (!empty($clientData)) {
// TESTME
$dataClass = $this->config->getDataClass();
if (null === $dataClass && is_object($clientData)) {
// TODO clarify error message: should not be transformed to object if data class is null
// possible solutions: set data class or change client transformers
throw new UnexpectedTypeException($clientData, 'scalar or array');
$expectedType = 'scalar';
if (count($this->children) > 0 && $this->config->getDataMapper()) {
$expectedType = 'array';
}
throw new FormException(
'The form\'s client data is expected to of type ' . $expectedType . ', ' .
'but is an instance of class ' . get_class($clientData) . '. You ' .
'can avoid this error by setting the "data_class" option to ' .
'"' . get_class($clientData) . '" or by adding a client transformer ' .
'that transforms ' . get_class($clientData) . ' to ' . $expectedType . '.'
);
}
// TESTME
if (null !== $dataClass && !$clientData instanceof $dataClass) {
// TODO clarify error message: should be transformed to object if data class is set
// possible solutions: change data class or client transformers
throw new UnexpectedTypeException($clientData, $dataClass);
throw new FormException(
'The form\'s client data is expected to be an instance of class ' .
$dataClass . ', but has the type ' . gettype($clientData) . '. You ' .
'can avoid this error by setting the "data_class" option to ' .
'null or by adding a client transformer that transforms ' .
gettype($clientData) . ' to ' . $dataClass . '.'
);
}
}

View File

@ -176,9 +176,7 @@ class FormBuilder extends FormConfig
*/
public function remove($name)
{
if (isset($this->unresolvedChildren[$name])) {
unset($this->unresolvedChildren[$name]);
}
if (isset($this->children[$name])) {
if ($this->children[$name] instanceof self) {

View File

@ -24,7 +24,7 @@ use Symfony\Component\EventDispatcher\EventSubscriberInterface;
class FormConfig implements FormConfigInterface
{
/**
* @var \Symfony\Component\EventDispatcher\EventDispatcherInterface
* @var EventDispatcherInterface
*/
private $dispatcher;
@ -64,7 +64,7 @@ class FormConfig implements FormConfigInterface
private $dataMapper;
/**
* @var FormValidatorInterface
* @var array
*/
private $validators = array();
@ -374,7 +374,7 @@ class FormConfig implements FormConfigInterface
/**
* {@inheritdoc}
*/
function hasAttribute($name)
public function hasAttribute($name)
{
return isset($this->attributes[$name]);
}
@ -382,7 +382,7 @@ class FormConfig implements FormConfigInterface
/**
* {@inheritdoc}
*/
function getAttribute($name)
public function getAttribute($name)
{
return isset($this->attributes[$name]) ? $this->attributes[$name] : null;
}

View File

@ -108,7 +108,7 @@ interface FormConfigInterface
/**
* Returns the data that should be returned when the form is empty.
*
* @return mixed|\Closure The data returned if the form is empty.
* @return mixed The data returned if the form is empty.
*/
function getEmptyData();

View File

@ -22,6 +22,8 @@ interface FormInterface extends \ArrayAccess, \Traversable, \Countable
* Sets the parent form.
*
* @param FormInterface $parent The parent form
*
* @return FormInterface The form instance
*/
function setParent(FormInterface $parent = null);
@ -43,6 +45,8 @@ interface FormInterface extends \ArrayAccess, \Traversable, \Countable
* Adds a child to the form.
*
* @param FormInterface $child The FormInterface to add as a child
*
* @return FormInterface The form instance
*/
function add(FormInterface $child);
@ -68,6 +72,8 @@ interface FormInterface extends \ArrayAccess, \Traversable, \Countable
* Removes a child from the form.
*
* @param string $name The name of the child to remove
*
* @return FormInterface The form instance
*/
function remove($name);
@ -97,7 +103,7 @@ interface FormInterface extends \ArrayAccess, \Traversable, \Countable
*
* @param array $appData The data formatted as expected for the underlying object
*
* @return Form The current form
* @return FormInterface The form instance
*/
function setData($appData);
@ -163,6 +169,8 @@ interface FormInterface extends \ArrayAccess, \Traversable, \Countable
* Adds an error to this form.
*
* @param FormError $error
*
* @return FormInterface The form instance
*/
function addError(FormError $error);
@ -215,6 +223,8 @@ interface FormInterface extends \ArrayAccess, \Traversable, \Countable
* Writes data into the form.
*
* @param mixed $data The data
*
* @return FormInterface The form instance
*/
function bind($data);

View File

@ -59,7 +59,7 @@ class FormConfigTest extends \PHPUnit_Framework_TestCase
/**
* @dataProvider getHtml4Ids
*/
public function testSetNameAcceptsOnlyNamesValidAsIdsInHtml4($name, $accepted)
public function testNameAcceptsOnlyNamesValidAsIdsInHtml4($name, $accepted)
{
$dispatcher = $this->getMock('Symfony\Component\EventDispatcher\EventDispatcherInterface');

View File

@ -1234,6 +1234,36 @@ class FormTest extends \PHPUnit_Framework_TestCase
$this->assertEquals(new PropertyPath('[name]'), $form->getPropertyPath());
}
/**
* @expectedException Symfony\Component\Form\Exception\FormException
*/
public function testClientDataMustNotBeObjectIfDataClassIsNull()
{
$config = new FormConfig('name', null, $this->dispatcher);
$config->appendClientTransformer(new FixedDataTransformer(array(
'' => '',
'foo' => new \stdClass(),
)));
$form = new Form($config);
$form->setData('foo');
}
/**
* @expectedException Symfony\Component\Form\Exception\FormException
*/
public function testClientDataMustBeObjectIfDataClassIsSet()
{
$config = new FormConfig('name', 'stdClass', $this->dispatcher);
$config->appendClientTransformer(new FixedDataTransformer(array(
'' => '',
'foo' => array('bar' => 'baz'),
)));
$form = new Form($config);
$form->setData('foo');
}
protected function getBuilder($name = 'name', EventDispatcherInterface $dispatcher = null, $dataClass = null)
{
return new FormBuilder($name, $dataClass, $dispatcher ?: $this->dispatcher, $this->factory);

View File

@ -93,40 +93,22 @@ class PropertyPathBuilderTest extends \PHPUnit_Framework_TestCase
public function testReplaceByIndex()
{
$this->builder->replaceByIndex(1, 1, 'new1');
$this->builder->replaceByIndex(1, 'new1');
$path = new PropertyPath('old1[new1].old3[old4][old5].old6');
$this->assertEquals($path, $this->builder->getPropertyPath());
}
public function testReplaceByIndexWithLength()
{
$this->builder->replaceByIndex(0, 2, 'new1');
$path = new PropertyPath('[new1].old3[old4][old5].old6');
$this->assertEquals($path, $this->builder->getPropertyPath());
}
public function testReplaceByProperty()
{
$this->builder->replaceByProperty(1, 1, 'new1');
$this->builder->replaceByProperty(1, 'new1');
$path = new PropertyPath('old1.new1.old3[old4][old5].old6');
$this->assertEquals($path, $this->builder->getPropertyPath());
}
public function testReplaceByPropertyWithLength()
{
$this->builder->replaceByProperty(0, 2, 'new1');
$path = new PropertyPath('new1.old3[old4][old5].old6');
$this->assertEquals($path, $this->builder->getPropertyPath());
}
public function testReplace()
{
$this->builder->replace(1, 1, new PropertyPath('new1[new2].new3'));

View File

@ -464,6 +464,11 @@ class PropertyPathTest extends \PHPUnit_Framework_TestCase
new PropertyPath(false);
}
public function testValidPropertyPath_zero()
{
new PropertyPath('0');
}
public function testGetParent_dot()
{
$propertyPath = new PropertyPath('grandpa.parent.child');

View File

@ -13,13 +13,14 @@ namespace Symfony\Component\Form;
use Symfony\Component\Form\Exception\UnexpectedTypeException;
use Symfony\Component\Form\Util\PropertyPath;
use Symfony\Component\EventDispatcher\UnmodifiableEventDispatcher;
/**
* An immutable form configuration.
* A read-only form configuration.
*
* @author Bernhard Schussek <bschussek@gmail.com>
*/
class ImmutableFormConfig implements FormConfigInterface
class UnmodifiableFormConfig implements FormConfigInterface
{
/**
* @var \Symfony\Component\EventDispatcher\EventDispatcherInterface
@ -102,13 +103,18 @@ class ImmutableFormConfig implements FormConfigInterface
private $dataClass;
/**
* Creates an immutable copy of a given configuration.
* Creates an unmodifiable copy of a given configuration.
*
* @param FormConfigInterface $config The configuration to copy.
*/
public function __construct(FormConfigInterface $config)
{
$this->dispatcher = $config->getEventDispatcher();
$dispatcher = $config->getEventDispatcher();
if (!$dispatcher instanceof UnmodifiableEventDispatcher) {
$dispatcher = new UnmodifiableEventDispatcher($dispatcher);
}
$this->dispatcher = $dispatcher;
$this->name = $config->getName();
$this->propertyPath = $config->getPropertyPath();
$this->mapped = $config->getMapped();
@ -243,7 +249,7 @@ class ImmutableFormConfig implements FormConfigInterface
/**
* {@inheritdoc}
*/
function hasAttribute($name)
public function hasAttribute($name)
{
return isset($this->attributes[$name]);
}
@ -251,7 +257,7 @@ class ImmutableFormConfig implements FormConfigInterface
/**
* {@inheritdoc}
*/
function getAttribute($name)
public function getAttribute($name)
{
return isset($this->attributes[$name]) ? $this->attributes[$name] : null;
}

View File

@ -60,7 +60,7 @@ class PropertyPath implements \IteratorAggregate, PropertyPathInterface
* String representation of the path
* @var string
*/
private $string;
private $pathAsString;
/**
* Positions where the individual elements start in the string representation
@ -85,20 +85,20 @@ class PropertyPath implements \IteratorAggregate, PropertyPathInterface
$this->singulars = $propertyPath->singulars;
$this->length = $propertyPath->length;
$this->isIndex = $propertyPath->isIndex;
$this->string = $propertyPath->string;
$this->pathAsString = $propertyPath->pathAsString;
$this->positions = $propertyPath->positions;
return;
}
if (!is_string($propertyPath)) {
throw new UnexpectedTypeException($propertyPath, 'string');
throw new UnexpectedTypeException($propertyPath, 'string or Symfony\Component\Form\Util\PropertyPath');
}
if (empty($propertyPath)) {
if ('' === $propertyPath) {
throw new InvalidPropertyPathException('The property path should not be empty.');
}
$this->string = (string) $propertyPath;
$this->pathAsString = $propertyPath;
$position = 0;
$remaining = $propertyPath;
@ -149,7 +149,7 @@ class PropertyPath implements \IteratorAggregate, PropertyPathInterface
*/
public function __toString()
{
return $this->string;
return $this->pathAsString;
}
/**
@ -180,7 +180,7 @@ class PropertyPath implements \IteratorAggregate, PropertyPathInterface
$parent = clone $this;
--$parent->length;
$parent->string = substr($parent->string, 0, $parent->positions[$parent->length]);
$parent->pathAsString = substr($parent->pathAsString, 0, $parent->positions[$parent->length]);
array_pop($parent->elements);
array_pop($parent->singulars);
array_pop($parent->isIndex);

View File

@ -46,6 +46,7 @@ class PropertyPathBuilder
* @param integer $offset The offset where the appended piece
* starts in $path.
* @param integer $length The length of the appended piece.
* If 0, the full path is appended.
*/
public function append(PropertyPathInterface $path, $offset = 0, $length = 0)
{
@ -103,6 +104,7 @@ class PropertyPathBuilder
* @param integer $pathOffset The offset where the inserted piece
* starts in $path.
* @param integer $pathLength The length of the inserted piece.
* If 0, the full path is inserted.
*/
public function replace($offset, $length, PropertyPathInterface $path, $pathOffset = 0, $pathLength = 0)
{
@ -122,13 +124,10 @@ class PropertyPathBuilder
* Replaces a sub-path by a single index element.
*
* @param integer $offset The offset at which to replace.
* @param integer $length The length of the piece to replace.
* @param string $name The inserted index name.
*/
public function replaceByIndex($offset, $length, $name)
public function replaceByIndex($offset, $name)
{
$this->resize($offset, $length, 1);
$this->elements[$offset] = $name;
$this->isIndex[$offset] = true;
}
@ -137,26 +136,66 @@ class PropertyPathBuilder
* Replaces a sub-path by a single property element.
*
* @param integer $offset The offset at which to replace.
* @param integer $length The length of the piece to replace.
* @param string $name The inserted property name.
*/
public function replaceByProperty($offset, $length, $name)
public function replaceByProperty($offset, $name)
{
$this->resize($offset, $length, 1);
$this->elements[$offset] = $name;
$this->isIndex[$offset] = false;
}
/**
* Returns the length of the current path.
*
* @return integer The path length.
*/
public function getLength()
{
return count($this->elements);
}
/**
* Returns the current property path.
*
* @return PropertyPathInterface The constructed property path.
*/
public function getPropertyPath()
{
$pathAsString = $this->__toString();
return '' !== $pathAsString ? new PropertyPath($pathAsString) : null;
}
/**
* Returns the current property path as string.
*
* @return string The property path as string.
*/
public function __toString()
{
$string = '';
foreach ($this->elements as $offset => $element) {
if ($this->isIndex[$offset]) {
$element = '[' . $element . ']';
} elseif ('' !== $string) {
$string .= '.';
}
$string .= $element;
}
return $string;
}
/**
* Resizes the path so that a chunk of length $cutLength is
* removed at $offset and another chunk of length $insertionLength
* is inserted.
* can be inserted.
*
* @param integer $offset The offset where a chunk should be removed.
* @param $cutLength
* @param $insertionLength
* @return mixed
* @param integer $offset The offset where the removed chunk starts.
* @param integer $cutLength The length of the removed chunk.
* @param integer $insertionLength The length of the inserted chunk.
*/
private function resize($offset, $cutLength, $insertionLength)
{
@ -210,36 +249,4 @@ class PropertyPathBuilder
}
}
}
/**
* Returns the length of the current path.
*
* @return integer The path length.
*/
public function getLength()
{
return count($this->elements);
}
/**
* Returns the current property path.
*
* @return PropertyPathInterface The constructed property path.
*/
public function getPropertyPath()
{
$string = null;
foreach ($this->elements as $offset => $element) {
if ($this->isIndex[$offset]) {
$element = '[' . $element . ']';
} elseif (null !== $string) {
$string .= '.';
}
$string .= $element;
}
return null !== $string ? new PropertyPath($string) : null;
}
}

View File

@ -26,7 +26,7 @@ interface PropertyPathIteratorInterface extends \Iterator, \SeekableIterator
/**
* Returns whether the current element in the property path is a property
* names.
* name.
*
* @return Boolean
*/