Fix support for composite primary key in AbstractAdmin::getSubject (#3873)

Admin/AbstractAdmin.php

@@ -1627,11 +1627,7 @@ abstract class AbstractAdmin implements AdminInterface, DomainObjectInterface
     {
         if ($this->subject === null && $this->request) {
             $id = $this->request->get($this->getIdParameter());
-            if (!preg_match('#^[0-9A-Fa-f\-]+$#', $id)) {
-                $this->subject = false;
-            } else {
-                $this->subject = $this->getModelManager()->find($this->class, $id);
-            }
+            $this->subject = $this->getModelManager()->find($this->class, $id);
         }
 
         return $this->subject;

Form/ChoiceList/ModelChoiceLoader.php

@@ -13,6 +13,7 @@ namespace Sonata\AdminBundle\Form\ChoiceList;
 
 use Doctrine\Common\Util\ClassUtils;
 use Sonata\AdminBundle\Model\ModelManagerInterface;
+use Sonata\CoreBundle\Model\Adapter\AdapterInterface;
 use Symfony\Component\Form\ChoiceList\ArrayChoiceList;
 use Symfony\Component\Form\ChoiceList\Loader\ChoiceLoaderInterface;
 use Symfony\Component\Form\Exception\RuntimeException;
@@ -109,7 +110,7 @@ class ModelChoiceLoader implements ChoiceLoaderInterface
                     }
                 }
 
-                $id = implode('~', $this->getIdentifierValues($entity));
+                $id = implode(AdapterInterface::ID_SEPARATOR, $this->getIdentifierValues($entity));
 
                 if (!array_key_exists($valueObject, $choices)) {
                     $choices[$valueObject] = array();

Form/DataTransformer/ModelsToArrayTransformer.php

@@ -14,6 +14,7 @@ namespace Sonata\AdminBundle\Form\DataTransformer;
 use Sonata\AdminBundle\Form\ChoiceList\ModelChoiceList;
 use Sonata\AdminBundle\Form\ChoiceList\ModelChoiceLoader;
 use Sonata\AdminBundle\Model\ModelManagerInterface;
+use Sonata\CoreBundle\Model\Adapter\AdapterInterface;
 use Symfony\Component\Form\ChoiceList\LazyChoiceList;
 use Symfony\Component\Form\DataTransformerInterface;
 use Symfony\Component\Form\Exception\RuntimeException;
@@ -76,7 +77,7 @@ class ModelsToArrayTransformer implements DataTransformerInterface
 
         $array = array();
         foreach ($collection as $key => $entity) {
-            $id = implode('~', $this->getIdentifierValues($entity));
+            $id = implode(AdapterInterface::ID_SEPARATOR, $this->getIdentifierValues($entity));
 
             $array[] = $id;
         }

Tests/Admin/AdminTest.php

@@ -26,6 +26,7 @@ use Sonata\AdminBundle\Tests\Fixtures\Bundle\Entity\Post;
 use Sonata\AdminBundle\Tests\Fixtures\Bundle\Entity\Tag;
 use Sonata\AdminBundle\Tests\Fixtures\Entity\FooToString;
 use Sonata\AdminBundle\Tests\Fixtures\Entity\FooToStringNull;
+use Sonata\CoreBundle\Model\Adapter\AdapterInterface;
 use Symfony\Component\DependencyInjection\Container;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\PropertyAccess\PropertyAccess;
@@ -1512,31 +1513,72 @@ class AdminTest extends \PHPUnit_Framework_TestCase
         $this->assertSame($bazFieldDescription, $modelAdmin->getFilterFieldDescription('baz'));
     }
 
-    public function testGetSubject()
+    public function testGetSubjectNoRequest()
+    {
+        $modelManager = $this->getMock('Sonata\AdminBundle\Model\ModelManagerInterface');
+        $modelManager
+            ->expects($this->never())
+            ->method('find');
+
+        $admin = new PostAdmin('sonata.post.admin.post', 'NewsBundle\Entity\Post', 'SonataNewsBundle:PostAdmin');
+        $admin->setModelManager($modelManager);
+
+        $this->assertNull($admin->getSubject());
+    }
+
+    /**
+     * @return array
+     */
+    public function provideGetSubject()
+    {
+        return array(
+            array(23),
+            array('azerty'),
+            array('4f69bbb5f14a13347f000092'),
+            array('0779ca8d-e2be-11e4-ac58-0242ac11000b'),
+            array('123'.AdapterInterface::ID_SEPARATOR.'my_type'), // composite keys are supported
+        );
+    }
+
+    /**
+     * @dataProvider provideGetSubject
+     */
+    public function testGetSubjectFailed($id)
     {
-        $entity = new Post();
         $modelManager = $this->getMock('Sonata\AdminBundle\Model\ModelManagerInterface');
         $modelManager
-            ->expects($this->any())
+            ->expects($this->once())
             ->method('find')
-            ->will($this->returnValue($entity));
+            ->with('NewsBundle\Entity\Post', $id)
+            ->will($this->returnValue(null)); // entity not found
+
         $admin = new PostAdmin('sonata.post.admin.post', 'NewsBundle\Entity\Post', 'SonataNewsBundle:PostAdmin');
         $admin->setModelManager($modelManager);
 
-        $admin->setRequest(new Request(array('id' => 'azerty')));
-        $this->assertFalse($admin->getSubject());
+        $admin->setRequest(new Request(array('id' => $id)));
+        $this->assertNull($admin->getSubject());
+    }
 
-        $admin->setSubject(null);
-        $admin->setRequest(new Request(array('id' => 42)));
-        $this->assertSame($entity, $admin->getSubject());
+    /**
+     * @dataProvider provideGetSubject
+     */
+    public function testGetSubject($id)
+    {
+        $entity = new Post();
 
-        $admin->setSubject(null);
-        $admin->setRequest(new Request(array('id' => '4f69bbb5f14a13347f000092')));
-        $this->assertSame($entity, $admin->getSubject());
+        $modelManager = $this->getMock('Sonata\AdminBundle\Model\ModelManagerInterface');
+        $modelManager
+            ->expects($this->once())
+            ->method('find')
+            ->with('NewsBundle\Entity\Post', $id)
+            ->will($this->returnValue($entity));
 
-        $admin->setSubject(null);
-        $admin->setRequest(new Request(array('id' => '0779ca8d-e2be-11e4-ac58-0242ac11000b')));
+        $admin = new PostAdmin('sonata.post.admin.post', 'NewsBundle\Entity\Post', 'SonataNewsBundle:PostAdmin');
+        $admin->setModelManager($modelManager);
+
+        $admin->setRequest(new Request(array('id' => $id)));
         $this->assertSame($entity, $admin->getSubject());
+        $this->assertSame($entity, $admin->getSubject()); // model manager must be used only once
     }
 
     /**

UPGRADE-3.x.md

@@ -7,6 +7,11 @@ Since `AbstractAdmin::configureBatchActions` is present, you should not override
 
 This method will be final in 4.0.
 
+## Backward compatibility break for AbstractAdmin::getSubject()
+
+Now `AbstractAdmin::getSubject()` return `null` or `object` of subject entity. Previously,
+`AbstractAdmin::getSubject()` may return `false` if entity identifier not match regexp `/^[0-9A-Fa-f\-]+$/`.
+
 UPGRADE FROM 3.0 to 3.1
 =======================