merged branch aboks/doctrine_data_collector (PR #2733)

Commits
-------

bb0d202 Switched sanitizeParameter() for existing varToString()-method; now always stores a string representation of each parameter
4fe4dfd Fixed vendor version mismatch in tests
28730e9 [DoctrineBridge] Added unit tests
4535abe [DoctrineBridge] Fixed attempt to serialize non-serializable values

Discussion
----------

[DoctrineBridge] Fixed attempt to serialize non-serializable values

Bug fix: yes
Feature addition: no
Backwards compatibility break: no (99%)
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -

The Doctrine DBAL type system does not pose any restrictions on the php-types of parameters in queries. Hence one could write a doctrine-type that uses a resource or an `\SplFileInfo` as its corresponding php-type. Parameters of these types are logged in the `DoctrineDataCollector` however, which is then serialized in the profiler. Since resources or `\SplFileInfo` variables cannot be serialized this throws an exception.

This PR fixes this problem (for known cases) by sanitizing the query parameters to only contain serializable types. The `isNotSerializable`-check surely is not complete yet, but more non-serializable classes can be added on a case-by-case basis.

---------------------------------------------------------------------------

by fabpot at 2011/12/07 07:04:43 -0800

Tests do not pass for me.

Furthermore, let's reuse what we already have in the framework (see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/HttpKernel.php#L187 -- yes you can just copy/paster the existing code).

---------------------------------------------------------------------------

by aboks at 2011/12/09 01:41:14 -0800

@fabpot I fixed the tests (seems I had the wrong vendor versions in my copy) and reused the `varToString()`-code. This introduces a tiny BC break in the rare case that someone writes his own templates for the web profiler (the parameters returned by the data collector are now always a string; could be any type before).

After merging this PR, merging 2.0 into master would give a merge conflict and failing tests (because of the changes related to the introduction of the `ManagerRegistry` interface). To prevent this, please merge #2820 into master directly after merging this PR (so before merging 2.0 into master). After that 2.0 can be cleanly merged into master.

---------------------------------------------------------------------------

by stof at 2011/12/09 03:43:38 -0800

it is not a BC break. Using ``yaml_encode`` on a string will not break the template

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

@@ -41,7 +41,7 @@ class DoctrineDataCollector extends DataCollector
     public function collect(Request $request, Response $response, \Exception $exception = null)
     {
         $this->data = array(
-            'queries'     => null !== $this->logger ? $this->logger->queries : array(),
+            'queries'     => null !== $this->logger ? $this->sanitizeQueries($this->logger->queries) : array(),
             'connections' => $this->connections,
             'managers'    => $this->managers,
         );
@@ -85,4 +85,48 @@ class DoctrineDataCollector extends DataCollector
         return 'db';
     }
 
+    private function sanitizeQueries($queries)
+    {
+        foreach ($queries as $i => $query) {
+            foreach ($query['params'] as $j => $param) {
+                $queries[$i]['params'][$j] = $this->varToString($param);
+            }
+        }
+
+        return $queries;
+    }
+
+    private function varToString($var)
+    {
+        if (is_object($var)) {
+            return sprintf('Object(%s)', get_class($var));
+        }
+
+        if (is_array($var)) {
+            $a = array();
+            foreach ($var as $k => $v) {
+                $a[] = sprintf('%s => %s', $k, $this->varToString($v));
+            }
+
+            return sprintf("Array(%s)", implode(', ', $a));
+        }
+
+        if (is_resource($var)) {
+            return sprintf('Resource(%s)', get_resource_type($var));
+        }
+
+        if (null === $var) {
+            return 'null';
+        }
+
+        if (false === $var) {
+            return 'false';
+        }
+
+        if (true === $var) {
+            return 'true';
+        }
+
+        return (string) $var;
+    }
 }

src/Symfony/Bundle/DoctrineBundle/Resources/views/Collector/db.html.twig

@@ -36,7 +36,7 @@
                         <code>{{ query.sql }}</code>
                     </div>
                     <small>
-                        <strong>Parameters</strong>: {{ query.params|yaml_encode }}<br />
+                        <strong>Parameters</strong>: {{ query.params }}<br />
                         <strong>Time</strong>: {{ '%0.2f'|format(query.executionMS * 1000) }} ms
                     </small>
                 </li>

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

