[HttpKernel] Use VarDumper in the profiler #19614

wouterj · 2016-08-14T18:51:46Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	part of #18149
License	MIT
Doc PR	-

/cc @javiereguiluz @nicolas-grekas As you're the main maintainers of the changed features.

Summary

The varToString() method is deprecated in favor of cloneVar() (using the VarCloner) and dump() (using the VarDumper).

This allows to show more detailed and better formatted data in the profiler.
The Data class of VarDumper is made serializable, to reduce the size of the stored profiler data.

Screenshots

Further Improvements

Change the dump colors (I've now implemented a very basic light theme, but my colorskills are close to zero, so a proper designer should look at it)

wouterj · 2016-08-14T18:52:37Z

src/Symfony/Bundle/WebProfilerBundle/Twig/WebProfilerExtension.php

+            return stream_get_contents($dump);
+        }
+
+        @trigger_error('Dumping non-cloned data is deprecated since version 3.2 and will be removed in 4.0. Use DataCollector::cloneVar().', E_USER_DEPRECATED);


not sure if this makes sense:

Strings are still perfectly fine

Deprecations aren't shown, as profiler pages aren't data-collected afaik

fabpot · 2016-08-14T19:55:56Z

@wouterj Looks really interesting. Would it be possible to try to keep the same vertical height as before?

Koc · 2016-08-14T20:58:44Z

src/Symfony/Bundle/WebProfilerBundle/Twig/WebProfilerExtension.php

+            $this->dumper->setOutput($prevOutput);
+            rewind($dump);
+
+            return stream_get_contents($dump);


@nicolas-grekas is it possible provide more convenient api for getting dumped content?

Proposed a way to do this: #19616

This snippet can be reduced to three lines by using the second arg of the dump method and the third of steam_get_contents

Koc · 2016-08-14T21:04:02Z

ref #18149, #18074

wouterj · 2016-08-14T21:12:53Z

Would it be possible to try to keep the same vertical height as before?

Yes. I've now updated the styles to accomplish this. The margin, padding and background (for visible correctness) have now been removed. New after screenshot:

nicolas-grekas · 2016-08-15T06:40:12Z

src/Symfony/Component/VarDumper/Cloner/Data.php

@@ -14,7 +14,7 @@
 /**
 * @author Nicolas Grekas <p@tchwork.com>
 */
-class Data
+class Data implements \Serializable


Is it need? I don't get why, can't the object be serialized without implementing this?

@wouterj can you show before/after content of serialized object? Looks like you pass all of the properties to serialize, does it some changes?

reverted this change, it was also causing build errors.

wouterj · 2016-08-15T13:26:12Z

I don't understand the PHP 7.1 failures.

yceruto · 2016-08-15T14:13:34Z

If anything helps "A non well formed numeric value encountered" is a common data conversion error str to time or number format.

nicolas-grekas · 2016-08-16T10:38:21Z

