Added improvements for the DB panel #117

Merged
merged 1 commit into from Nov 10, 2012

Projects

None yet

4 participants

@dlsniper

Hi,

After seeing #107 I've decided to change the DB panel a bit.

You can see it in action here: http://sf2demo.rodb.ro/app_dev.php/

It's using https://github.com/jdorn/sql-formatter , thanks goes to @jdorn as a dependency.

Feedback is most welcomed!

Best regards.

@stof stof commented on an outdated diff Sep 14, 2012
DependencyInjection/DoctrineExtension.php
@@ -50,6 +50,9 @@ public function load(array $configs, ContainerBuilder $container)
if (!empty($config['orm'])) {
$this->ormLoad($config['orm'], $container);
}
+ $definition = new Definition('Doctrine\Bundle\DoctrineBundle\Extension\DoctrineBundleExtension');
+ $definition->addTag('twig.extension');
+ $container->setDefinition('doctrine_bundle_extension', $definition);
@stof
stof Sep 14, 2012

why not definining this in the XM file ?

@stof stof commented on an outdated diff Sep 14, 2012
Extension/DoctrineBundleExtension.php
@@ -0,0 +1,105 @@
+<?php
+
+/*namespace
+{
+ require_once '/var/www/forks/sql-formatter/lib/SqlFormatter.php';
+}*/
@stof
stof Sep 14, 2012

this must be removed, and the license header must be added

@stof stof and 1 other commented on an outdated diff Sep 14, 2012
Extension/DoctrineBundleExtension.php
@@ -0,0 +1,105 @@
+<?php
+
+/*namespace
+{
+ require_once '/var/www/forks/sql-formatter/lib/SqlFormatter.php';
+}*/
+
+namespace Doctrine\Bundle\DoctrineBundle\Extension;
@stof
stof Sep 14, 2012

a better subnamespace would be Twig

@dlsniper
dlsniper Sep 15, 2012

Hi @stof , I'm working on defining this better, but I'm not sure what do you mean by move to the Twig namespace.
Should I make a PR in twig extensions to add the SQL formmater then continue with this? Best regards.

