bug #34384 [DoctrineBridge] Improve queries parameters display in Profiler (fancyweb)

This PR was merged into the 4.4 branch.

Discussion
----------

[DoctrineBridge] Improve queries parameters display in Profiler

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | https://github.com/symfony/symfony/issues/34234
| License       | MIT
| Doc PR        | -

##  Stringable object:
_The query is runnable._
### Before
<img width="773" alt="Screenshot 2019-11-14 at 21 05 53" src="https://user-images.githubusercontent.com/3658119/68892948-669cf180-0724-11ea-889b-8ce781956fc7.png">

### After
<img width="780" alt="Screenshot 2019-11-14 at 20 56 38" src="https://user-images.githubusercontent.com/3658119/68892978-787e9480-0724-11ea-843b-70a595633192.png">

## Non stringable object:
_Exception, the query is not runnable._
### Before
<img width="783" alt="Screenshot 2019-11-14 at 21 05 31" src="https://user-images.githubusercontent.com/3658119/68892993-80d6cf80-0724-11ea-9961-e32b65f81508.png">

### After
<img width="622" alt="Screenshot 2019-11-14 at 20 57 17" src="https://user-images.githubusercontent.com/3658119/68893007-87fddd80-0724-11ea-98cb-2ef695135673.png">

## Error with an object:
_`ConversionException` or a `\TypeError` when you specify the type when you set the parameter but you provide an invalid value and this value is an object (eg `->setParameter('foo', new \stdClass(), 'date')`), the query is not runnable._
### Before
<img width="775" alt="Screenshot 2019-11-14 at 21 04 45" src="https://user-images.githubusercontent.com/3658119/68893078-a9f76000-0724-11ea-9e9a-bb806ffa0a73.png">

### After
<img width="821" alt="Screenshot 2019-11-14 at 20 59 23" src="https://user-images.githubusercontent.com/3658119/68893098-b24f9b00-0724-11ea-8e9d-7179725b8bc1.png">

## Error with anything else:
_`ConversionException` or a `\TypeError` when you specify the type when you set the parameter but you provide an invalid value and this value is not an object (eg `->setParameter('foo', 'bar', 'date')`), the query is not runnable._
### Before
<img width="809" alt="Screenshot 2019-11-14 at 21 05 10" src="https://user-images.githubusercontent.com/3658119/68893031-93e99f80-0724-11ea-9c23-f8d05f2dbfbb.png">

### After
<img width="832" alt="Screenshot 2019-11-14 at 20 57 54" src="https://user-images.githubusercontent.com/3658119/68893055-9d730780-0724-11ea-8ce1-a7b8946aaf94.png">

The new `runnable` key will be used in `DoctrineBundle` profiler template to hide the `View runnable query` link when the query is not runnable.

Commits
-------

fe15f51d4d [DoctrineBridge] Improve queries parameters display in Profiler

src/Symfony/Bridge/Doctrine/DataCollector/DoctrineDataCollector.php

