New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VarDumper and DebugBundle #10640

Merged
merged 31 commits into from Sep 23, 2014

Conversation

Projects
None yet
@nicolas-grekas
Member

nicolas-grekas commented Apr 5, 2014

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR symfony/symfony-docs#4243

From a user land point of view, this PR creates a global dump() function that is to be used instead of var_dump(). The component can be used standalone in any dev workflow. Please see the provided README for details.

When used with the Framework bundle, variables passed to dump() are dumped in a new dedicated panel in the web toolbar. The function is also available in twig templates.

Regarding the implementation, I'm pretty sure you'll find a lot to comment. As I'm sure of nothing else, not even the names of things, please do.

I tried to organize this PR in several commits, from the most fundamental algorithm to pure Symfony glue.
I suggest you follow this order while progressing in the review and the discussion around this PR, so that we can together validate commits one after the other.

Don't hesitate to fork the PR and submit PR on it, I'll cherry-pick your patches.

TODO:

  • open a doc PR: symfony/symfony-docs#4243
  • open a PR on the Standard edition: symfony/symfony-standard#710
  • prefix the CSS classes
  • tests for the DebugBundle + other Symfony glue classes
  • inline css and js for compat with e.g. Silex
  • finish and merge nicolas-grekas/Patchwork-Dumper#5 for better UX
  • show a dump excerpt on hovering the icon in the toolbar
  • verify README and comments
  • validate interfaces/names (Caster / Cloner / Dumper)
  • validate new VarDumper component + DebugBundle
  • validate Resource/ext/ vs independent repos.
  • test and define behavior after KernelEvents::RESPONSE
  • update dependencies between components/bundles and composer.json files
  • no hard dep on iconv

Not for this PR but might be worth later:

  • show a light stack trace + timing + memory at debug() calls
  • create a "theme" concept for custom colors/UX
@Koc

This comment has been minimized.

Contributor

Koc commented Apr 5, 2014

Awesome bulk of work was done! Very interesting component. I hope it will be part of Symfony.