@stof stof and 1 other commented on an outdated diff Sep 14, 2012
Extension/DoctrineBundleExtension.php
+<?php
+
+/*namespace
+{
+ require_once '/var/www/forks/sql-formatter/lib/SqlFormatter.php';
+}*/
+
+namespace Doctrine\Bundle\DoctrineBundle\Extension;
+
+class DoctrineBundleExtension extends \Twig_Extension
+{
+
+ public function getFilters()
+ {
+ return array(
+ 'doctrineBundleMinifyQuery' => new \Twig_Filter_Method($this, 'minifyQuery'),
@stof
stof Sep 14, 2012

the Twig CS are to use underscore in names for variable and functions, not camelCase. And you should remove bundle IMO

@dlsniper
dlsniper Sep 15, 2012

I've added the bundle part as it is specific to the bundle only, currently and I didn't wanted to collide with other existing user defined functions.

@stof stof commented on an outdated diff Sep 14, 2012
Resources/views/Collector/db.html.twig
@@ -55,22 +55,48 @@
<em>No queries.</em>
</p>
{% else %}
- <ul class="alt">
+ <small><span><abbr title="Click to change the sort type">Sort queries by</abbr> <span class="sortableQueryType" data-target-id="sortable-{{ loop.index }}" onclick="javascript:sortQueries(this);">execution time</span></small>
@stof
stof Sep 14, 2012

I don't like this. It is confusing, especially as the part on which you should click is not the one giving the tooltip

@stof stof and 1 other commented on an outdated diff Sep 14, 2012
Extension/DoctrineBundleExtension.php
+
+ preg_match('/SELECT \[\.{3}\] FROM (.*) WHERE (.*)/i', $result, $miniFromWhere);
+
+ if (strlen($miniFromWhere[1]) > 33) {
+ $miniFrom = substr($miniFromWhere[1], 0, 30) . '...';
+ } else {
+ $miniFrom = $miniFromWhere[1];
+ }
+
+ if (strlen($miniFromWhere[2]) > 33) {
+ $miniWhere = substr($miniFromWhere[2], 0, 30) . '...';
+ } else {
+ $miniWhere = $miniFromWhere[2];
+ }
+
+ $result = preg_replace('/SELECT \[\.{3}\] FROM (.*) WHERE (.*)/i', 'SELECT [...] FROM ' . $miniFrom . ' WHERE ' . $miniWhere, $result);
@stof
stof Sep 14, 2012

what about join, order by, having, ... ?

@stof
stof Sep 14, 2012

and what about queries without a WHERE clause ?

@dlsniper
dlsniper Sep 15, 2012

I've wanted to provide a demo first to obtain some feedback on the looks/functionality of the UI then move on better SQL handling.

I'll update the matching part once the UI is OK .

@stof stof and 1 other commented on an outdated diff Sep 14, 2012
Resources/views/Collector/db.html.twig
- <div>
- {% if query.explainable %}
- <a href="{{ path('_profiler', {'panel': 'db', 'token': token, 'page': 'explain', 'connection': connection, 'query': i}) }}" onclick="return explain(this);" style="text-decoration: none;" title="Explains" data-target-id="explain-{{ i }}-{{ loop.parent.loop.index }}" >
- <img alt="+" src="{{ asset('bundles/framework/images/blue_picto_more.gif') }}" style="display: inline;" />
- <img alt="-" src="{{ asset('bundles/framework/images/blue_picto_less.gif') }}" style="display: none;" />
- </a>
- {% endif %}
- <code>{{ query.sql }}</code>
+ <li class="{{ cycle(['odd', 'even'], i) }}" data-extra-info="{{ '%0.2f'|format(query.executionMS * 1000) }}" data-target-id="{{ i }}">
+ <div style="margin-top: 4px">
+ <span id="smallcode-{{ i }}-{{ loop.parent.loop.index }}">
+ {{ query.sql|doctrineBundleMinifyQuery }}
+ </span>
+
+ <code id="code-{{ i }}-{{ loop.parent.loop.index }}">
+ {{ query.sql|doctrineBundlePrettyQuery|raw }}
@stof
stof Sep 14, 2012

This is not safe. The query is not escaped

@stof
stof Sep 14, 2012

Note that this can occur. Try running this valid SQL query and look at your profiler after that: SELECT '<script type="text/javascript">alert("XSS owns your profiler");</script>'

@dlsniper
dlsniper Sep 15, 2012

Yep, I know, see the comment above with the UI first, then functionality. Thanks!

@stof
Doctrine member

I don't like the new design of the query panel using a table for the parameters, the time and the expand button. The current way to display the parameters and the execution time is better IMO: the table looks weird in small widths (I often use my browser on a half screen width so I see this case) and it seems weird as there is always only 1 row

@stof stof commented on an outdated diff Sep 14, 2012
composer.json
@@ -28,7 +28,8 @@
"require-dev": {
"doctrine/orm": ">=2.2,<2.4-dev",
"symfony/yaml": "2.1.*",
- "symfony/validator": "2.1.*"
+ "symfony/validator": "2.1.*",
+ "jdorn/sql-formatter": "dev-master"
@stof
stof Sep 14, 2012

you should depend on 1.0.*, not on dev-master which is an unbound constraint (dev-master will change forever, even if a BC rbeak is introduced)

@dlsniper

I've did some of the changes mentioned in the comments, related to UI, namespaces and some others.
I'll fix the handling of the rest of the query types once the interface is OK.
The change is visible as well in the link from the OP

@stof stof and 1 other commented on an outdated diff Sep 15, 2012
DependencyInjection/DoctrineExtension.php
@@ -50,6 +50,9 @@ public function load(array $configs, ContainerBuilder $container)
if (!empty($config['orm'])) {
$this->ormLoad($config['orm'], $container);
}
+
+ $loader = new XmlFileLoader($container, new FileLocator(__DIR__.'/../Resources/config'));
@stof
stof Sep 15, 2012

Why creating a new loader ? There is already one.

And why creating a new XML file instead of putting it in the existing one ?

@dlsniper
dlsniper Sep 15, 2012

Because a new loader is used anyway in each of the functions like dbalLoad / ormLoad and because it's a separate scope for what we want to do.

I've added a new file because it isn't related neither to DBAL nor ORM so I didn't saw a point in having the extension in either of them.

@stof
stof Sep 15, 2012

not related to DBAL ? The profiler collector is related to DBAL

@dlsniper
dlsniper Sep 15, 2012

So should I move this to dbal.xml then?

@stof stof commented on an outdated diff Sep 15, 2012
Resources/config/services.xml
@@ -0,0 +1,11 @@
+<?xml version="1.0" ?>
+
+<container xmlns="http://symfony.com/schema/dic/services"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
+ <services>
+ <service id="doctrine.bundle.twig.doctrine_bundle_extension" class="Doctrine\Bundle\DoctrineBundle\Twig\DoctrineBundleExtension">
@stof
stof Sep 15, 2012

the service id does not follow the naming used in the bundle

@stof stof commented on an outdated diff Sep 15, 2012
Twig/DoctrineBundleExtension.php
+ * The code was originally distributed inside the Symfony framework.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ * (c) Doctrine Project, Benjamin Eberlei <kontakt@beberlei.de>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Doctrine\Bundle\DoctrineBundle\Twig;
+
+use Twig_Extension;
+use Twig_Filter_Method;
+use SqlFormatter;
+
+class DoctrineBundleExtension extends Twig_Extension
@stof
stof Sep 15, 2012

Please remove Bundle from the class name

@stof stof and 1 other commented on an outdated diff Sep 15, 2012
Twig/DoctrineBundleExtension.php
+ * This file is part of the Doctrine Bundle
+ *
+ * The code was originally distributed inside the Symfony framework.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ * (c) Doctrine Project, Benjamin Eberlei <kontakt@beberlei.de>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Doctrine\Bundle\DoctrineBundle\Twig;
+
+use Twig_Extension;
+use Twig_Filter_Method;
+use SqlFormatter;
@stof
stof Sep 15, 2012

Symfony does not use use statements for classes in the global namespace

@stof stof and 1 other commented on an outdated diff Sep 15, 2012
Twig/DoctrineBundleExtension.php
+
+ public function getFilters()
+ {
+ return array(
+ 'doctrine_minify_query' => new Twig_Filter_Method($this, 'minifyQuery'),
+ 'doctrine_pretty_query' => new Twig_Filter_Method($this, 'prettyQuery'),
+ );
+ }
+
+ public function minifyQuery($query)
+ {
+ if (strpos($query, 'SELECT') === false) {
+ return $query;
+ }
+
+ $result = preg_replace('/SELECT (.*) FROM (.*) WHERE (.*) /i', 'SELECT [...] FROM $2 WHERE $3', $query);
@stof
stof Sep 15, 2012

My previous comment still apply: what about queries without a WHERE clause ?

@dlsniper
dlsniper Sep 15, 2012

Like I've said, I want first the UI then the rest will follow, like escaping/sanitization and rest of the functionality.

@stof
Doctrine member

The UI for the sorting seems weird to me. When it is ordered by execution order, the only thing you see is Sort by execution time, without any indication about the current sorting.
And as it is totally independant of the pretty SQL display, could you split it in a separate PR so that it can be discussed separately ?

@dlsniper

Ok, I'll do a separate PR for it. I'm not sure how to better display it atm.

@dlsniper

I've added the ability of expanding/collapsing the long sections of a query and squashed the commits as well.
I'll implement the copy of query using the parameters and if that works well and the UI is OK to be used then I'll go and implement the rest of the changes so that this can be merged.
The new changes are visible on the same URL (I'll always update that as well until this gets merged).

Thanks for your help @stof ^^

@stof stof and 1 other commented on an outdated diff Sep 15, 2012
Twig/DoctrineExtension.php
+ * This class contains the needed functions in order to do the query highlighting
+ *
+ * @author Florin Patan <florinpatan@gmail.com>
+ */
+class DoctrineExtension extends \Twig_Extension
+{
+ private $assetsService;
+
+ /**
+ * Inject the container here so that we can extract the assets service and use it
+ *
+ * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
+ */
+ public function __construct(ContainerInterface $container)
+ {
+ $this->assetsService = $container->get('templating.helper.assets');
@stof
stof Sep 15, 2012

This will not solve the issue of the narrower scope, as you are still retrieved a request-scope service when building Twig. You simply avoid the detection done by Symfony. This will not be merged as it makes it impossible to use Twig in the CLI.

@dlsniper
dlsniper Sep 15, 2012

I'm not sure what you mean. I've tried to add the dependency to the service directly or using the scope = 'request' but that didn't worked either.
If there's a recommended way of doing it? I need to get the service in order to retrieve the URL from the extension else I'll just pass the paths as parameters but I don't really like that approach.

@stof
stof Sep 15, 2012

@dlsniper If you retrieve the service in the constructor, it causes the same scope issue than retrieving it outside the constructor and passing it. the logic runs at the same time: during the service creation. The only difference is that the container cannot detect the wrong logic. You should read the doc about scopes. You should retrieve the asset helper only when needing it, and not storing it in a property.

@stof
Doctrine member

I don't see the need to collapse some parts of the full query when the user asks to expand it. It makes no sense to collapse an expanded view IMO.

@dlsniper

There's a need for that as you might have 20+ rows what are columns from the tables which is plain annoying on large queries. The collapsed view is meant to display only the essential parts of the queries for a user to be able to identify it faster in the sea of queries that he/she might have.
Look on the link I've provided at the 3rd query for example and see what happens when you expand the parameters, those are generally useless IMHO once you've narrowed them down.

@stof
Doctrine member

@dlsniper I don't like the fact that you have to expand several times to have the full query. A collapsed view and an expanded view are enough IMO. We don't need a semi-expanded one.

@dlsniper

I've removed the query parts collapse/extend feature.

Also I've changed the way the assets service is now used and I've added a way to display the query with the parameters so that one can copy&paste the query that was ran by Doctrine. I'm not sure about the escaping but as far as my tests went it was pretty good. If I've missed something let me know.

Also regarding the original comment about escaping the highlighted query being displayed with |raw, I don't think it's necessary to do any additional escaping as I wasn't able to run a malformed query, at least not the one in the example within the controller part. When I've looked it up in the Profiler part it wasn't there as it wasn't registered in the collector I suppose.

Next I'll add support for the missing queries from the collapsed view (update/delete, without from and so on).

Thank you for all your help so far @stof ^^

@stof stof and 2 others commented on an outdated diff Sep 16, 2012
Twig/DoctrineExtension.php
+ */
+ public function prettyQuery($query)
+ {
+ return \SqlFormatter::format($query);
+ }
+
+
+ /**
+ * Return a query with the parameters replaced
+ *
+ * @param string $query
+ * @param array $parameters
+ *
+ * @return string
+ */
+ public function replaceQueryParameters($query, $parameters) {
@stof
stof Sep 16, 2012

Please fix the coding standards

@dlsniper
dlsniper Sep 16, 2012

Hi, sorry, what are the coding standards? Symfony2, Doctrine2 or Twig? Thanks :)

@yethee
yethee Sep 16, 2012

The opening brace must be on own line.

This rule suitable for any coding standard that you mentioned :)

@stof
stof Sep 16, 2012

Twig does not have its own CS for the PHP code. It uses the Symfony2 ones (the Twig CS are for templates).
And indeed, this rules comes from PSR-2, which is the base for both the Doctrine and the Symfony CS

@stof stof and 1 other commented on an outdated diff Sep 16, 2012
Twig/DoctrineExtension.php
+ {
+ return \SqlFormatter::format($query);
+ }
+
+
+ /**
+ * Return a query with the parameters replaced
+ *
+ * @param string $query
+ * @param array $parameters
+ *
+ * @return string
+ */
+ public function replaceQueryParameters($query, $parameters) {
+ $i = 0;
+ $escapeFunction = function($parameter) use (&$escapeFunction) {
@stof
stof Sep 16, 2012

Please use a private method instead of a recursive closure

@dlsniper
dlsniper Sep 16, 2012

I'm not sure what do you mean by that as I don't know how to pass the other parameters to the function in the preg_replace_callback below this. I didn't liked the approach but atm I don't know a better one.

@stof stof commented on an outdated diff Sep 16, 2012
Twig/DoctrineExtension.php
+ * Return a query with the parameters replaced
+ *
+ * @param string $query
+ * @param array $parameters
+ *
+ * @return string
+ */
+ public function replaceQueryParameters($query, $parameters) {
+ $i = 0;
+ $escapeFunction = function($parameter) use (&$escapeFunction) {
+ $result = $parameter;
+
+ switch (true) {
+ case is_string($result) : {
+ $result = "'" . addslashes($result) . "'";
+ }; break;
@stof
stof Sep 16, 2012

Please remove the curly braces around the cases.

@stof stof commented on an outdated diff Sep 16, 2012
Twig/DoctrineExtension.php
+ */
+class DoctrineExtension extends \Twig_Extension
+{
+ /**
+ * @var \Symfony\Component\DependencyInjection\ContainerInterface
+ */
+ private $container;
+
+ /**
+ * Inject the container here so that we can extract the assets service and use it
+ *
+ * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
+ */
+ public function __construct(ContainerInterface $container)
+ {
+ $this->container = $container;
@stof
stof Sep 16, 2012

There is no need to inject the container as you don't use it

@stof stof and 1 other commented on an outdated diff Sep 16, 2012
Resources/views/Collector/db.html.twig
<img alt="+" src="{{ asset('bundles/framework/images/blue_picto_more.gif') }}" style="display: inline;" />
<img alt="-" src="{{ asset('bundles/framework/images/blue_picto_less.gif') }}" style="display: none;" />
- </a>
+ <span style="display: none">Shrink query</span>
+ <span id="smallcode-{{ i }}-{{ loop.parent.loop.index }}">
+ {{ query.sql|doctrine_minify_query }}
+ </span>
+ </div>
+ <code id="code-{{ i }}-{{ loop.parent.loop.index }}">
+ {{ query.sql|doctrine_pretty_query(i, loop.parent.loop.index)|raw }}
+ </code>
+ <span id="original-query-{{ i }}-{{ loop.parent.loop.index }}" style="display: none;">{{ query.sql|doctrine_replace_query_parameters(query.params)|raw }}</span>
+ <br/>
+ <small>
+ <strong>Parameters</strong>: {{ query.params|yaml_encode }}
+ [<span id="expandParams-{{ i }}-{{ loop.parent.loop.index }}" onclick="javascript:copyQueryToClipboard(this);" target-data-id="original-query-{{ i }}-{{ loop.parent.loop.index }}" style="cursor: pointer;">Display runnable query</span>]<br/>
@stof
stof Sep 16, 2012

the onclick event seems wrong

@dlsniper
dlsniper Sep 16, 2012

I'm not sure how it's wrong. Please detail.

@stof stof and 1 other commented on an outdated diff Sep 16, 2012
Resources/views/Collector/db.html.twig
+ document.getElementById('small' + target).style.display = 'none';
+ document.getElementById(target).style.display = 'inline';
+
+ sections[0].style.display = 'none';
+ sections[1].style.display = 'inline';
+ sections[2].style.display = 'inline';
+ } else {
+ document.getElementById('small' + target).style.display = 'inline';
+ document.getElementById(target).style.display = 'none';
+
+ sections[0].style.display = 'inline';
+ sections[1].style.display = 'none';
+ sections[2].style.display = 'none';
+ }
+
+ window.location.hash = target.replace('code', 'queryNo');
@stof
stof Sep 16, 2012

why changing the location hash ?

@stof
stof Sep 16, 2012

btw, changing the hash makes the browser scroll to put the query at the top, which is confusing IMO.

@dlsniper
dlsniper Sep 16, 2012

I've removed it for the next commit.

@stof stof commented on an outdated diff Sep 16, 2012
Twig/DoctrineExtension.php
+
+ $result = preg_replace('/SELECT \[\.{3}\] FROM (.*) WHERE (.*)/i', 'SELECT [...] FROM ' . $miniFrom . ' WHERE ' . $miniWhere, $result);
+
+ return $result;
+ }
+
+ /**
+ * Convert a query into a pretty query
+ *
+ * @param string $query
+ *
+ * @return string
+ */
+ public function prettyQuery($query)
+ {
+ return \SqlFormatter::format($query);
@stof
stof Sep 16, 2012

you don't need to create a method for this. You could use new \Twig_Filter_Function('SqlFormatter::format')

@stof stof commented on an outdated diff Sep 16, 2012
Resources/views/Collector/db.html.twig
//]]></script>
+
+ <style>
+ h3 {
+ margin-bottom: 0px;
+ }
+
+ code {
+ display: none;
+ }
+
+ code pre {
+ padding: 5px;
+ }
+
+ .expandable-portion {
@stof
stof Sep 16, 2012

I think this class is not used anymore

@stof stof commented on an outdated diff Sep 16, 2012
Twig/DoctrineExtension.php
+ $escapeFunction = function($parameter) use (&$escapeFunction) {
+ $result = $parameter;
+
+ switch (true) {
+ case is_string($result) : {
+ $result = "'" . addslashes($result) . "'";
+ }; break;
+ case is_array($result) : {
+ foreach($result as &$value) {
+ $value = html_entity_decode($escapeFunction($value));
+ }
+
+ $result = implode(', ', $result);
+ }; break;
+ case is_object($result) : {
+ $result = addslashes($result->__toString());
@stof
stof Sep 16, 2012

what about objects without a __toString method ? I see a common case: DateTime.

And btw, the common way to use it is to cast as string, not to call the method directly (some classes implemented in C can be casted as string without defining this method IIRC)

@stof stof commented on an outdated diff Sep 16, 2012
Twig/DoctrineExtension.php
+ case is_array($result) : {
+ foreach($result as &$value) {
+ $value = html_entity_decode($escapeFunction($value));
+ }
+
+ $result = implode(', ', $result);
+ }; break;
+ case is_object($result) : {
+ $result = addslashes($result->__toString());
+ }
+ }
+
+ return htmlentities($result);
+ };
+
+ $result = preg_replace_callback('/\?/', function() use ($parameters, &$i, $escapeFunction) {
@stof
stof Sep 16, 2012

what about named parameters (the ORM does not use them, but you can use them when using DBAL directly)

@dlsniper

Hello @stof,

Could you please give me some feedback as to how the query shrink should work?

I'm thinking to detect the SELECT/DELETE/UPDATE/INSERT queries and for them provide minification for all content based on FROM/WHERE/GROUP BY and LIMIT keywords with a total size of about 80 chars or so, depending on how much can the layout support when it's on the lowest available width. Would that be good enough?

I've had some issues with my laptop so I couldn't work a demo for it but I think I'll have some time tonight.

I'd like to finish this so that I could add the query sorting part as well.

Thanks for your help!

Best regards.

@dlsniper

I've finally made the required changes in order to minify the query, it will be visible in the URL from the PR description in a couple of minutes.

@dlsniper

When the PR will be merged I'll add the ability for query sorting so that this can be even more functional for the users.

@stof
Doctrine member

The minified query looks really weird currently. for instance, one of them looks like INSERT INTO `match` (`matc VALUES (NULL, \'2006-1-[...]

@stof
Doctrine member

And you have some double escaping in the runnable query. It shows

SELECT m0_.match_id AS match_id0, m0_.match_date AS match_date1,
m0_.hometeam_goals AS hometeam_goals2, m0_.awayteam_goals AS awayteam_goals3, 
t1_.team_id AS team_id4, t1_.name AS name5, t1_.goals_given AS goals_given6, 
t1_.goals_taken AS goals_taken7, t1_.wins AS wins8, t1_.draws AS draws9, t1_.losses AS losses10, 
t2_.team_id AS team_id11, t2_.name AS name12, t2_.goals_given AS goals_given13, 
t2_.goals_taken AS goals_taken14, t2_.wins AS wins15, t2_.draws AS draws16, t2_.losses AS losses17, 
m0_.awayteam_id AS awayteam_id18, m0_.hometeam_id AS hometeam_id19 
FROM `match` m0_ INNER JOIN team t1_ ON m0_.hometeam_id = t1_.team_id 
INNER JOIN team t2_ ON m0_.awayteam_id = t2_.team_id 
WHERE m0_.match_id IN 
('SELECT \'&lt;script type=\&quot;text/javascript\&quot;&gt;alert(\&quot;XSS owns your profiler\&quot;);&lt;/script&gt;\'', 100, 'asd')
@dlsniper

@stof I didn't noticed the double escaping until now, thanks!

In regards to the minified query... I'm not sure what to write in place of the small query to be honest as I don't like the minified version as well but on the other hand I don't have too much space to make it fit in one line only, there's about 80 chars maximum when the view is at minimum width.

I'll try and come up with a version that minifies the query parts after the first space available when having more that 20 chars. That should make it more user friendly but beyond that any idea to improve this is welcomed.

@dlsniper

At least in my examples it looks a bit better now and as you can see I've changed the contents extraction algorithms a bit so hopefully it will be better at finding out which is the end of the string and so on.

@stof stof commented on an outdated diff Oct 2, 2012
Resources/config/dbal.xml
@@ -58,5 +58,10 @@
<argument>%doctrine.default_connection%</argument>
<argument>%doctrine.default_entity_manager%</argument>
</service>
+
+ <service id="doctrine.twig.doctrine_extension" class="Doctrine\Bundle\DoctrineBundle\Twig\DoctrineExtension">
@stof
stof Oct 2, 2012

The service should be private as it does not need to be retrieved dynamically from the container

@stof stof commented on an outdated diff Oct 2, 2012
Resources/views/Collector/db.html.twig
@@ -2,7 +2,9 @@
{% block toolbar %}
{% set icon %}
- <img width="20" height="28" alt="Database" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABQAAAAcCAYAAABh2p9gAAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAAQRJREFUeNpi/P//PwM1ARMDlcGogZQDlpMnT7pxc3NbA9nhQKxOpL5rQLwJiPeBsI6Ozl+YBOOOHTv+AOllQNwtLS39F2owKYZ/gRq8G4i3ggxEToggWzvc3d2Pk+1lNL4fFAs6ODi8JzdS7mMRVyDVoAMHDsANdAPiOCC+jCQvQKqBQB/BDbwBxK5AHA3E/kB8nKJkA8TMQBwLxaBIKQbi70AvTADSBiSadwFXpCikpKQU8PDwkGTaly9fHFigkaKIJid4584dkiMFFI6jkTJII0WVmpHCAixZQEXWYhDeuXMnyLsVlEQKI45qFBQZ8eRECi4DBaAlDqle/8A48ip6gAADANdQY88Uc0oGAAAAAElFTkSuQmCC"/>
+ <img width="20" height="28" alt="Database"
+ src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABQAAAAcCAYAAABh2p9gAAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAAQRJREFUeNpi/P//PwM1ARMDlcGogZQDlpMnT7pxc3NbA9nhQKxOpL5rQLwJiPeBsI6Ozl+YBOOOHTv+AOllQNwtLS39F2owKYZ/gRq8G4i3ggxEToggWzvc3d2Pk+1lNL4fFAs6ODi8JzdS7mMRVyDVoAMHDsANdAPiOCC+jCQvQKqBQB/BDbwBxK5AHA3E/kB8nKJkA8TMQBwLxaBIKQbi70AvTADSBiSadwFXpCikpKQU8PDwkGTaly9fHFigkaKIJid4584dkiMFFI6jkTJII0WVmpHCAixZQEXWYhDeuXMnyLsVlEQKI45qFBQZ8eRECi4DBaAlDqle/8A48ip6gAADANdQY88Uc0oGAAAAAElFTkSuQmCC"
+ xmlns="http://www.w3.org/1999/html"/>
@stof
stof Oct 2, 2012

Why adding the xmns attribute ?

@stof stof commented on an outdated diff Oct 2, 2012
Resources/views/Collector/db.html.twig
@@ -2,7 +2,9 @@
{% block toolbar %}
{% set icon %}
- <img width="20" height="28" alt="Database" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABQAAAAcCAYAAABh2p9gAAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAAQRJREFUeNpi/P//PwM1ARMDlcGogZQDlpMnT7pxc3NbA9nhQKxOpL5rQLwJiPeBsI6Ozl+YBOOOHTv+AOllQNwtLS39F2owKYZ/gRq8G4i3ggxEToggWzvc3d2Pk+1lNL4fFAs6ODi8JzdS7mMRVyDVoAMHDsANdAPiOCC+jCQvQKqBQB/BDbwBxK5AHA3E/kB8nKJkA8TMQBwLxaBIKQbi70AvTADSBiSadwFXpCikpKQU8PDwkGTaly9fHFigkaKIJid4584dkiMFFI6jkTJII0WVmpHCAixZQEXWYhDeuXMnyLsVlEQKI45qFBQZ8eRECi4DBaAlDqle/8A48ip6gAADANdQY88Uc0oGAAAAAElFTkSuQmCC"/>
+ <img width="20" height="28" alt="Database"
@stof
stof Oct 2, 2012

Please revert the indentation change

@stof stof commented on an outdated diff Oct 2, 2012
Twig/DoctrineExtension.php
+ }
+ }
+ }
+
+ return $result;
+ }
+
+ /**
+ * Shrink the values of parameters from a combination
+ *
+ * @param array $parameters
+ * @param array $combination
+ *
+ * @return string
+ */
+ private function shrinkParameters($parameters, $combination) {
@stof
stof Oct 2, 2012

Please keep the curly brace on its own line

@stof stof and 1 other commented on an outdated diff Oct 2, 2012
Twig/DoctrineExtension.php
+ * Minify the query
+ *
+ * @param string $query
+ *
+ * @return string
+ */
+ public function minifyQuery($query)
+ {
+ $result = '';
+ $keywords = array();
+ $required = 1;
+
+ // Check if we can match the query against any of the major types
+ switch (true) {
+ case stripos($query, 'SELECT') !== false:
+ $keywords = array('SELECT', 'FROM', 'WHERE', 'ORDER BY', 'LIMIT');
@stof
stof Oct 2, 2012

What about HAVING, LEFT JOIN, RIGHT JOIN and INNER JOIN ?

@dlsniper
dlsniper Oct 2, 2012

I've considered that for adding but I believe that that's the minimum information you need to identify a query when in minified mode. Adding more content to that would just make the minified queries not so minified imo.

@stof stof commented on an outdated diff Oct 2, 2012
Twig/DoctrineExtension.php
+
+ // If we had a match then we should minify it
+ if ($result == '') {
+ $result = $this->composeMiniQuery($query, $keywords, $required);
+ }
+
+ // There's a bug in the formatter library at the moment so we want to have a workaround for it at this time
+ $oldLevel = error_reporting(E_ALL ^ E_NOTICE);
+
+ $result = \SqlFormatter::highlight($result);
+
+ // Remove unneeded boilerplate HTML
+ $result = str_replace(array("<pre style='background:white;'", "</pre>"), array("<span", "</span>"), $result);
+
+ // Restore the error reporting level back to user default settings
+ error_reporting($oldLevel);
@stof
stof Oct 2, 2012

you should do it before the str_replace call

@stof stof and 1 other commented on an outdated diff Oct 2, 2012
Twig/DoctrineExtension.php
+ case stripos($query, 'INSERT') !== false :
+ $keywords = array('INSERT', 'INTO', 'VALUE', 'VALUES');
+ $required = 2;
+ break;
+
+ // If there's no match so far just truncate it to the maximum allowed by the interface
+ default:
+ $result = substr($query, 0, 80);
+ }
+
+ // If we had a match then we should minify it
+ if ($result == '') {
+ $result = $this->composeMiniQuery($query, $keywords, $required);
+ }
+
+ // There's a bug in the formatter library at the moment so we want to have a workaround for it at this time
@stof
stof Oct 2, 2012

I don't see any bug report on the repo of the library, and no PR fixing the bug either. Please report it upstream (or even better provide a fix) to allow removing this hack

@dlsniper
dlsniper Oct 2, 2012

I'll follow it up with a PR when I'll get the time to find/correct it, it is on my todo list :)

@dlsniper
dlsniper Oct 7, 2012

This seems to be fixed in the latest master as I couldn't reproduce it myself.
I've removed it from sources and if in the future people will bump into issues then a simple report with a use case should help me identify / fix the problem.

@stof stof and 1 other commented on an outdated diff Oct 2, 2012
composer.json
@@ -28,7 +28,8 @@
"require-dev": {
"doctrine/orm": ">=2.2,<2.4-dev",
"symfony/yaml": ">=2.1,<2.3-dev",
- "symfony/validator": ">=2.1,<2.3-dev"
+ "symfony/validator": ">=2.1,<2.3-dev",
+ "jdorn/sql-formatter": "1.0.*"
@stof
stof Oct 2, 2012

why would it be a dev requirement ? This would mean the profiler is broken for everyone (with a Fatal error).

require-dev lists packages needed to develop DoctrineBundle, i.e. to run the testsuite.

@dlsniper
dlsniper Oct 2, 2012

I didn't knew that, thanks!

To be honest Composer changes the definition of dev a bit more that I'd like.

@stof
stof Oct 2, 2012

require-dev is only considered when the repo is the root repo, as documented
this means it will never be read for your projects (btw, you should have seen it in your own demo project, by forcing you to depend on jdorn/sql-formatter in your project)

@stof stof commented on an outdated diff Oct 2, 2012
Twig/DoctrineExtension.php
+ *
+ * @return string
+ */
+ private function shrinkParameters($parameters, $combination) {
+ array_shift($parameters);
+ $result = '';
+
+ $maxLength = 100;
+ $maxLength -= count($parameters) * 5;
+ $maxLength = $maxLength / count($parameters);
+
+ foreach ($parameters as $key => $value) {
+ $value = wordwrap($value, $maxLength, "\n", true);
+ $value = explode("\n", $value);
+ $value = $value[0];
+ $value = DoctrineExtension::escapeFunction($value);
@stof
stof Oct 2, 2012

use self::

@stof stof commented on the diff Oct 2, 2012
Twig/DoctrineExtension.php
+ error_reporting($oldLevel);
+
+ return $result;
+ }
+
+ /**
+ * Escape parameters of a SQL query
+ * DON'T USE THIS FUNCTION OUTSIDE ITS INTEDED SCOPE
+ *
+ * @internal
+ *
+ * @param mixed $parameter
+ *
+ * @return string
+ */
+ static public function escapeFunction($parameter)
@stof
stof Oct 2, 2012

does it really need to be public ? Using a private method as callback for the replaceQueryParameters callback instead of a closure would allow makign thsi method private, thus removing the need to mark it as @internal

@dlsniper
dlsniper Oct 7, 2012

Like I've said, I'm really clueless about what do you mean, sorry :)
If you can help me out with this one it would be much appreciated.

@stof stof commented on an outdated diff Oct 2, 2012
Twig/DoctrineExtension.php
+
+ switch (true) {
+ case is_string($result) :
+ $result = "'" . addslashes($result) . "'";
+ break;
+
+ case is_array($result) :
+ foreach ($result as &$value) {
+ $value = static::escapeFunction($value);
+ }
+
+ $result = implode(', ', $result);
+ break;
+
+ case is_object($result) :
+ $result = addslashes((string)$result);
@stof
stof Oct 2, 2012

missing space after the typecast.

@stof
Doctrine member

your algorithm still has an issue. In some cases, it makes the query longer. See for instance DELETE FROM `match` WHERE match_id = 1 becoming DELETE [...] FROM `match` [...] WHERE match_id = 1 [...]

@dlsniper

I've added most of the feedback on the latest commit but I'm not sure what to do about the escaping function.
I don't really like the solution either but right now I don't understand what do you mean. If you can provide some sample then I'll gladly adapt/learn from it.

Also I've added you @stof to the authors list as your input is invaluable and I think it's only fair / nice to have you there as well. Thanks for helping getting this to the current state :)

@dlsniper

I think this is ready to be merged.

I'll wait for @jdorn to solve the issue jdorn/sql-formatter#6 in order to have a proper stable release of the library and so have a stable release of this feature.

I can squash the changes if you want that before merging this, just let me know.

@dlsniper

I've fixed the last remaining issue, as far as I can tell.
Any other opinion about this PR or could this be merged so that I can start working on something else for this bundle?

Thank you!

@dlsniper

Is there anything that prevents this to be merged? Thanks :)

@dlsniper

Any words on this? Thanks

@beberlei beberlei merged commit 4fa47ca into doctrine:master Nov 10, 2012

1 check passed

Details default The Travis build passed
@doctrinebot doctrinebot referenced this pull request in doctrine/dbal Dec 6, 2015
Open

DBAL-372: Add SQL parser #1562

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment