NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[VarDumper][HttpKernel] Add context to all dumps#28395

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

fancyweb wants to merge1 commit intosymfony:masterfromfancyweb:feat-add-context-to-all-dumps

Closed

[VarDumper][HttpKernel] Add context to all dumps#28395

fancyweb wants to merge1 commit intosymfony:masterfromfancyweb:feat-add-context-to-all-dumps

Conversation

Copy link

Contributor

fancyweb commentedSep 7, 2018

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

Following some discussion in#28317, I think it would be nice if the defaultVarDumper handler would add where thedump calls comes from. This is the case when theDebugBundle is enabled on html dumps. The behavior I added is exactly the same.

I also added it for cli dumps, but for those one I think it's actually better to display the file path instead of just the class name as sometimes you have multiple classes with the same name. This is not a problem for html dumps since there is a title attribute on the span or on the link.

carsonbot added Status: Needs Review VarDumper HttpKernel Feature labels

Sep 7, 2018

[VarDumper][HttpKernel] Add context to all dumps

2f49e78

fancyweb force-pushed thefeat-add-context-to-all-dumps branch from2807858 to2f49e78Compare

September 7, 2018 18:54

fancyweb commented

Sep 7, 2018

View reviewed changes

src/Symfony/Component/VarDumper/VarDumper.php

		$name =$file;
		}

		$this->line =$this->style('meta',$name,$attr).' on line'.$this->style('meta',$line).':';

Copy link

ContributorAuthor

fancywebSep 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

In this scenario, we are 100% sure that the dumper is an instance ofCliDumper so those methods are safe to use.

Copy link

Member

nicolas-grekas commentedSep 8, 2018•
edited
Loading

I'd prefer not doing this change personally: when debugging, it's already hard enough to figure out what's going on, I'd prefer not having to visually parse extra output...

Copy link

ContributorAuthor

fancyweb commentedSep 8, 2018

Here is how it renders :
CLI

HTML

nicolas-grekas modified the milestones:2.8,next

Sep 8, 2018

Copy link

Member

nicolas-grekas commentedSep 9, 2018•
edited
Loading

The location of the dump is already printed in the dump-server and when the toolbar is used, isn't it?
When debugging e.g. CLI apps, I often end up using dump() in loops. Personally, I'd prefer not having the file+line in the output, as it would mostly be noise to my eyes.
Note that whendd() is used, I'd welcome this change, because, by definition, this will be printed only once.

Copy link

ContributorAuthor

fancyweb commentedSep 9, 2018•
edited
Loading

When you only use theVarDumper component, no location is printed at all. So it would be an improvement for everyone using this component without Symfony, which is currently my case (working on Drupal) ^^

When you use theDebugBundle, a new handler is registered, so this new code will never be used and indeed the location is displayed in the toolbar, in the profiler, in the dump itself (if you die), and in the dump server.

For the cli dumps, I can understand and agree with the loop example. However, currently, when you use the dump server, 7 lines and displayed before each dump. Example with a loop :