{
foreach ($c as $k => $obj) {
$a[$k] = $obj;
if (null !== $i = $c->getInfo()) $a["\0~\0$k"] = $i;

This comment has been minimized.

@romainneutron

romainneutron Apr 5, 2014

Member

code style : it should be on two lines, using brackets

function symfony_zval_info($key, $array, $options = 0)
{
// $options is currenlty not used, but could be in future version.

This comment has been minimized.

@pborreli

pborreli Apr 5, 2014

Contributor

currenlty -> currently

switch (true)
{
case null === data: data = 'null';

This comment has been minimized.

@pborreli

pborreli Apr 5, 2014

Contributor

[cs] case(s) should be indented

debug($vars);
return '';

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

This looks weird to me. The meaning of a Twig function is that it returns some content. If debug() is outputing the debugging data, there is 2 solutions to respect the Twig semantic:

  • make it a Twig tag rather than a Twig function
  • use output buffering to return the output

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 6, 2014

Member

This is the indented behavior: rather than outputting anything, the debug() function fills in a data collector. The data is then displayed in the toolbar. The same occurs when debug() is used in PHP code.

This comment has been minimized.

@Koc

Koc Apr 6, 2014

Contributor

So construction like debug($variable); exit; will not output anything?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 6, 2014

Member

It depends on how you configure the debug handler. In CLI mode, vars are displayed on stderr. In web mode, data is collected for displaying in the toolbar. So I think that yes: debug+die is not expected workflow currently.

This comment has been minimized.

@stof

stof Apr 6, 2014

Member

@nicolas-grekas then it should not be a Twig function but a tag, to respect the Twig semantic.

It is perfectly fine to collect the output separately, but it should be done in the Twig way

This comment has been minimized.

@Koc

Koc Apr 6, 2014

Contributor

What about two sollutions: override standard Twig's dump function http://twig.sensiolabs.org/doc/functions/dump.html for immediately output as now and tag for collect?

This comment has been minimized.

@Koc

Koc Apr 6, 2014

Contributor

Maybe same for native functions: debug and debug_collect.

This comment has been minimized.

@stof

stof Apr 6, 2014

Member

@Koc The VarDebug component is not about immediate output. So overwriting the standard dump function would be weird IMO.

and a debug_collect function would still suffer from the same issue. Collecting the debug of the variable is not a function is the Twig semantic, it is a tag. This ends here.

This comment has been minimized.

@Koc

Koc Apr 6, 2014

Contributor

You misunderstood me: I'm proposing debug_collect function for the native php (for using in controllers).

$rootNode
->children()
->arrayNode('var_debug')
->info('va_debug configuration')

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

typo: var_debug

->children()
->integerNode('max_items')
->info('Max number of displayed items, all levels included, 0 means no limit, -1 only first level')
->min(0)

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

the info explains that -1 means only first level, but this is preventing to pass -1. There is a mistake somewhere

->min(0)
->defaultValue(500)
->end()
->integerNode('max_string')

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

shouldn't it be max_string_length instead ?

@@ -130,6 +130,21 @@ public function load(array $configs, ContainerBuilder $container)
$loader->load('serializer.xml');
}
$loader->load('var_debug.xml');
if (!$container->getParameter('var_debug.collector.class')) {

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

this check should be removed, as the parameter is not set yet in the temp container passed to the extension and so the condition will always be true (and if you set the parameter yourself in the main config, it will win over the parameter set in the DI extension anyway)

);
}
if (isset($config['var_debug'])) {

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

you should add a call to addDefaultsIfNotSet() in the configuration class, so that the default values of the configuration tree are used even when you don't put var_debug: ~ in your config

set_debug_handler(function ($var) use ($container) {
$data = $container->get('var_debug.collector')->collect($var);
$container->get('var_debug.dumper.cli')->dump($data);

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

shouldn't the CLI dumper be used only in a CLI context ?

<services>
<service id="var_debug.collector" class="%var_debug.collector.class%">
</service>

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

you should use a self-closing tag for consistency with the Symfony code base

xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
<parameters>
<parameter key="var_debug.collector.class"><!-- empty for auto-selection --></parameter>

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

just don't set it there, and set it always in the DI extension

@@ -0,0 +1,243 @@
"use strict";

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

Please don't put a use strict outside functions, as this will make all JS code running in strict mode (it puts the global scope in strict mode), which can break other JS on the page. It is not sage

case null === data: data = 'null';
case true === data:
case false === data:
default:

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

should't default be the last case ?

{% set icon %}
<img alt="debug()" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABwAAAAcCAMAAABF0y+mAAAAt1BMVEUAAAANDQ0AAAAhISESEhIAAABEREQAAAAAAAAQEBAAAAAAAAAFBQUMDAwVFRU2NjYJCQkGBgYNDQ0AAAA+Pj5FRUUuLi4QEBAuLi41NTUICAgFBQUAAAAAAAA6Ojo+Pj4jIyMJCQkEBAQAAAATExMqKipAQEAbGxssLCwAAAA6OjoKCgodHR0fHx89PT0ZGRlKSkpOTk5PT09dXV1SUlJaWlpYWFhGRkZVVVVERERiYmJUVFRISEj7yUYlAAAAOHRSTlMAahiweAX4AgFvCAo4Y4TjUyxmKO773HLO3EQ2JBHv/bRYOw59xPWW0B/5T56o5o7//////////vCDrPIAAAGKSURBVCjPbVPneoMwDBQrQPYgjCxSMpu2qQcz8fs/V22RfDSl+oGFzrKl0xl+mQP7zAUb/jPb2X3eTjunjWJCz7h9dQH+wo4DoE8GEY8GEx1/X9ImrlFxfue8Y7gTjDTYW5Txe3jNrmHJs2jcoHLVCk6NqT7ke326ZbwaPFFH5hWZN90BmNwFWGteNh+rOCbORrn3AdBFUJY79vJoBXadqOU0kJtsBOUCJs0HElFN6dv80JVBC0zJkCXRXpgb6/rWlaAaKEMQzaXFu2KlvxiWXmJbliXvzAPoSs9OvHK4WMKIEpqyeUcZOZgHgp5gKSXiAVKiLA3NMEWPIojHpsfpst/vJ7NT+T1LpLfUjqk8FgsqxLOg8lFQQCpVELbCVCsgW0ld+VWtMGwFSWAikLseoCJBMCShpo9tavrSAOnbMB/pq4mP6UZTxDMkfkNjJP45sphWamRMjayiMY6sGbZPqDiPyOgsKPFx2K8yuRAiCLnUMmkLzCd+I7CWNEkjzbaoRVvUzXOoXp7DD5aJLSEAGSFlAAAAAElFTkSuQmCC" />
<span class="sf-toolbar-status{% if dumps_count > 0 %} sf-toolbar-status-yellow{% endif %}">{{ dumps_count }}</span>

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

I suggest hiding the toolbar element entirely if there is no dumped data (just like the Swiftmailer and form items are hidden), to limit the amount of useless elements in the toolbar (the toolbar becomes broken when the content cannot fit in a single row anymore)

set_debug_handler(function ($var) use ($container) {
$data = $container->get('var_debug.collector')->collect($var);
$container->get('data_collector.var_debug')->dump($data);

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

shoudln't this be handled by the FrameworkBundle ? WebProfilerBundle is not responsible for configuring the profiler and its collectors. This is done in FrameworkBundle. WebProfilerBundle is only the web interface to display the profiler data

*/
class DoctrineCaster
{
public static function castCommonProxy(\Doctrine\Common\Proxy\Proxy $p, array $a)

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

please add a use statement

namespace Symfony\Component\VarDebug\Caster;
use PDO;
use PDOStatement;

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

The Symfony codebase does not put use statements for the global namespace

return new Data($data);
}
abstract protected function doCollect($var);

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

Please add the phpdoc on the different methods (use {@inheritDoc} for methods documented in the interface)

{
try {
// Ignore invalid $callback
$callback = @call_user_func($callback, $obj, $a);

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

the naming seems weird. $callback is not the callback anymore after this call. It is the casted data

$dumper->dumpEnd();
}
protected function dumpItem($dumper, $cursor, &$refs, $item)

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

should be private (we make everything private in Symfony unless we want to provide an extension point, which does not seem to be the case here)

`stream_get_meta_data()`. Add your own dedicated `Dumper\Caster` and get the
view *you* need.
- configurable output format: HTML, command line with colors or [a dedicated high
accuracy JSON format](Resource/doc/json-spec.md).

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

having a doc folder in the component looks weird to me. This should go in the symfony-docs repo itself

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 11, 2014

Member

This is not an end user doc but a specification that explains why the JsonDumper behaves the way it is

dnl Comments in this file start with the string 'dnl'.
dnl Remove where necessary. This file will not work
dnl without editing.

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

Shoudln't this file be cleaned from comments coming from the default template to adapt the file to the extension itself ?

@@ -0,0 +1,13 @@
// $Id$
// vim:ft=javascript

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

vim setting should be removed IMO (and $Id$ as well)

return $h['handler']($var);
}
function set_debug_handler(\Closure $closure)

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

couldn't it accept any callable rather than just closures ?

$dumper->dump($collector->collect($var));
};
} else {
$h['handler'] = function ($var) {var_dump($var);};

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

once any callable is supported, this could become $h['handler'] = 'var_dump'

"files": [ "Resources/functions/debug.php" ],
"psr-0": { "Symfony\\Component\\VarDebug\\": "" }
},
"target-dir": "Symfony/Component/ClassLoader"

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

I suggest using PSR-4 instead of PSR-0 + target-dir (target-dir is deprecated in composer as it was only a workaround because of the missing PSR-4 spec)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 11, 2014

Member

PSR-4 is not used currently in the code base. I'd prefer not being the first to make the move, but move together with all the other components.

],
"require": {
"php": ">=5.3.3"
},

This comment has been minimized.

@stof

stof Apr 5, 2014

Member

I would add a suggestion on ext-symfony_debug to make people aware of the available extension

This comment has been minimized.

@nicolas-grekas
@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Sep 23, 2014

Here is the new default rendering:
capture du 2014-09-23 14 59 13
capture du 2014-09-23 14 58 57

I removed the option for several rendering engines and choose to keep the "base" one. This removes the Patchwork and @OwlyCode renderer. As the JsonDumper is not used anymore, I also removed it and its related tests+doc from the git history of this PR. I kept them in patchwork/dumper so they can come back again later.

I also enhanced the base rendering engine with some JS. This should match some of your suggestions @webmozart .
The main benefit of this approach is that enhancing the HtmlDumper not only enhances the profiler panel, but also inline dumps (the twig function and dump()+exit in your code).

(the remaining fabbot error is intentional)

@fabpot

This comment has been minimized.

Member

fabpot commented Sep 23, 2014

It took time, but here we go, this is in now. Thank you very much @nicolas-grekas.

@fabpot fabpot merged commit 80fd736 into symfony:master Sep 23, 2014

0 of 2 checks passed

continuous-integration/travis-ci The Travis CI build failed
Details
default Failure: Travis, fabbot
Details

fabpot added a commit that referenced this pull request Sep 23, 2014

feature #10640 VarDumper and DebugBundle (jpauli, nicolas-grekas, rui…
…an, moux2003, tony-co, romainneutron, oscherler, lyrixx)

This PR was merged into the 2.6-dev branch.

Discussion
----------

VarDumper and DebugBundle

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | none
| License       | MIT
| Doc PR        | none

From a user land point of view, this PR creates a global `dump()` function that is to be used instead of `var_dump()`. The component can be used standalone in any dev workflow. Please see the [provided README](https://github.com/symfony/symfony/pull/10640/files?short_path=52d526f#diff-52d526f19bc9e3825c80e7694755409c) for details.

When used with the Framework bundle, variables passed to `dump()` are dumped in a new dedicated panel in the web toolbar. The function is also available in twig templates.

Regarding the implementation, I'm pretty sure you'll find a lot to comment. As I'm sure of nothing else, not even the names of things, please do.

I tried to organize this PR in several commits, from the most fundamental algorithm to pure Symfony glue.
I suggest you follow this order while progressing in the review and the discussion around this PR, so that we can together validate commits one after the other.

Don't hesitate to fork the PR and submit PR on it, I'll cherry-pick your patches.

TODO:
- [x] open a doc PR: symfony/symfony-docs#4243
- [x] open a PR on the Standard edition: symfony/symfony-standard#710
- [x] prefix the CSS classes
- [x] tests for the DebugBundle + other Symfony glue classes
- [x] inline css and js for compat with e.g. Silex
- [x] finish and merge nicolas-grekas/Patchwork-Dumper#5 for better UX
- [x] show a dump excerpt on hovering the icon in the toolbar
- [x] verify README and comments
- [x] validate interfaces/names (Caster / Cloner / Dumper)
- [x] validate new VarDumper component + DebugBundle
- [x] validate Resource/ext/ vs independent repos.
- [x] test and define behavior after KernelEvents::RESPONSE
- [x] update dependencies between components/bundles and composer.json files
- [x] no hard dep on iconv

Not for this PR but might be worth later:
- show a light stack trace + timing + memory at debug() calls
- create a "theme" concept for custom colors/UX

Commits
-------

80fd736 [DebugBundle] Enhance some comments
2e167ba [TwigBridge] add Twig dump() function + tests and fixes
0f8d30f [VarDumper] Replace \e with \x1B in CliDumper to support colour in PHP < 5.4
d43ae82 [VarDumper] Add workaround to https://bugs.php.net/65967
a8d81e4 [DebugBundle] Inlined assets to avoid installation issues
5f59811 [DebugBundle] Add doc example for Twig usage
e4e00ef [TwigBridge] DumpNode and Token parser
de05cd9 [DebugBundle] enhance dump excerpts
49f13c6 [HttpKernel] add tests for DumpDataCollector
081363c [HttpKernel] tests for DumpListener
0d8a942 [VarDumper] add Stub objects for cutting cleanly and dumping consts
c8746a4 [DebugBundle] add tests for twig and for the bundle
8d5d970 [DebugBundle] adjust after review
eb98c81 [DebugBundle] dump() + better Symfony glue
9dea601 [DebugBundle] global dump() function for daily use
297d373 [VarDumper] README, LICENSE and composer.json
a69e962 [VarDumper] tests for HtmlDumper
5eaa187 [VarDumper] tests for CliDumper
e6dde33 [VarDumper] HTML variant of the CLI dumper
fa81544 [VarDumper] CLI dedicated dumper and related abstract
1d5e3f4 [VarDumper] interface for dumping collected variables
0266072 [VarDumper] casters for DOM objects
c426d8b [VarDumper] casters for Doctrine objects
0a92c08 [VarDumper] casters for PDO related objects
da3e50a [VarDumper] casters for SPL data structures
c91bc83 [VarDumper] casters for exceptions representation
3ddbf4b [VarDumper] add casters for per class/resource custom state extraction
5b7ae28 [VarDumper] symfony_debug ext. fast and memory efficient cloning algo
07135a0 [VarDumper] algo to clone any PHP variable to a breadth-first queue
4bf9300 [Debug] a README for the debug extension
eec5c92 [Debug] Symfony debug extension

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:var-debug branch Sep 23, 2014

@mykiwi

This comment has been minimized.

Contributor

mykiwi commented Sep 23, 2014

👏

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Sep 23, 2014

This is an amazing enhancement, thanks to all contributors !

@kristoffeys

This comment has been minimized.

kristoffeys commented Sep 24, 2014

awesome! 👊

@bonswouar

This comment has been minimized.

bonswouar commented Sep 24, 2014

Was waiting impatiently for this feature, thanks guys !

EDIT : Though, bad news for LadybugBundle, won't be very useful anymore...

fabpot added a commit to symfony/symfony-standard that referenced this pull request Sep 25, 2014

feature #710 Enable DebugBundle (nicolas-grekas)
This PR was merged into the 2.6-dev branch.

Discussion
----------

Enable DebugBundle

This enables the DebugBundle as proposed in symfony/symfony#10640

Commits
-------

dadea61 Enable DebugBundle
@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 2, 2014

For reference, if you'd like to use VarDumper / dump() in your Symfony 2.3/4/5 app, I backported the Debug-Bundle, here it is:
https://github.com/tchwork/debug-bundle

@xabbuh

This comment has been minimized.

Member

xabbuh commented Dec 2, 2014

@nicolas-grekas Could it be worth to add this information to the documentation?

@kriswallsmith

This comment has been minimized.

I found an issue here. If I add the following to a form…

$builder->addEventListener(FormEvents::SUBMIT, 'dump');

…I get a PHP notice…

Undefined index: file

Thank you for your great work on this!

This comment has been minimized.

Member

stof replied Mar 12, 2015

Please open issues to report bugs. Comments on old commits are not tracked so it gets lost

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