@@ -0,0 +1,131 @@
+<?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\Tests\Bridge\Doctrine\DataCollector;
+
+use Symfony\Bridge\Doctrine\DataCollector\DoctrineDataCollector;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
+
+class DoctrineDataCollectorTest extends \PHPUnit_Framework_TestCase
+{
+    public function testCollectConnections()
+    {
+        $c = $this->createCollector(array());
+        $c->collect(new Request(), new Response());
+        $this->assertEquals(array('default' => 'doctrine.dbal.default_connection'), $c->getConnections());
+    }
+
+    public function testCollectManagers()
+    {
+        $c = $this->createCollector(array());
+        $c->collect(new Request(), new Response());
+        $this->assertEquals(array('default' => 'doctrine.orm.default_entity_manager'), $c->getManagers());
+    }
+
+    public function testCollectQueryCount()
+    {
+        $c = $this->createCollector(array());
+        $c->collect(new Request(), new Response());
+        $this->assertEquals(0, $c->getQueryCount());
+
+        $queries = array(
+            array('sql' => "SELECT * FROM table1", 'params' => array(), 'types' => array(), 'executionMS' => 0)
+        );
+        $c = $this->createCollector($queries);
+        $c->collect(new Request(), new Response());
+        $this->assertEquals(1, $c->getQueryCount());
+    }
+
+    public function testCollectTime()
+    {
+        $c = $this->createCollector(array());
+        $c->collect(new Request(), new Response());
+        $this->assertEquals(0, $c->getTime());
+
+        $queries = array(
+            array('sql' => "SELECT * FROM table1", 'params' => array(), 'types' => array(), 'executionMS' => 1)
+        );
+        $c = $this->createCollector($queries);
+        $c->collect(new Request(), new Response());
+        $this->assertEquals(1, $c->getTime());
+
+        $queries = array(
+            array('sql' => "SELECT * FROM table1", 'params' => array(), 'types' => array(), 'executionMS' => 1),
+            array('sql' => "SELECT * FROM table2", 'params' => array(), 'types' => array(), 'executionMS' => 2)
+        );
+        $c = $this->createCollector($queries);
+        $c->collect(new Request(), new Response());
+        $this->assertEquals(3, $c->getTime());
+    }
+
+    /**
+     * @dataProvider paramProvider
+     */
+    public function testCollectQueries($param, $expected)
+    {
+        $queries = array(
+            array('sql' => "SELECT * FROM table1 WHERE field1 = ?1", 'params' => array($param), 'types' => array(), 'executionMS' => 1)
+        );
+        $c = $this->createCollector($queries);
+        $c->collect(new Request(), new Response());
+
+        $collected_queries = $c->getQueries();
+        $this->assertEquals($expected, $collected_queries[0]['params'][0]);
+    }
+
+    /**
+     * @dataProvider paramProvider
+     */
+    public function testSerialization($param, $expected)
+    {
+        $queries = array(
+            array('sql' => "SELECT * FROM table1 WHERE field1 = ?1", 'params' => array($param), 'types' => array(), 'executionMS' => 1)
+        );
+        $c = $this->createCollector($queries);
+        $c->collect(new Request(), new Response());
+        $c = unserialize(serialize($c));
+
+        $collected_queries = $c->getQueries();
+        $this->assertEquals($expected, $collected_queries[0]['params'][0]);
+    }
+
+    public function paramProvider()
+    {
+        return array(
+            array('some value', 'some value'),
+            array(1, '1'),
+            array(true, 'true'),
+            array(null, 'null'),
+            array(new \stdClass(), 'Object(stdClass)'),
+            array(fopen(__FILE__, 'r'), 'Resource(stream)'),
+            array(new \SplFileInfo(__FILE__), 'Object(SplFileInfo)'),
+        );
+    }
+
+    private function createCollector($queries)
+    {
+        $registry = $this->getMock('Symfony\Bridge\Doctrine\RegistryInterface');
+        $registry
+                ->expects($this->any())
+                ->method('getConnectionNames')
+                ->will($this->returnValue(array('default' => 'doctrine.dbal.default_connection')));
+        $registry
+                ->expects($this->any())
+                ->method('getEntityManagerNames')
+                ->will($this->returnValue(array('default' => 'doctrine.orm.default_entity_manager')));
+
+        $logger = $this->getMock('Symfony\Bridge\Doctrine\Logger\DbalLogger');
+        $logger->queries = $queries;
+
+        return new DoctrineDataCollector($registry, $logger);
+    }
+}