@@ -18,6 +18,8 @@ use Doctrine\DBAL\Types\Type;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpKernel\DataCollector\DataCollector;
+use Symfony\Component\VarDumper\Caster\Caster;
+use Symfony\Component\VarDumper\Cloner\Stub;
 
 /**
  * DoctrineDataCollector.
@@ -121,6 +123,38 @@ class DoctrineDataCollector extends DataCollector
         return 'db';
     }
 
+    /**
+     * {@inheritdoc}
+     */
+    protected function getCasters()
+    {
+        return parent::getCasters() + [
+            ObjectParameter::class => static function (ObjectParameter $o, array $a, Stub $s): array {
+                $s->class = $o->getClass();
+                $s->value = $o->getObject();
+
+                $r = new \ReflectionClass($o->getClass());
+                if ($f = $r->getFileName()) {
+                    $s->attr['file'] = $f;
+                    $s->attr['line'] = $r->getStartLine();
+                } else {
+                    unset($s->attr['file']);
+                    unset($s->attr['line']);
+                }
+
+                if ($error = $o->getError()) {
+                    return [Caster::PREFIX_VIRTUAL.'⚠' => $error->getMessage()];
+                }
+
+                if ($o->isStringable()) {
+                    return [Caster::PREFIX_VIRTUAL.'__toString()' => (string) $o->getObject()];
+                }
+
+                return [Caster::PREFIX_VIRTUAL.'⚠' => sprintf('Object of class "%s" could not be converted to string.', $o->getClass())];
+            },
+        ];
+    }
+
     private function sanitizeQueries(string $connectionName, array $queries): array
     {
         foreach ($queries as $i => $query) {
@@ -133,6 +167,7 @@ class DoctrineDataCollector extends DataCollector
     private function sanitizeQuery(string $connectionName, array $query): array
     {
         $query['explainable'] = true;
+        $query['runnable'] = true;
         if (null === $query['params']) {
             $query['params'] = [];
         }
@@ -143,6 +178,7 @@ class DoctrineDataCollector extends DataCollector
             $query['types'] = [];
         }
         foreach ($query['params'] as $j => $param) {
+            $e = null;
             if (isset($query['types'][$j])) {
                 // Transform the param according to the type
                 $type = $query['types'][$j];
@@ -154,18 +190,19 @@ class DoctrineDataCollector extends DataCollector
                     try {
                         $param = $type->convertToDatabaseValue($param, $this->registry->getConnection($connectionName)->getDatabasePlatform());
                     } catch (\TypeError $e) {
-                        // Error thrown while processing params, query is not explainable.
-                        $query['explainable'] = false;
                     } catch (ConversionException $e) {
-                        $query['explainable'] = false;
                     }
                 }
             }
 
-            list($query['params'][$j], $explainable) = $this->sanitizeParam($param);
+            list($query['params'][$j], $explainable, $runnable) = $this->sanitizeParam($param, $e);
             if (!$explainable) {
                 $query['explainable'] = false;
             }
+
+            if (!$runnable) {
+                $query['runnable'] = false;
+            }
         }
 
         $query['params'] = $this->cloneVar($query['params']);
@@ -180,32 +217,33 @@ class DoctrineDataCollector extends DataCollector
      * indicating if the original value was kept (allowing to use the sanitized
      * value to explain the query).
      */
-    private function sanitizeParam($var): array
+    private function sanitizeParam($var, ?\Throwable $error): array
     {
         if (\is_object($var)) {
-            $className = \get_class($var);
+            return [$o = new ObjectParameter($var, $error), false, $o->isStringable() && !$error];
+        }
 
-            return method_exists($var, '__toString') ?
+        if ($error) {
-                [sprintf('/* Object(%s): */"%s"', $className, $var->__toString()), false] :
+            return ['⚠ '.$error->getMessage(), false, false];
-                [sprintf('/* Object(%s) */', $className), false];
         }
 
         if (\is_array($var)) {
             $a = [];
-            $original = true;
+            $explainable = $runnable = true;
             foreach ($var as $k => $v) {
-                list($value, $orig) = $this->sanitizeParam($v);
+                list($value, $e, $r) = $this->sanitizeParam($v, null);
-                $original = $original && $orig;
+                $explainable = $explainable && $e;
+                $runnable = $runnable && $r;
                 $a[$k] = $value;
             }
 
-            return [$a, $original];
+            return [$a, $explainable, $runnable];
         }
 
         if (\is_resource($var)) {
-            return [sprintf('/* Resource(%s) */', get_resource_type($var)), false];
+            return [sprintf('/* Resource(%s) */', get_resource_type($var)), false, false];
         }
 
-        return [$var, true];
+        return [$var, true, true];
     }
 }

src/Symfony/Bridge/Doctrine/DataCollector/ObjectParameter.php

@@ -0,0 +1,54 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bridge\Doctrine\DataCollector;
+
+final class ObjectParameter
+{
+    private $object;
+    private $error;
+    private $stringable;
+    private $class;
+
+    /**
+     * @param object $object
+     */
+    public function __construct($object, ?\Throwable $error)
+    {
+        $this->object = $object;
+        $this->error = $error;
+        $this->stringable = \is_callable([$object, '__toString']);
+        $this->class = \get_class($object);
+    }
+
+    /**
+     * @return object
+     */
+    public function getObject()
+    {
+        return $this->object;
+    }
+
+    public function getError(): ?\Throwable
+    {
+        return $this->error;
+    }
+
+    public function isStringable(): bool
+    {
+        return $this->stringable;
+    }
+
+    public function getClass(): string
+    {
+        return $this->class;
+    }
+}

src/Symfony/Bridge/Doctrine/Tests/DataCollector/DoctrineDataCollectorTest.php

