Skip to content
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

[HttpKernel] Use VarDumper in the profiler #19614

Merged
merged 2 commits into from Sep 17, 2016

Conversation

Projects
None yet
9 participants
@wouterj
Copy link
Member

commented Aug 14, 2016

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

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)
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);

This comment has been minimized.

Copy link
@wouterj

wouterj Aug 14, 2016

Author Member

not sure if this makes sense:

  • Strings are still perfectly fine
  • Deprecations aren't shown, as profiler pages aren't data-collected afaik
@@ -132,6 +132,30 @@ HttpKernel
have your own `ControllerResolverInterface` implementation, you should
inject an `ArgumentResolverInterface` instance.

* `DataCollector::varToString()` ciwhas been removed in favor of `cloneVar()`.

This comment has been minimized.

Copy link
@fabpot
$this->maxDepth,
$this->maxItemsPerDepth,
$this->useRefHandles,
));

This comment has been minimized.

Copy link
@fabpot

fabpot Aug 14, 2016

Member

I would put everything on one line.

$this->maxDepth,
$this->maxItemsPerDepth,
$this->useRefHandles
) = unserialize($str);

This comment has been minimized.

Copy link
@fabpot

fabpot Aug 14, 2016

Member

I would put everything on one line.

@fabpot

This comment has been minimized.

Copy link
Member

commented Aug 14, 2016

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

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

This comment has been minimized.

Copy link
@Koc

Koc Aug 14, 2016

Contributor

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

This comment has been minimized.

Copy link
@wouterj

wouterj Aug 14, 2016

Author Member

Proposed a way to do this: #19616

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Aug 15, 2016

Member

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

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2016

ref #18149, #18074

@wouterj

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2016

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:

sf-profiler-dumper-vertical-height

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Aug 15, 2016

Member

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

This comment has been minimized.

Copy link
@Koc

Koc Aug 15, 2016

Contributor

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

This comment has been minimized.

Copy link
@wouterj

wouterj Aug 15, 2016

Author Member

reverted this change, it was also causing build errors.

@wouterj wouterj force-pushed the wouterj:profiler_dumper branch from 8010483 to 6e092ef Aug 15, 2016

@wouterj

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2016

I don't understand the PHP 7.1 failures.

@yceruto

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2016

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

/**
* {@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 comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Aug 16, 2016

Member

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

This comment has been minimized.

Copy link
Member

commented Aug 17, 2016

See suggestions as patch directly in nicolas-grekas#6

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

@wouterj wouterj force-pushed the wouterj:profiler_dumper branch from 6e092ef to e6cc848 Aug 17, 2016

@wouterj

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2016

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

This comment has been minimized.

Copy link
Member

commented Aug 17, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2016

@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

This comment has been minimized.

Copy link
Member

commented Aug 18, 2016

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 #19568)
  • properly remove sanitizeContext: VarDumper does the job.

fabpot added a commit that referenced this pull request Aug 19, 2016

feature #19672 [VarDumper] Allow dumping subparts of cloned Data stru…
…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

This comment has been minimized.

Copy link
Member

commented Aug 19, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2016

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

This comment has been minimized.

Copy link
Member

commented Aug 20, 2016

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?

{
$this->valueExporter = $valueExporter ?: new ValueExporter();
if (null !== $valueExporter && $triggerDeprecationNotice) {

This comment has been minimized.

Copy link
@stof

stof Aug 26, 2016

Member

in which case do we need to avoid the deprecation ? The ValueExporter is ignored now, so we could stop injecting it in the core.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Aug 26, 2016

Member

The ValueExporter needs to be injected from a FrameworkBundle pov to keep BC with symfony/form < 3.2, thus the current logic

++$this->stackLevel;
}

public function leave(\Twig_Profiler_Profile $profile)

This comment has been minimized.

Copy link
@stof

stof Aug 26, 2016

Member

What is the use case for that ?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Aug 26, 2016

Member

The use case is changing the output stream of the HtmlDumper so that it resets its headerIsDumped internal state and can be reused after use.

/**
* Twig extension for the profiler.
*
* @author Fabien Potencier <fabien@symfony.com>
*/
class WebProfilerExtension extends \Twig_Extension
class WebProfilerExtension extends \Twig_Extension_Profiler

