Merge pull request #2345 from pulzarraider/listmapper_duplicate_exception_improved

Improved ListMapper error handling.

Datagrid/ListMapper.php

@@ -83,14 +83,18 @@ class ListMapper extends BaseMapper
         if ($name instanceof FieldDescriptionInterface) {
             $fieldDescription = $name;
             $fieldDescription->mergeOptions($fieldDescriptionOptions);
-        } elseif (is_string($name) && !$this->admin->hasListFieldDescription($name)) {
+        } elseif (is_string($name)) {
+            if ($this->admin->hasListFieldDescription($name)) {
+                throw new \RuntimeException(sprintf('Duplicate field name "%s" in list mapper. Names should be unique.', $name));
+            }
+
             $fieldDescription = $this->admin->getModelManager()->getNewFieldDescriptionInstance(
                 $this->admin->getClass(),
                 $name,
                 $fieldDescriptionOptions
             );
         } else {
-            throw new \RuntimeException('Unknown or duplicate field name in list mapper. Field name should be either of FieldDescriptionInterface interface or string. Names should be unique.');
+            throw new \RuntimeException('Unknown field name in list mapper. Field name should be either of FieldDescriptionInterface interface or string.');
         }
 
         if (!$fieldDescription->getLabel()) {

Tests/Datagrid/ListMapperTest.php

@@ -34,15 +34,20 @@ class ListMapperTest extends \PHPUnit_Framework_TestCase
      */
     private $fieldDescriptionCollection;
 
+    /**
+     * @var AdminInterface
+     */
+    private $admin;
+
     public function setUp()
     {
         $listBuilder = $this->getMock('Sonata\AdminBundle\Builder\ListBuilderInterface');
         $this->fieldDescriptionCollection = new FieldDescriptionCollection();
-        $admin = $this->getMock('Sonata\AdminBundle\Admin\AdminInterface');
+        $this->admin = $this->getMock('Sonata\AdminBundle\Admin\AdminInterface');
 
         $listBuilder->expects($this->any())
             ->method('addField')
-            ->will($this->returnCallback(function($list, $type, $fieldDescription, $admin) {
+            ->will($this->returnCallback(function ($list, $type, $fieldDescription, $admin) {
                 $list->add($fieldDescription);
             }));
 
@@ -61,17 +66,17 @@ class ListMapperTest extends \PHPUnit_Framework_TestCase
                 return $fieldDescriptionClone;
             }));
 
-        $admin->expects($this->any())
+        $this->admin->expects($this->any())
             ->method('getModelManager')
             ->will($this->returnValue($modelManager));
 
-        $labelTranslatorStrategy = new NoopLabelTranslatorStrategy;
+        $labelTranslatorStrategy = new NoopLabelTranslatorStrategy();
 
-        $admin->expects($this->any())
+        $this->admin->expects($this->any())
             ->method('getLabelTranslatorStrategy')
             ->will($this->returnValue($labelTranslatorStrategy));
 
-        $this->listMapper = new ListMapper($listBuilder, $this->fieldDescriptionCollection, $admin);
+        $this->listMapper = new ListMapper($listBuilder, $this->fieldDescriptionCollection, $this->admin);
     }
 
     public function testFluidInterface()
@@ -127,7 +132,7 @@ class ListMapperTest extends \PHPUnit_Framework_TestCase
     public function testAddViewInlineAction()
     {
         // ignore E_USER_DEPRECATED error
-        $previousErrorHandler = set_error_handler( function() {}, E_USER_DEPRECATED);
+        $previousErrorHandler = set_error_handler( function () {}, E_USER_DEPRECATED);
 
         $this->assertFalse($this->listMapper->has('_action'));
         $this->listMapper->add('_action', 'actions', array('actions'=>array('view'=>array())));
@@ -157,12 +162,38 @@ class ListMapperTest extends \PHPUnit_Framework_TestCase
         $this->assertFalse($this->listMapper->has('fooName'));
     }
 
-    public function testAddException()
+    public function testAddDuplicateNameException()
+    {
+        $tmpNames = array();
+        $this->admin->expects($this->any())
+            ->method('hasListFieldDescription')
+            ->will($this->returnCallback(function ($name) use (&$tmpNames) {
+                if (isset($tmpNames[$name])) {
+                    return true;
+                }
+                $tmpNames[$name] = $name;
+
+                return false;
+            }));
+
+        try {
+            $this->listMapper->add('fooName');
+            $this->listMapper->add('fooName');
+        } catch (\RuntimeException $e) {
+            $this->assertContains('Duplicate field name "fooName" in list mapper. Names should be unique.', $e->getMessage());
+
+            return;
+        }
+
+        $this->fail('Failed asserting that exception of type "\RuntimeException" is thrown.');
+    }
+
+    public function testAddWrongTypeException()
     {
         try {
             $this->listMapper->add(12345);
         } catch (\RuntimeException $e) {
-            $this->assertContains('Unknown or duplicate field name in list mapper. Field name should be either of FieldDescriptionInterface interface or string. Names should be unique.', $e->getMessage());
+            $this->assertContains('Unknown field name in list mapper. Field name should be either of FieldDescriptionInterface interface or string.', $e->getMessage());
 
             return;
         }