@@ -18,6 +18,7 @@ use Symfony\Bridge\Doctrine\DataCollector\DoctrineDataCollector;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\VarDumper\Cloner\Data;
+use Symfony\Component\VarDumper\Dumper\CliDumper;
 
 class DoctrineDataCollectorTest extends TestCase
 {
@@ -74,7 +75,7 @@ class DoctrineDataCollectorTest extends TestCase
     /**
      * @dataProvider paramProvider
      */
-    public function testCollectQueries($param, $types, $expected, $explainable)
+    public function testCollectQueries($param, $types, $expected, $explainable, bool $runnable = true)
     {
         $queries = [
             ['sql' => 'SELECT * FROM table1 WHERE field1 = ?1', 'params' => [$param], 'types' => $types, 'executionMS' => 1],
@@ -83,8 +84,19 @@ class DoctrineDataCollectorTest extends TestCase
         $c->collect(new Request(), new Response());
 
         $collectedQueries = $c->getQueries();
-        $this->assertEquals($expected, $collectedQueries['default'][0]['params'][0]);
+
+        $collectedParam = $collectedQueries['default'][0]['params'][0];
+        if ($collectedParam instanceof Data) {
+            $dumper = new CliDumper($out = fopen('php://memory', 'r+b'));
+            $dumper->setColors(false);
+            $collectedParam->dump($dumper);
+            $this->assertStringMatchesFormat($expected, print_r(stream_get_contents($out, -1, 0), true));
+        } else {
+            $this->assertEquals($expected, $collectedParam);
+        }
+
         $this->assertEquals($explainable, $collectedQueries['default'][0]['explainable']);
+        $this->assertSame($runnable, $collectedQueries['default'][0]['runnable']);
     }
 
     public function testCollectQueryWithNoParams()
@@ -100,9 +112,11 @@ class DoctrineDataCollectorTest extends TestCase
         $this->assertInstanceOf(Data::class, $collectedQueries['default'][0]['params']);
         $this->assertEquals([], $collectedQueries['default'][0]['params']->getValue());
         $this->assertTrue($collectedQueries['default'][0]['explainable']);
+        $this->assertTrue($collectedQueries['default'][0]['runnable']);
         $this->assertInstanceOf(Data::class, $collectedQueries['default'][1]['params']);
         $this->assertEquals([], $collectedQueries['default'][1]['params']->getValue());
         $this->assertTrue($collectedQueries['default'][1]['explainable']);
+        $this->assertTrue($collectedQueries['default'][1]['runnable']);
     }
 
     public function testCollectQueryWithNoTypes()
@@ -134,7 +148,7 @@ class DoctrineDataCollectorTest extends TestCase
     /**
      * @dataProvider paramProvider
      */
-    public function testSerialization($param, $types, $expected, $explainable)
+    public function testSerialization($param, $types, $expected, $explainable, bool $runnable = true)
     {
         $queries = [
             ['sql' => 'SELECT * FROM table1 WHERE field1 = ?1', 'params' => [$param], 'types' => $types, 'executionMS' => 1],
@@ -144,8 +158,19 @@ class DoctrineDataCollectorTest extends TestCase
         $c = unserialize(serialize($c));
 
         $collectedQueries = $c->getQueries();
-        $this->assertEquals($expected, $collectedQueries['default'][0]['params'][0]);
+
+        $collectedParam = $collectedQueries['default'][0]['params'][0];
+        if ($collectedParam instanceof Data) {
+            $dumper = new CliDumper($out = fopen('php://memory', 'r+b'));
+            $dumper->setColors(false);
+            $collectedParam->dump($dumper);
+            $this->assertStringMatchesFormat($expected, print_r(stream_get_contents($out, -1, 0), true));
+        } else {
+            $this->assertEquals($expected, $collectedParam);
+        }
+
         $this->assertEquals($explainable, $collectedQueries['default'][0]['explainable']);
+        $this->assertSame($runnable, $collectedQueries['default'][0]['runnable']);
     }
 
     public function paramProvider()
@@ -156,19 +181,46 @@ class DoctrineDataCollectorTest extends TestCase
             [true, [], true, true],
             [null, [], null, true],
             [new \DateTime('2011-09-11'), ['date'], '2011-09-11', true],
-            [fopen(__FILE__, 'r'), [], '/* Resource(stream) */', false],
+            [fopen(__FILE__, 'r'), [], '/* Resource(stream) */', false, false],
-            [new \stdClass(), [], '/* Object(stdClass) */', false],
+            [
+                new \stdClass(),
+                [],
+                <<<EOTXT
+{#%d
+  ⚠: "Object of class "stdClass" could not be converted to string."
+}
+EOTXT
+                ,
+                false,
+                false,
+            ],
             [
                 new StringRepresentableClass(),
                 [],
-                '/* Object(Symfony\Bridge\Doctrine\Tests\DataCollector\StringRepresentableClass): */"string representation"',
+                <<<EOTXT
+Symfony\Bridge\Doctrine\Tests\DataCollector\StringRepresentableClass {#%d
+  __toString(): "string representation"
+}
+EOTXT
+                ,
                 false,
             ],
         ];
 
         if (version_compare(Version::VERSION, '2.6', '>=')) {
-            $tests[] = ['this is not a date', ['date'], 'this is not a date', false];
+            $tests[] = ['this is not a date', ['date'], "⚠ Could not convert PHP value 'this is not a date' of type 'string' to type 'date'. Expected one of the following types: null, DateTime", false, false];
-            $tests[] = [new \stdClass(), ['date'], '/* Object(stdClass) */', false];
+            $tests[] = [
+                new \stdClass(),
+                ['date'],
+                <<<EOTXT
+{#%d
+  ⚠: "Could not convert PHP value of type 'stdClass' to type 'date'. Expected one of the following types: null, DateTime"
+}
+EOTXT
+                ,
+                false,
+                false,
+            ];
         }
 
         return $tests;