Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[VarDumper] Allow dd() to be called without arguments#28317
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ro0NL commentedAug 30, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
i like the required var as a function semantic =/ Not sure |
mvannes commentedAug 30, 2018
But isn't this just calling |
nicolas-grekas commentedAug 30, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for submitting, that's indeed a difference with the previous Laravel's dd() function. |
SjorsO commentedAug 30, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
With Laravel's helper you could move around an empty Also, allowing it to be called with no arguments has the (very) minor advantage that programs alway die when they call Apart from making the function less semantic (which i'd argue doesn't make a difference in this case), is there a good reason to not allow |
ro0NL commentedAug 30, 2018
Hm that's a strong argument actually :) |
curry684 commentedAug 30, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Apart from that the generic code now after the second commit is also cleaner. |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok, looks like you have some supporters :)
nicolas-grekas commentedAug 31, 2018
What about making this a bit more useful and make it dump the stack trace instead of nothing? diff --git a/src/Symfony/Component/VarDumper/Caster/ExceptionCaster.php b/src/Symfony/Component/VarDumper/Caster/ExceptionCaster.phpindex f3af638d94..35f2f2b63a 100644--- a/src/Symfony/Component/VarDumper/Caster/ExceptionCaster.php+++ b/src/Symfony/Component/VarDumper/Caster/ExceptionCaster.php@@ -111,7 +111,7 @@ class ExceptionCaster public static function castTraceStub(TraceStub $trace, array $a, Stub $stub, $isNested) {- if (!$isNested) {+ if (!$isNested && !$trace->castWhenRoot) { return $a; } $stub->class = '';diff --git a/src/Symfony/Component/VarDumper/Caster/TraceStub.php b/src/Symfony/Component/VarDumper/Caster/TraceStub.phpindex 5eea1c8766..c3c51bb1ef 100644--- a/src/Symfony/Component/VarDumper/Caster/TraceStub.php+++ b/src/Symfony/Component/VarDumper/Caster/TraceStub.php@@ -24,13 +24,15 @@ class TraceStub extends Stub public $sliceOffset; public $sliceLength; public $numberingOffset;+ public $castWhenRoot;- public function __construct(array $trace, bool $keepArgs = true, int $sliceOffset = 0, int $sliceLength = null, int $numberingOffset = 0)+ public function __construct(array $trace, bool $keepArgs = true, int $sliceOffset = 0, int $sliceLength = null, int $numberingOffset = 0, bool $castWhenRoot = false) { $this->value = $trace; $this->keepArgs = $keepArgs; $this->sliceOffset = $sliceOffset; $this->sliceLength = $sliceLength; $this->numberingOffset = $numberingOffset;+ $this->castWhenRoot = $castWhenRoot; } }diff --git a/src/Symfony/Component/VarDumper/Resources/functions/dump.php b/src/Symfony/Component/VarDumper/Resources/functions/dump.phpindex 1ea3dc8434..46dbb3a434 100644--- a/src/Symfony/Component/VarDumper/Resources/functions/dump.php+++ b/src/Symfony/Component/VarDumper/Resources/functions/dump.php@@ -10,6 +10,7 @@ */ use Symfony\Component\VarDumper\VarDumper;+use Symfony\Component\VarDumper\Caster\TraceStub; if (!function_exists('dump')) { /**@@ -32,11 +33,13 @@ if (!function_exists('dump')) { } if (!function_exists('dd')) {- function dd($var, ...$moreVars)+ function dd(...$vars) {- VarDumper::dump($var);+ if (!$vars) {+ VarDumper::dump(new TraceStub(debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS), false, 0, null, 0, true));+ }- foreach ($moreVars as $var) {+ foreach ($vars as $var) { VarDumper::dump($var); } |
nicolas-grekas commentedAug 31, 2018
Could also be done for |
SjorsO commentedAug 31, 2018
I always liked the way the Laravel helper worked. If i put an empty I can imagine situations where it is useful to dump a stacktrace, but i'd prefer that to be a separate helper, or maybe a static method on the |
ro0NL commentedAug 31, 2018
What about something minimal, just a file/line ref of where |
SjorsO commentedAug 31, 2018
I like that idea. Dumping the last call (or maybe last 3 calls) of the stacktrace would be useful, and it would be clean enough to not flood the console. |
nicolas-grekas commentedAug 31, 2018
PR update welcome then (the number of frames can be limited thanks to debug_backtrace()'s 2nd argument) |
curry684 commentedAug 31, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I think that's generically useful, independent of whether parameters were supplied to always show it when invoked. Then followed by the sequential dumps of the parameters, if supplied. I wouldn't include more than 1 line of callstack as indeed it's not up to us whether to judge by default how many lines are relevant. 2 or 3 would also be arbitrarily either too much or too few for any specific situation. |
SjorsO commentedAug 31, 2018
curry684 commentedAug 31, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Looks good to me presentation-wise. Anyone care to chime in on my opinion that we should standardize this to always dump the call location and not "magically" change behavior based on parameters or not? |
SjorsO commentedAug 31, 2018
I think that for debug-only helpers like this being useful is most important, we shouldn't worry about semantics or "magic". |
curry684 commentedAug 31, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Why is showing the location of the break suddenly not useful anymore when also dumping parameters? "Magic" is not bad per se, we do a lot of awesome magic in Symfony like autowiring services. "Magic" is cool when it boils down to "it just works". "Magic" becomes bad when it's not consistent. |
SjorsO commentedAug 31, 2018
Oh, my bad. I thought you meant removing the ability to dump vars, i thought it was strange 😄 I'd prefer not changing the helper's behavior when it's given arguments, it has worked like this for a long time in Laravel, as far as i know everyone's happy with it. |
fancyweb commentedAug 31, 2018
+1 to curry684 suggestion to add this behavior to every dump() call. |
SjorsO commentedSep 3, 2018
I'd prefer to keep this PR like it is now. It only restores something i used to be able to do in Laravel, and doesn't change the behavior of other helpers. We can always open another PR to change the behavior of |
curry684 commentedSep 3, 2018
I didn't suggest changing Imho it should just be: Either do it never, or always. In |
SjorsO commentedSep 3, 2018
To keep the helper consistent, i've changed the PR to never make it print the backtrace. It now works exactly like the Laravel helper used to. |
curry684 commentedSep 3, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
That's fine with me, we can then have the discussion about changing the behavior elsewhere as it seems to have more implications than initially anticipated. (kicking off the other discussion: I'd also suggest actually making |
javiereguiluz commentedSep 6, 2018
I don't understand why are we trying to complicate things so much here 😕
Well, if I use
So doing the change proposed originally in this PR is the most consistent behavior in my opinion. There's no need to add stack traces or any other information. |
curry684 commentedSep 6, 2018
See the commits, it's been brought back to that state 😉 |
javiereguiluz commentedSep 6, 2018
Great! |
nicolas-grekas left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(I'll propose displaying file+line info in a separate PR)
javiereguiluz commentedSep 21, 2018
@SjorsO could you please fix the merge conflict to make this PR mergeable again? Thanks! |
SjorsO commentedSep 21, 2018
@javiereguiluz done |
nicolas-grekas commentedSep 21, 2018
Thank you@SjorsO. |
…(SjorsO)This PR was merged into the 4.2-dev branch.Discussion----------[VarDumper] Allow dd() to be called without arguments| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |**Description**A while back the `dd()` helper was [added to the VarDumper component](#26970) which was (i think) inspired by Laravel's `dd()` helper. Laravel has [removed their version of the helper](laravel/framework#25087) in favor of the helper in Symfony.However, as opposed to the Laravel helper, the Symfony helper requires at least one argument. Calling the Laravel helper with no arguments simply killed the program (and usually showed a white screen), calling the Symfony helper with no arguments throws a `TypeError: Too few arguments to function dd()` exception (which gets rendered by the error handler and fills the whole screen with useless information).Being able to call the `dd()` helper with no arguments is useful because it is a quick way to tell you if your code reaches a certain point. If it does, you can fill in the `dd()` with variables to keep debugging.This PR allows the dd helper to be called without arguments.This PR also makes the helper call `die` instead of `exit` to better reflect the function name 😄Commits-------a73dfad [VarDumper] Allow dd() to be called without arguments



Description
A while back the
dd()helper wasadded to the VarDumper component which was (i think) inspired by Laravel'sdd()helper. Laravel hasremoved their version of the helper in favor of the helper in Symfony.However, as opposed to the Laravel helper, the Symfony helper requires at least one argument. Calling the Laravel helper with no arguments simply killed the program (and usually showed a white screen), calling the Symfony helper with no arguments throws a
TypeError: Too few arguments to function dd()exception (which gets rendered by the error handler and fills the whole screen with useless information).Being able to call the
dd()helper with no arguments is useful because it is a quick way to tell you if your code reaches a certain point. If it does, you can fill in thedd()with variables to keep debugging.This PR allows the dd helper to be called without arguments.
This PR also makes the helper call
dieinstead ofexitto better reflect the function name 😄