Ensure boolean values for "identifier" option

When passed to ListMapper::add(), the "identifier" should be a boolean. Closes sonata-project#4554
BitScout · Jun 22, 2019 · 34a4ef1 · 34a4ef1
1 parent 4a79a21
commit 34a4ef1
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 2 deletions.
diff --git a/src/Datagrid/ListMapper.php b/src/Datagrid/ListMapper.php
@@ -92,6 +92,17 @@ public function add($name, $type = null, array $fieldDescriptionOptions = [])
             $fieldDescriptionOptions['virtual_field'] = true;
         }
 
+        if (\array_key_exists('identifier', $fieldDescriptionOptions) && !\is_bool($fieldDescriptionOptions['identifier'])) {
+            @trigger_error(
+                'Passing a non boolean value for the "identifier" option is deprecated since sonata-project/admin-bundle 3.x and will throw an exception in 4.0.',
+                E_USER_DEPRECATED
+            );
+
+            $fieldDescriptionOptions['identifier'] = (bool) $fieldDescriptionOptions['identifier'];
+            // NEXT_MAJOR: Remove the previous 6 lines and use commented line below it instead
+            // throw new \InvalidArgumentException(sprintf('Value for "identifier" option must be boolean, %s given.', gettype($fieldDescriptionOptions['identifier'])));
+        }
+
         if ($name instanceof FieldDescriptionInterface) {
             $fieldDescription = $name;
             $fieldDescription->mergeOptions($fieldDescriptionOptions);

diff --git a/src/Resources/views/CRUD/base_list_field.html.twig b/src/Resources/views/CRUD/base_list_field.html.twig
@@ -13,7 +13,7 @@ file that was distributed with this source code.
     {% set route = field_description.options.route.name|default(null) %}
 
     {% if
-        field_description.options.identifier is defined
+        field_description.options.identifier|default(false)
         and route
         and admin.hasRoute(route)
         and admin.hasAccess(route, route in ['show', 'edit'] ? object : null)

diff --git a/src/Resources/views/CRUD/base_list_flat_field.html.twig b/src/Resources/views/CRUD/base_list_flat_field.html.twig
@@ -11,7 +11,7 @@ file that was distributed with this source code.
 
 <span class="sonata-ba-list-field sonata-ba-list-field-{{ field_description.type }}" objectId="{{ admin.id(object) }}">
     {% if
-            field_description.options.identifier is defined
+        field_description.options.identifier|default(false)
         and field_description.options.route is defined
         and admin.hasAccess(field_description.options.route.name, object)
         and admin.hasRoute(field_description.options.route.name)

diff --git a/tests/Datagrid/ListMapperTest.php b/tests/Datagrid/ListMapperTest.php
@@ -116,6 +116,71 @@ public function testAddIdentifier(): void
 
         $this->listMapper->addIdentifier($fieldDescription);
         $this->assertTrue($this->listMapper->has('fooName'));
+
+        $fieldDescription = $this->listMapper->get('fooName');
+        $this->assertTrue($fieldDescription->getOption('identifier'));
+    }
+
+    public function testAddOptionIdentifier(): void
+    {
+        $this->assertFalse($this->listMapper->has('fooName'));
+        $this->assertFalse($this->listMapper->has('barName'));
+        $this->assertFalse($this->listMapper->has('bazName'));
+
+        $this->listMapper->add('barName');
+        $this->assertNull($this->listMapper->get('barName')->getOption('identifier'));
+        $this->listMapper->add('fooName', null, ['identifier' => true]);
+        $this->assertTrue($this->listMapper->has('fooName'));
+        $this->assertTrue($this->listMapper->get('fooName')->getOption('identifier'));
+        $this->listMapper->add('bazName', null, ['identifier' => false]);
+        $this->assertTrue($this->listMapper->has('bazName'));
+        $this->assertFalse($this->listMapper->get('bazName')->getOption('identifier'));
+    }
+
+    /**
+     * @group legacy
+     *
+     * @expectedDeprecation Passing a non boolean value for the "identifier" option is deprecated since sonata-project/admin-bundle 3.x and will throw an exception in 4.0.
+     *
+     * @dataProvider getWrongIdentifierOptions
+     */
+    public function testAddOptionIdentifierWithDeprecatedValue(bool $expected, $value): void
+    {
+        $this->assertFalse($this->listMapper->has('fooName'));
+        $this->listMapper->add('fooName', null, ['identifier' => $value]);
+        $this->assertTrue($this->listMapper->has('fooName'));
+        $this->assertSame($expected, $this->listMapper->get('fooName')->getOption('identifier'));
+    }
+
+    /**
+     * @dataProvider getWrongIdentifierOptions
+     */
+    public function testAddOptionIdentifierWithWrongValue(bool $expected, $value): void
+    {
+        // NEXT_MAJOR: Remove the following `markTestSkipped()` call and the `testAddOptionIdentifierWithDeprecatedValue()` test
+        $this->markTestSkipped('This test must be run in 4.0');
+
+        $this->assertFalse($this->listMapper->has('fooName'));
+
+        $this->expectException(\InvalidArgumentException::class);
+        $this->expectExceptionMessageRegExp('{^Value for "identifier" option must be boolean, [^]+ given.$}');
+
+        $this->listMapper->add('fooName', null, ['identifier' => $value]);
+    }
+
+    public function getWrongIdentifierOptions(): iterable
+    {
+        return [
+            [true, 1],
+            [true, 'string'],
+            [true, new \stdClass()],
+            [true, [null]],
+            [false, 0],
+            [false, null],
+            [false, ''],
+            [false, '0'],
+            [false, []],
+        ];
     }
 
     public function testAdd(): void