From 613b5f647b22d6f0b880d1a022b00f3d3f1e6e55 Mon Sep 17 00:00:00 2001 From: Toni Uebernickel Date: Wed, 6 Nov 2013 15:01:49 +0100 Subject: [PATCH] re-factor Propel1 ModelChoiceList * add BaseModelChoiceListTest ensuring compatibility * fix keys and order are preserved * fix lazy-load to use filters of initial query --- .../Form/ChoiceList/ModelChoiceList.php | 287 ++++++++++-------- .../Propel1/Tests/Fixtures/ItemQuery.php | 17 +- .../ChoiceList/CompatModelChoiceListTest.php | 128 ++++++++ .../Form/ChoiceList/ModelChoiceListTest.php | 42 ++- .../Bridge/Propel1/Tests/Propel1TestCase.php | 4 +- 5 files changed, 346 insertions(+), 132 deletions(-) create mode 100644 src/Symfony/Bridge/Propel1/Tests/Form/ChoiceList/CompatModelChoiceListTest.php diff --git a/src/Symfony/Bridge/Propel1/Form/ChoiceList/ModelChoiceList.php b/src/Symfony/Bridge/Propel1/Form/ChoiceList/ModelChoiceList.php index 65356f47e222..1cfce4ef00a3 100644 --- a/src/Symfony/Bridge/Propel1/Form/ChoiceList/ModelChoiceList.php +++ b/src/Symfony/Bridge/Propel1/Form/ChoiceList/ModelChoiceList.php @@ -21,7 +21,7 @@ use Symfony\Component\PropertyAccess\PropertyAccessorInterface; /** - * Widely inspired by the EntityChoiceList. + * A choice list for object choices based on Propel model. * * @author William Durand * @author Toni Uebernickel @@ -59,7 +59,7 @@ class ModelChoiceList extends ObjectChoiceList protected $loaded = false; /** - * Whether to use the identifier for index generation + * Whether to use the identifier for index generation. * * @var Boolean */ @@ -68,7 +68,7 @@ class ModelChoiceList extends ObjectChoiceList /** * Constructor. * - * @see Symfony\Bridge\Propel1\Form\Type\ModelType How to use the preferred choices. + * @see \Symfony\Bridge\Propel1\Form\Type\ModelType How to use the preferred choices. * * @param string $class The FQCN of the model class to be loaded. * @param string $labelPath A property path pointing to the property used for the choice labels. @@ -87,8 +87,8 @@ public function __construct($class, $labelPath = null, $choices = null, $queryOb $queryClass = $this->class . 'Query'; $query = new $queryClass(); - $this->identifier = $query->getTableMap()->getPrimaryKeys(); $this->query = $queryObject ?: $query; + $this->identifier = $this->query->getTableMap()->getPrimaryKeys(); $this->loaded = is_array($choices) || $choices instanceof \Traversable; if ($preferred instanceof ModelCriteria) { @@ -102,7 +102,7 @@ public function __construct($class, $labelPath = null, $choices = null, $queryOb $preferred = array(); } - if (1 === count($this->identifier) && $this->isInteger(current($this->identifier))) { + if (1 === count($this->identifier) && $this->isScalar(current($this->identifier))) { $this->identifierAsIndex = true; } @@ -110,7 +110,7 @@ public function __construct($class, $labelPath = null, $choices = null, $queryOb } /** - * Returns the class name + * Returns the class name of the model. * * @return string */ @@ -120,162 +120,179 @@ public function getClass() } /** - * Returns the list of model objects - * - * @return array - * - * @see Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceListInterface + * {@inheritdoc} */ public function getChoices() { - if (!$this->loaded) { - $this->load(); - } + $this->load(); return parent::getChoices(); } /** - * Returns the values for the model objects - * - * @return array - * - * @see Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceListInterface + * {@inheritdoc} */ public function getValues() { - if (!$this->loaded) { - $this->load(); - } + $this->load(); return parent::getValues(); } /** - * Returns the choice views of the preferred choices as nested array with - * the choice groups as top-level keys. - * - * @return array - * - * @see Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceListInterface + * {@inheritdoc} */ public function getPreferredViews() { - if (!$this->loaded) { - $this->load(); - } + $this->load(); return parent::getPreferredViews(); } /** - * Returns the choice views of the choices that are not preferred as nested - * array with the choice groups as top-level keys. - * - * @return array - * - * @see Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceListInterface + * {@inheritdoc} */ public function getRemainingViews() { - if (!$this->loaded) { - $this->load(); - } + $this->load(); return parent::getRemainingViews(); } /** - * Returns the model objects corresponding to the given values. - * - * @param array $values - * - * @return array - * - * @see Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceListInterface + * {@inheritdoc} */ public function getChoicesForValues(array $values) { + if (empty($values)) { + return array(); + } + + /** + * This performance optimization reflects a common scenario: + * * A simple select of a model entry. + * * The choice option "expanded" is set to false. + * * The current request is the submission of the selected value. + * + * @see \Symfony\Component\Form\Extension\Core\DataTransformer\ChoicesToValuesTransformer::reverseTransform + * @see \Symfony\Component\Form\Extension\Core\DataTransformer\ChoiceToValueTransformer::reverseTransform + */ if (!$this->loaded) { if (1 === count($this->identifier)) { $filterBy = 'filterBy' . current($this->identifier)->getPhpName(); - return (array) $this->query->create() + // The initial query is cloned, so all additional filters are applied correctly. + $query = clone $this->query; + $result = (array) $query ->$filterBy($values) ->find(); - } - $this->load(); + // Preserve the keys as provided by the values. + $models = array(); + foreach ($values as $index => $value) { + foreach ($result as $eachModel) { + if ($value === $this->createValue($eachModel)) { + // Make sure to convert to the right format + $models[$index] = $this->fixChoice($eachModel); + + // If all values have been assigned, skip further loops. + unset($values[$index]); + if (0 === count($values)) { + break 2; + } + } + } + } + + return $models; + } } + $this->load(); + return parent::getChoicesForValues($values); } /** - * Returns the values corresponding to the given model objects. - * - * @param array $models - * - * @return array - * - * @see Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceListInterface + * {@inheritdoc} */ public function getValuesForChoices(array $models) { - if (!$this->loaded) { - // Optimize performance for single-field identifiers. We already - // know that the IDs are used as values + if (empty($models)) { + return array(); + } - // Attention: This optimization does not check choices for existence + if (!$this->loaded) { + /** + * This performance optimization assumes the validation of the respective values will be done by other means. + * + * It correlates with the performance optimization in {@link ModelChoiceList::getChoicesForValues()} + * as it won't load the actual entries from the database. + * + * @see \Symfony\Component\Form\Extension\Core\DataTransformer\ChoicesToValuesTransformer::transform + * @see \Symfony\Component\Form\Extension\Core\DataTransformer\ChoiceToValueTransformer::transform + */ if (1 === count($this->identifier)) { $values = array(); - foreach ($models as $model) { + foreach ($models as $index => $model) { if ($model instanceof $this->class) { // Make sure to convert to the right format - $values[] = $this->fixValue(current($this->getIdentifierValues($model))); + $values[$index] = $this->fixValue(current($this->getIdentifierValues($model))); } } return $values; } + } + + $this->load(); + + $values = array(); + $availableValues = $this->getValues(); + + /* + * Overwriting default implementation. + * + * The two objects may represent the same entry in the database, + * but if they originated from different queries, there are not the same object within the code. + * + * This happens when using m:n relations with either sides model as data_class of the form. + * The choicelist will retrieve the list of available related models with a different query, resulting in different objects. + */ + $choices = $this->fixChoices($models); + foreach ($choices as $i => $givenChoice) { + if (null === $givenChoice) { + continue; + } + + foreach ($this->getChoices() as $j => $choice) { + if ($this->isEqual($choice, $givenChoice)) { + $values[$i] = $availableValues[$j]; - $this->load(); + // If all choices have been assigned, skip further loops. + unset($choices[$i]); + if (0 === count($choices)) { + break 2; + } + } + } } - return parent::getValuesForChoices($models); + return $values; } /** - * Returns the indices corresponding to the given models. - * - * @param array $models - * - * @return array - * - * @see Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceListInterface + * {@inheritdoc} */ public function getIndicesForChoices(array $models) { - $indices = array(); - - if (!$this->loaded) { - // Optimize performance for single-field identifiers. We already - // know that the IDs are used as indices - - // Attention: This optimization does not check choices for existence - if ($this->identifierAsIndex) { - foreach ($models as $model) { - if ($model instanceof $this->class) { - // Make sure to convert to the right format - $indices[] = $this->fixIndex(current($this->getIdentifierValues($model))); - } - } + if (empty($models)) { + return array(); + } - return $indices; - } + $this->load(); - $this->load(); - } + $indices = array(); /* * Overwriting default implementation. @@ -287,12 +304,17 @@ public function getIndicesForChoices(array $models) * The choicelist will retrieve the list of available related models with a different query, resulting in different objects. */ $choices = $this->fixChoices($models); - foreach ($this->getChoices() as $i => $choice) { - foreach ($choices as $j => $givenChoice) { - if (null !== $givenChoice && $this->getIdentifierValues($choice) === $this->getIdentifierValues($givenChoice)) { - $indices[] = $i; - unset($choices[$j]); + foreach ($choices as $i => $givenChoice) { + if (null === $givenChoice) { + continue; + } + + foreach ($this->getChoices() as $j => $choice) { + if ($this->isEqual($choice, $givenChoice)) { + $indices[$i] = $j; + // If all choices have been assigned, skip further loops. + unset($choices[$i]); if (0 === count($choices)) { break 2; } @@ -304,28 +326,16 @@ public function getIndicesForChoices(array $models) } /** - * Returns the models corresponding to the given values. - * - * @param array $values - * - * @return array - * - * @see Symfony\Component\Form\Extension\Core\ChoiceList\ChoiceListInterface + * {@inheritdoc} */ public function getIndicesForValues(array $values) { - if (!$this->loaded) { - // Optimize performance for single-field identifiers. We already - // know that the IDs are used as indices and values - - // Attention: This optimization does not check values for existence - if ($this->identifierAsIndex) { - return $this->fixIndices($values); - } - - $this->load(); + if (empty($values)) { + return array(); } + $this->load(); + return parent::getIndicesForValues($values); } @@ -363,7 +373,7 @@ protected function createIndex($model) */ protected function createValue($model) { - if (1 === count($this->identifier)) { + if ($this->identifierAsIndex) { return (string) current($this->getIdentifierValues($model)); } @@ -371,10 +381,16 @@ protected function createValue($model) } /** - * Loads the list with model objects. + * Loads the complete choice list entries, once. + * + * If data has been loaded the choice list is initialized with the retrieved data. */ private function load() { + if ($this->loaded) { + return; + } + $models = (array) $this->query->find(); $preferred = array(); @@ -385,15 +401,15 @@ private function load() try { // The second parameter $labels is ignored by ObjectChoiceList parent::initialize($models, array(), $preferred); + + $this->loaded = true; } catch (StringCastException $e) { throw new StringCastException(str_replace('argument $labelPath', 'option "property"', $e->getMessage()), null, $e); } - - $this->loaded = true; } /** - * Returns the values of the identifier fields of an model + * Returns the values of the identifier fields of a model. * * Propel must know about this model, that is, the model must already * be persisted or added to the idmodel map before. Otherwise an @@ -407,6 +423,10 @@ private function load() */ private function getIdentifierValues($model) { + if (!$model instanceof $this->class) { + return array(); + } + if ($model instanceof Persistent) { return array($model->getPrimaryKey()); } @@ -416,7 +436,7 @@ private function getIdentifierValues($model) return array($model->getPrimaryKey()); } - if (null === $model) { + if (!method_exists($model, 'getPrimaryKeys')) { return array(); } @@ -424,14 +444,39 @@ private function getIdentifierValues($model) } /** - * Whether this column in an integer + * Whether this column contains scalar values (to be used as indices). * * @param \ColumnMap $column * * @return Boolean */ - private function isInteger(\ColumnMap $column) + private function isScalar(\ColumnMap $column) + { + return in_array($column->getPdoType(), array( + \PDO::PARAM_BOOL, + \PDO::PARAM_INT, + \PDO::PARAM_STR, + )); + } + + /** + * Check the given choices for equality. + * + * @param mixed $choice + * @param mixed $givenChoice + * + * @return Boolean + */ + private function isEqual($choice, $givenChoice) { - return $column->getPdoType() === \PDO::PARAM_INT; + if ($choice === $givenChoice) { + return true; + } + + if ($this->getIdentifierValues($choice) === $this->getIdentifierValues($givenChoice)) { + return true; + } + + return false; } } diff --git a/src/Symfony/Bridge/Propel1/Tests/Fixtures/ItemQuery.php b/src/Symfony/Bridge/Propel1/Tests/Fixtures/ItemQuery.php index ba3a9b451613..56a0c943871c 100644 --- a/src/Symfony/Bridge/Propel1/Tests/Fixtures/ItemQuery.php +++ b/src/Symfony/Bridge/Propel1/Tests/Fixtures/ItemQuery.php @@ -20,12 +20,20 @@ class ItemQuery 'is_active' => \PropelColumnTypes::BOOLEAN, 'enabled' => \PropelColumnTypes::BOOLEAN_EMU, 'updated_at' => \PropelColumnTypes::TIMESTAMP, - - 'updated_at' => \PropelColumnTypes::TIMESTAMP, - 'updated_at' => \PropelColumnTypes::TIMESTAMP, - 'updated_at' => \PropelColumnTypes::TIMESTAMP, ); + public static $result = array(); + + public function find() + { + return self::$result; + } + + public function filterById($id) + { + return $this; + } + public function getTableMap() { // Allows to define methods in this class @@ -37,6 +45,7 @@ public function getPrimaryKeys() { $cm = new \ColumnMap('id', new \TableMap()); $cm->setType('INTEGER'); + $cm->setPhpName('Id'); return array('id' => $cm); } diff --git a/src/Symfony/Bridge/Propel1/Tests/Form/ChoiceList/CompatModelChoiceListTest.php b/src/Symfony/Bridge/Propel1/Tests/Form/ChoiceList/CompatModelChoiceListTest.php new file mode 100644 index 000000000000..c58db19a5ac0 --- /dev/null +++ b/src/Symfony/Bridge/Propel1/Tests/Form/ChoiceList/CompatModelChoiceListTest.php @@ -0,0 +1,128 @@ +query + ->expects($this->once()) + ->method('filterById') + ->with(array(1, 2)) + ->will($this->returnSelf()) + ; + + ItemQuery::$result = array( + $this->item2, + $this->item1, + ); + + parent::testGetChoicesForValues(); + } + + protected function setUp() + { + $this->query = $this->getMock('Symfony\Bridge\Propel1\Tests\Fixtures\ItemQuery', array( + 'filterById', + ), array(), '', true, true, true, false, true); + + $this->createItems(); + + ItemQuery::$result = array( + $this->item1, + $this->item2, + $this->item3, + $this->item4, + ); + + parent::setUp(); + } + + protected function createItems() + { + $this->item1 = new Item(1, 'Foo'); + $this->item2 = new Item(2, 'Bar'); + $this->item3 = new Item(3, 'Baz'); + $this->item4 = new Item(4, 'Cuz'); + } + + protected function createChoiceList() + { + return new ModelChoiceList(self::ITEM_CLASS, 'value', null, $this->query); + } + + protected function getChoices() + { + return array( + 1 => $this->item1, + 2 => $this->item2, + 3 => $this->item3, + 4 => $this->item4, + ); + } + + protected function getLabels() + { + return array( + 1 => 'Foo', + 2 => 'Bar', + 3 => 'Baz', + 4 => 'Cuz', + ); + } + + protected function getValues() + { + return array( + 1 => '1', + 2 => '2', + 3 => '3', + 4 => '4', + ); + } + + protected function getIndices() + { + return array( + 1, + 2, + 3, + 4, + ); + } +} diff --git a/src/Symfony/Bridge/Propel1/Tests/Form/ChoiceList/ModelChoiceListTest.php b/src/Symfony/Bridge/Propel1/Tests/Form/ChoiceList/ModelChoiceListTest.php index f98c71ad4cd4..cf868fee5e12 100644 --- a/src/Symfony/Bridge/Propel1/Tests/Form/ChoiceList/ModelChoiceListTest.php +++ b/src/Symfony/Bridge/Propel1/Tests/Form/ChoiceList/ModelChoiceListTest.php @@ -12,26 +12,34 @@ namespace Symfony\Bridge\Propel1\Tests\Form\ChoiceList; use Symfony\Bridge\Propel1\Form\ChoiceList\ModelChoiceList; -use Symfony\Component\Form\Extension\Core\View\ChoiceView; +use Symfony\Bridge\Propel1\Tests\Propel1TestCase; use Symfony\Bridge\Propel1\Tests\Fixtures\Item; +use Symfony\Bridge\Propel1\Tests\Fixtures\ItemQuery; use Symfony\Bridge\Propel1\Tests\Fixtures\ReadOnlyItem; -use Symfony\Bridge\Propel1\Tests\Propel1TestCase; +use Symfony\Component\Form\Extension\Core\View\ChoiceView; class ModelChoiceListTest extends Propel1TestCase { const ITEM_CLASS = '\Symfony\Bridge\Propel1\Tests\Fixtures\Item'; - protected function setUp() + public static function setUpBeforeClass() { + parent::setUpBeforeClass(); + if (!class_exists('Symfony\Component\Form\Form')) { - $this->markTestSkipped('The "Form" component is not available'); + self::markTestSkipped('The "Form" component is not available'); } if (!class_exists('Symfony\Component\PropertyAccess\PropertyAccessor')) { - $this->markTestSkipped('The "PropertyAccessor" component is not available'); + self::markTestSkipped('The "PropertyAccessor" component is not available'); } } + protected function setUp() + { + ItemQuery::$result = array(); + } + public function testEmptyChoicesReturnsEmpty() { $choiceList = new ModelChoiceList( @@ -173,6 +181,11 @@ public function testGetValuesForChoices() $item1 = new Item(1, 'Foo'); $item2 = new Item(2, 'Bar'); + ItemQuery::$result = array( + $item1, + $item2, + ); + $choiceList = new ModelChoiceList( self::ITEM_CLASS, 'value', @@ -189,6 +202,11 @@ public function testGetValuesForChoices() public function testDifferentEqualObjectsAreChoosen() { $item = new Item(1, 'Foo'); + + ItemQuery::$result = array( + $item, + ); + $choiceList = new ModelChoiceList( self::ITEM_CLASS, 'value', @@ -198,6 +216,7 @@ public function testDifferentEqualObjectsAreChoosen() $choosenItem = new Item(1, 'Foo'); $this->assertEquals(array(1), $choiceList->getIndicesForChoices(array($choosenItem))); + $this->assertEquals(array('1'), $choiceList->getValuesForChoices(array($choosenItem))); } public function testGetIndicesForNullChoices() @@ -211,4 +230,17 @@ public function testGetIndicesForNullChoices() $this->assertEquals(array(), $choiceList->getIndicesForChoices(array(null))); } + + public function testDontAllowInvalidChoiceValues() + { + $item = new Item(1, 'Foo'); + $choiceList = new ModelChoiceList( + self::ITEM_CLASS, + 'value', + array($item) + ); + + $this->assertEquals(array(), $choiceList->getValuesForChoices(array(new Item(2, 'Bar')))); + $this->assertEquals(array(), $choiceList->getChoicesForValues(array(2))); + } } diff --git a/src/Symfony/Bridge/Propel1/Tests/Propel1TestCase.php b/src/Symfony/Bridge/Propel1/Tests/Propel1TestCase.php index 93cc80856519..3a03391aaed5 100644 --- a/src/Symfony/Bridge/Propel1/Tests/Propel1TestCase.php +++ b/src/Symfony/Bridge/Propel1/Tests/Propel1TestCase.php @@ -13,10 +13,10 @@ abstract class Propel1TestCase extends \PHPUnit_Framework_TestCase { - protected function setUp() + public static function setUpBeforeClass() { if (!class_exists('\Propel')) { - $this->markTestSkipped('Propel is not available.'); + self::markTestSkipped('Propel is not available.'); } } }