src/Symfony/Bundle/WebProfilerBundle/Twig/WebProfilerExtension.php


    /**
     * {@inheritdoc}
     */
    public function getFunctions()
    {
        return array(
-            new \Twig_SimpleFunction('profiler_dump', array($this, 'dumpValue')),
+            new \Twig_SimpleFunction('profiler_dump', array($this, 'dumpValue'), array('is_safe' => array('html'))),


This looks dangerous to me: the legacy implementation is still in use, but it's now considered "html-safe".
What about using a new function instead?

nicolas-grekas · 2016-08-17T16:16:48Z

See suggestions as patch directly in nicolas-grekas#6

wouterj · 2016-08-17T21:13:27Z

Thanks @nicolas-grekas for your patch! I've cherry-picked it into this PR and tried to fix the LoggerDataCollector (not sure if this is what was expected though, will test later this week in a Symfony app).

I hope the PHP 7.1 test failure is suddently fixed as well. I've tested it using PHP 7.1.0-beta2 on my local PC and don't get the error.

weaverryan · 2016-08-17T21:52:06Z

This is great! Is it possible in the component to have the objects dumped in a non-expanded state by default? I was thinking that for things like your contentDocument, it pushes the other content far down on the initial load of the page.

Thanks!

wouterj · 2016-08-17T22:38:06Z

@weaverryan I've now collapsed all dumps by default (and expanded the first level for the log context).

Also tried to fix the tests.

nicolas-grekas · 2016-08-18T12:27:06Z

New suggestions in the latest commit of nicolas-grekas#6 (needs #19656 to be at its best):

add warnings in the "Logs" count, more important now that throwing exceptions in dev can be turned of (see [Debug] Better error handling #19568)
properly remove sanitizeContext: VarDumper does the job.

@wouterj

…ctures (nicolas-grekas) This PR was merged into the 3.2-dev branch. Discussion ---------- [VarDumper] Allow dumping subparts of cloned Data structures | Q | A | ------------- | --- | Branch? | master | New feature? | yes | Tests pass? | yes | License | MIT | Doc PR | symfony/symfony-docs#6891 ping @wouterj: with this, we'll be able to dump only the trace for deprecations in #19614 instead of being forced to dump the full exception right now. See test case. We'd do `{{ profiler_dump(log.context.seek('trace')) }}` in `logger.html.twig`. Commits ------- 8f2f440 [VarDumper] Allow dumping subparts of cloned Data structures

nicolas-grekas · 2016-08-19T17:21:47Z

PR rebased in my fork to benefit from latest VarDumper features. Don't forget to set framework.php_errors.log to true in app/config/config.yml until #19656 is merged.
This still needs work to put everything together in a nice way but I think my contrib can pause here as the difficult part related to tightly plugging VarDumper should be OK now.

wouterj · 2016-08-20T13:04:25Z

Discovered a problem with this implementation (with all latest work by @nicolas-grekas): The logger panel is now completely unresponsive in the CMF sandbox (it has 251 debug messages with context that includes a Doctrine mapped document).

Is there anyway to work around this?

nicolas-grekas · 2016-08-20T15:32:24Z

That's one of the reasons why I think this needs more work :) The dumps integration is a bit rough right now, but there is no issue with the base implementation, only more CSS to write and a few JS to fine tune the US. What about adding back the show/hide context button?

stof · 2016-08-26T08:33:51Z

src/Symfony/Component/Form/Extension/DataCollector/FormDataExtractor.php

@@ -11,10 +11,15 @@

 namespace Symfony\Component\Form\Extension\DataCollector;

+use Symfony\Component\Form\Extension\DataCollector\FormDataExtractorInterface;


useless as it is already in the same namespace

stof · 2016-08-26T08:41:21Z

The serialized data is fine. We never guaranteed BC on the storage format of the profiler between minor versions (i.e. you cannot load a profile generated with X.Y in a X.Z runtime, you have to clear your profile storage when upgrading, which is done by default as they are stored in the cache).

regarding the DebugAccessDecisionManager and the FormDataExtractor, I think it is fine, as they are mostly meant to be consumed by the profiler collectors.

fabpot · 2016-09-14T19:09:06Z

what's the status of this PR? I would really like to be able to merge it in time for 3.2 (end of dev by the end of the month).

wouterj · 2016-09-17T11:09:15Z

With the great help of @nicolas-grekas, I think this PR is now finished and ready for review. I've just tested all changed profiler panels and fixed some small rendering issues.

nicolas-grekas · 2016-09-17T12:37:25Z

Two glitches:

one test needs a fix in DumpDataCollectorTest
var-dumper ~3.2 needs to be added to the composer.json of SecurityBundle (require-dev)

wouterj · 2016-09-17T14:23:48Z

thanks, applied the changes

nicolas-grekas · 2016-09-17T14:44:51Z

👍

fabpot · 2016-09-17T15:50:28Z

Thank you @wouterj.

@javiereguiluz

…icolas-grekas) This PR was merged into the 3.2-dev branch. Discussion ---------- [HttpKernel] Use VarDumper in the profiler | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | part of #18149 | License | MIT | Doc PR | - /cc @javiereguiluz @nicolas-grekas As you're the main maintainers of the changed features. Summary --- * The `varToString()` method is deprecated in favor of `cloneVar()` (using the VarCloner) and `dump()` (using the VarDumper). This allows to show more detailed and better formatted data in the profiler. * The `Data` class of VarDumper is made serializable, to reduce the size of the stored profiler data. Screenshots --- ![sf-profiler-dumper](https://cloud.githubusercontent.com/assets/749025/17651142/9bcddc14-6260-11e6-80f6-81b84c82c0a3.png) Further Improvements --- * Change the dump colors (I've now implemented a very basic light theme, but my colorskills are close to zero, so a proper designer should look at it) Commits ------- eddecbd [HttpKernel] Use VarDumper in the Logs&Events panels of the profiler 41a7649 [HttpKernel] Use VarDumper in the profiler

…or should use cloneVar (ogizanagi) This PR was merged into the 3.2 branch. Discussion ---------- [WebProfilerBundle][Translator] Fix TranslationDataCollector should use cloneVar | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes? | New feature? | no? | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A Was missed in #19614 ? ### Before ![capture d ecran 2016-11-30 a 15 45 33](https://cloud.githubusercontent.com/assets/2211145/20756865/9a416934-b714-11e6-9cb5-890a6222b6fa.png) ### After ![capture d ecran 2016-11-30 a 15 43 58](https://cloud.githubusercontent.com/assets/2211145/20756877/9efaccae-b714-11e6-9523-b3f8f2e4bd8c.png) Commits ------- 07cdfd5 [WebProfiler][Translator] Fix TranslationDataCollector should use cloneVar

…Collector should use cloneVar (ogizanagi) This PR was merged into the 3.2 branch. Discussion ---------- [WebProfilerBundle][Translator] Fix TranslationDataCollector should use cloneVar | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes? | New feature? | no? | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A Was missed in symfony#19614 ? ### Before ![capture d ecran 2016-11-30 a 15 45 33](https://cloud.githubusercontent.com/assets/2211145/20756865/9a416934-b714-11e6-9cb5-890a6222b6fa.png) ### After ![capture d ecran 2016-11-30 a 15 43 58](https://cloud.githubusercontent.com/assets/2211145/20756877/9efaccae-b714-11e6-9523-b3f8f2e4bd8c.png) Commits ------- 07cdfd5 [WebProfiler][Translator] Fix TranslationDataCollector should use cloneVar

carsonbot added Status: Needs Review Feature Deprecation labels Aug 14, 2016

wouterj reviewed Aug 14, 2016
View reviewed changes

Koc reviewed Aug 14, 2016
View reviewed changes

wouterj mentioned this pull request Aug 14, 2016

[POC] [VarDumper] Added StringDumper to export dump as string #19616

Closed

1 task

nicolas-grekas reviewed Aug 15, 2016
View reviewed changes

wouterj force-pushed the profiler_dumper branch from 8010483 to 6e092ef Compare August 15, 2016 13:00

nicolas-grekas reviewed Aug 16, 2016
View reviewed changes

nicolas-grekas mentioned this pull request Aug 17, 2016

[HttpKernel] Use VarDumper in the profiler nicolas-grekas/symfony#6

Closed

nicolas-grekas changed the title ~~[Profiler] Use VarDumper in the profiler~~ [HttpKernel] Use VarDumper in the profiler Aug 17, 2016

wouterj force-pushed the profiler_dumper branch from 6e092ef to e6cc848 Compare August 17, 2016 21:11

nicolas-grekas mentioned this pull request Aug 19, 2016

[VarDumper] Allow dumping subparts of cloned Data structures #19672

Merged

wouterj force-pushed the profiler_dumper branch from 06d75a7 to 9ec1d88 Compare August 25, 2016 08:54

stof reviewed Aug 26, 2016
View reviewed changes

wouterj force-pushed the profiler_dumper branch 2 times, most recently from 693386d to a6774cb Compare August 31, 2016 10:33

This was referenced Sep 2, 2016

[HttpKernel][EventDispatcher] Wrap some log contexts with ClassStub #19829

Closed

Correctly display multiline log entries in Profiler Logs section #19888

Closed

wouterj force-pushed the profiler_dumper branch from a6774cb to fa41e63 Compare September 17, 2016 11:08

wouterj and others added 2 commits September 17, 2016 16:23

[HttpKernel] Use VarDumper in the profiler

41a7649

[HttpKernel] Use VarDumper in the Logs&Events panels of the profiler

eddecbd

wouterj force-pushed the profiler_dumper branch from fa41e63 to eddecbd Compare September 17, 2016 14:23

fabpot merged commit eddecbd into symfony:master Sep 17, 2016

wouterj deleted the profiler_dumper branch September 17, 2016 15:52

nicolas-grekas mentioned this pull request Sep 20, 2016

[Form][EventDispatcher] Fix VarDumper usage related to perf regression #19986

Merged

chalasr mentioned this pull request Sep 21, 2016

[SecurityBundle] Integrate current firewall in Profiler #19490

Merged

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

derrabus mentioned this pull request Nov 18, 2016

[HttpKernel] HttpKernel Don't allow installing HttpKernel 3.2 with WebProfilerBundle 3.1 #20558

Closed

ogizanagi mentioned this pull request Nov 30, 2016

[WebProfilerBundle][Translator] Fix TranslationDataCollector should use cloneVar #20700

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpKernel] Use VarDumper in the profiler #19614

[HttpKernel] Use VarDumper in the profiler #19614

wouterj commented Aug 14, 2016

wouterj Aug 14, 2016

fabpot commented Aug 14, 2016

Koc Aug 14, 2016

wouterj Aug 14, 2016

nicolas-grekas Aug 15, 2016

Koc commented Aug 14, 2016

wouterj commented Aug 14, 2016 •

edited

Loading

nicolas-grekas Aug 15, 2016

Koc Aug 15, 2016

wouterj Aug 15, 2016

wouterj commented Aug 15, 2016

yceruto commented Aug 15, 2016 •

edited

Loading

nicolas-grekas Aug 16, 2016 •

edited

Loading

nicolas-grekas commented Aug 17, 2016

wouterj commented Aug 17, 2016

weaverryan commented Aug 17, 2016

wouterj commented Aug 17, 2016

nicolas-grekas commented Aug 18, 2016 •

edited

Loading

nicolas-grekas commented Aug 19, 2016

wouterj commented Aug 20, 2016

nicolas-grekas commented Aug 20, 2016

stof Aug 26, 2016

stof commented Aug 26, 2016

fabpot commented Sep 14, 2016

wouterj commented Sep 17, 2016

nicolas-grekas commented Sep 17, 2016 •

edited

Loading

wouterj commented Sep 17, 2016

nicolas-grekas commented Sep 17, 2016

fabpot commented Sep 17, 2016

		@@ -11,10 +11,15 @@

		namespace Symfony\Component\Form\Extension\DataCollector;

		use Symfony\Component\Form\Extension\DataCollector\FormDataExtractorInterface;

[HttpKernel] Use VarDumper in the profiler #19614

[HttpKernel] Use VarDumper in the profiler #19614

Conversation

wouterj commented Aug 14, 2016

Summary

Screenshots

Further Improvements

Choose a reason for hiding this comment

fabpot commented Aug 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Koc commented Aug 14, 2016

wouterj commented Aug 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented Aug 15, 2016

yceruto commented Aug 15, 2016 • edited Loading

nicolas-grekas Aug 16, 2016 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Aug 17, 2016

wouterj commented Aug 17, 2016

weaverryan commented Aug 17, 2016

wouterj commented Aug 17, 2016

nicolas-grekas commented Aug 18, 2016 • edited Loading

nicolas-grekas commented Aug 19, 2016

wouterj commented Aug 20, 2016

nicolas-grekas commented Aug 20, 2016

Choose a reason for hiding this comment

stof commented Aug 26, 2016

fabpot commented Sep 14, 2016

wouterj commented Sep 17, 2016

nicolas-grekas commented Sep 17, 2016 • edited Loading

wouterj commented Sep 17, 2016

nicolas-grekas commented Sep 17, 2016

fabpot commented Sep 17, 2016

wouterj commented Aug 14, 2016 •

edited

Loading

yceruto commented Aug 15, 2016 •

edited

Loading

nicolas-grekas Aug 16, 2016 •

edited

Loading

nicolas-grekas commented Aug 18, 2016 •

edited

Loading

nicolas-grekas commented Sep 17, 2016 •

edited

Loading