This comment has been minimized.

Copy link
@stof

stof Aug 26, 2016

Member

this looks wrong to me, because it will register the twig profiler token parsers twice (and this extension will register them without the Stopwatch integration, which means it is bad if this extension wins)

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Aug 26, 2016

Member

this is required, and there is no conflict: the two inserted nodes won't be the same because they get the extension name as first parameter, which make them different. see comment below for the reason why we need to track enter/leave here.

@stof

This comment has been minimized.

Copy link
Member

commented Aug 26, 2016

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

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

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 wouterj force-pushed the wouterj:profiler_dumper branch from a6774cb to fa41e63 Sep 17, 2016

@wouterj

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2016

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

This comment has been minimized.

Copy link
Member

commented Sep 17, 2016

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 wouterj force-pushed the wouterj:profiler_dumper branch from fa41e63 to eddecbd Sep 17, 2016

@wouterj

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2016

thanks, applied the changes

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 17, 2016

👍

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 17, 2016

Thank you @wouterj.

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

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Sep 17, 2016

feature #19614 [HttpKernel] Use VarDumper in the profiler (wouterj, n…
…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

@wouterj wouterj deleted the wouterj:profiler_dumper branch Sep 17, 2016

@lunetics

This comment has been minimized.

@wouterj This somehow brakes on (in my case) the serialized security session info. $var has the value:

_sf2_attributes|a:4:{s:15:"lunetics_locale";s:5:"de_DE";s:14:"_security_main";s:819:"C:74:"Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken":731:{a:3:{i:0;N;i:1;s:4:"main";i:2;s:691:"a:4:{i:0;C:80:"Foo\IdentityAccess\Infrastructure\DomainSecurityBundle\Model\SecurityUser":431:{a:1:{i:0;O:62:"Foo\IdentityAccess\Domain\Model\Security\Authentication":3:{s:72:"�Foo\IdentityAccess\Domain\Model\Security\Authentication�username";s:5:"1test";s:76:"�Foo\IdentityAccess\Domain\Model\Security\Authentication�passwordHash";s:60:"$2y$12$NeSpSXA6hh4L43uBm98zeOnUwgns9dMD07rRX1rMbyGM3sKcrSrjq";s:69:"�Foo\IdentityAccess\Domain\Model\Security\Authentication�roles";a:1:{i:0;s:9:"ROLE_USER";}}}}i:1;b:1;i:2;a:1:{i:0;O:41:"Symfony\Component\Security\Core\Role\Role":1:{s:47:"�Symfony\Component\Security\Core\Role\Role�role";s:9:"ROLE_USER";}}i:3;a:0:{}}";}}";s:21:"_csrf/forgot_password";s:43:"7URiUPlKw_eMmVFxl6XKoyUqYqvkdotPtWVHko0cF1c";s:22:"_csrf/register_an_user";s:43:"N-d4CWgu3QHRb7HIcmvdSiwo26oMSrxWkfkBt6CMH6o";}_sf2_flashes|a:0:{}_sf2_meta|a:3:{s:1:"u";i:1475973053;s:1:"c";i:1475972045;s:1:"l";s:1:"0";}

I get an php error:

php.CRITICAL: Uncaught Warning: file_exists() expects parameter 1 to be a valid path, string given {"exception":"[object] (Symfony\\Component\\Debug\\Exception\\ContextErrorException(code: 0): Warning: file_exists() expects parameter 1 to be a valid path, string given at /usr/sites/metalflirt/webapp/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/DataCollector/DataCollector.php:133)"} {"request_id":"93159b6f26b17bde83356424b5520983"} ```

This comment has been minimized.

Copy link

replied Oct 9, 2016

Forgot to add that happens with the symfony "throw" warning settings:

framework:
 php_errors:
        throw: true

Maybe the the check could be done somewhat more sensible? took some time to debug :) (outcome was just a 404 for the profiler / token)

@fabpot fabpot referenced this pull request Oct 27, 2016

Merged

Release v3.2.0-BETA1 #20317

fabpot added a commit that referenced this pull request Dec 2, 2016

bug #20700 [WebProfilerBundle][Translator] Fix TranslationDataCollect…
…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

ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018

bug symfony#20700 [WebProfilerBundle][Translator] Fix TranslationData…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.