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

[VarDumper] Add an options builder to configure VarDumper's behavior at runtime #48667

Open
wants to merge 1 commit into
base: 7.2
Choose a base branch
from

Conversation

alexandre-daubois
Copy link
Contributor

@alexandre-daubois alexandre-daubois commented Dec 15, 2022

Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #48474
License MIT
Doc PR symfony/symfony-docs#17787

Following up #48432, this PR brings an options builder to configure VarDumper's behavior thanks to a special _options named argument. Here is a snippet of it in action and a screenshot of the result. Feedbacks on UI are really welcomed, especially about the debug backtrace!

#[AsController]
#[Route('/')]
class FooController
{
    public function __invoke(): Response
    {
        $options = (new VarDumperOptions())
            ->trace()
            ->stringLength()
        ;

        dump(var: ['Hi', 'Greetings' => 'Good morning, Symfony!'], _options: $options);
        dump('Classic dump');
        dd(['Options are cloned!'], named: 'This is named', _options: (clone $options)->lightArray()->traceLimit(2));

        throw new \LogicException('This code should never be reached!');
    }
}

Capture d’écran 2022-12-12 à 21 38 20

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this, that's a complex feature because the matrix of possibilities is huge.

Here are some random thoughts:

  • I like the style proposed in the issue: dump()->trace()->maxDepth(4)->values($var);. I'm just wondering if values() shouldn't be named dump() or var() or ?
  • I also like the dump($var, _trace: true) style, which could work well with autocompletion if we describe the options as virtual arguments on the function? array{_trace: bool, ...}
  • instead of trace_limit, what about trace: bool|int?
  • have a look at FrameStub/TraceStub for dumping traces
  • we might want to provide a way to define the default options on the VarDumper class?

@lyrixx
Copy link
Member

lyrixx commented Dec 16, 2022

  • I like the style proposed in the issue: dump()->trace()->maxDepth(4)->values($var);. I'm just wondering if values() shouldn't be named dump() or var() or ?

As states by @derrabus, this cannot work


I don't really like the current object API:

$options = (new VarDumperOptions())
    ->trace()
    ->stringLength()
;

it's too long to type. We should not forgot we are a in debug context. So we want to be able to get a result ASAP. that's why we have the function dd(). As short as possible.

That's why I prefer the second bullet point of @nicolas-grekas comment:

dd($this, _trace: true, _maxDepth: 4);

@nicolas-grekas
Copy link
Member

#48474 (comment), this cannot work

yes it can, see #48474 (comment) ;)

@lyrixx
Copy link
Member

lyrixx commented Dec 16, 2022

This is ugly :) I really don't like it. And it's too slow to type.

@ro0NL
Copy link
Contributor

ro0NL commented Dec 16, 2022

how is a fully autodiscoverable dump()->... too slow to type?

@lyrixx
Copy link
Member

lyrixx commented Dec 16, 2022

@ro0NL I mixed my thought :/ I was talking about:

$options = (new VarDumperOptions())
    ->trace()
    ->stringLength()
;

dump($this, _options: $options);

@ro0NL
Copy link
Contributor

ro0NL commented Dec 16, 2022

I'm just wondering if values() shouldn't be named dump() or var() or ?

maybe __invoke works, IIUC we could do

dump(1,2,3);
dump()(1,2,3)->trace();
dump()->trace()(1,2,3);

@alexandre-daubois
Copy link
Contributor Author

I agree the current object API is a bit much to type. I am 👍 to find a better solution.

Should I try to implement both @GromNaN solution (dump returning mixed|VarDumperOptions) and @nicolas-grekas' one, together? I wonder if it would be a bit disturbing to have both ways available, and I don't have, yet, a strong opinion for one solution over the other.

Also, I really like the solution proposed by @ro0NL. If I may find a drawback, I'm afraid this syntax could be difficult to be "discovered" by yourself without reading the documentation.

@GromNaN
Copy link
Member

GromNaN commented Dec 16, 2022

The syntax that don't work is the one that rely on __destruct.
The last suggestion mentioned by @nicolas-grekas is dump()->trace()->maxDepth(4)->dump($var) which returns the $var as expected.

@alexandre-daubois
Copy link
Contributor Author

alexandre-daubois commented Dec 17, 2022

I updated the feature. The new version allows you to do both ways:

// Passing options as named parameters
dump('Values', _trace: 5, _flags: AbstractDumper::DUMP_STRING_LENGTH);

// Use the options builder syntax
dump()
    ->trace(5)
    ->stringLength()
    ->dd('Values') // or ->dump('Values')
;

Still investigating on how to use TraceStub. Otherwise, this works well! And it is so much cleaner and quicker to type.

Also, I made the choice to only have the options builder syntax on dump, not dd. I did this, because I think it is, in my opinion, a good thing to keep never return type on dd (because never|VarDumpOptions is obviously a forbidden return type). Of course, passing options as named parameters to dd also works!

@alexandre-daubois alexandre-daubois force-pushed the feat/var-dumper-options branch 2 times, most recently from 1b933a2 to c8f0cd9 Compare December 19, 2022 13:00
@alexandre-daubois
Copy link
Contributor Author

I've been able to use TraceStub to display the backtrace. Here is the result:

image

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

About the default options, I'd suggest configuring them via a VAR_DUMPER_OPTIONS env var. I'd go with a url-encoded string so that it's easy to turn into an options array

src/Symfony/Bundle/DebugBundle/composer.json Outdated Show resolved Hide resolved
src/Symfony/Component/VarDumper/CHANGELOG.md Outdated Show resolved Hide resolved
src/Symfony/Component/VarDumper/Dumper/HtmlDumperTheme.php Outdated Show resolved Hide resolved
}

if (self::requiresRegister($options)) {
self::register($options);
Copy link
Member

Choose a reason for hiding this comment

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

this will change the options of the default handler when any options changes?
this would be unexpected to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

requiresRegister helps on taking care of registering again the handler if dump/dd is not called with the exact same set of options as the previous call (this should work great with options defined in an env var!). So if you call the default dumper (I assume what you called "default dumper" is the one called without any special option? I may be wrong 😄), it should be registered again with its default options.

I struggled a bit on this part, only a few options need a full and new registration. I may be missing something?

@dunglas
Copy link
Member

dunglas commented Dec 22, 2022

When dumping large objects, it could be nice to be able to have the possibility to dump only one or several properties instead of the full object. Something like: _path: 'foo.bar' and _path: ['foo.bar', 'baz.bat'] maybe?

@alexandre-daubois
Copy link
Contributor Author

alexandre-daubois commented Dec 23, 2022

@nicolas-grekas thank you for the review, I'll check this out! The env var is also a great idea too, in my opinion.
Edit: the env var mechanism is in!

@dunglas I really like your proposition! Giving the size of this PR, I think it would deserve a separate one. Definitely keeping your idea this in mind 👍

@alexandre-daubois
Copy link
Contributor Author

Tests are fixed and PR is rebased 🙂 (failing tests seem unrelated to this)

Copy link
Contributor

@alamirault alamirault left a comment

Choose a reason for hiding this comment

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

Thanks Alexandre for this PR, I really like syntax with named parameters !

@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@alexandre-daubois alexandre-daubois force-pushed the feat/var-dumper-options branch 3 times, most recently from 3a9b3e9 to 74b5287 Compare July 18, 2023 08:29
@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@butschster
Copy link

butschster commented May 2, 2024

Hey @alexandre-daubois! We're currently working on Buggregator, and it can be used as a server for displaying dumps for the var-dumper component. I really like your PR with options. However, it would be great if you also provided the ability to add custom context as an array to a dump function.

$options = (new VarDumperOptions())
    ->withContext(['language' => 'php'])
;

dump($this, _options: $options);

This feature can be used in various scenarios, such as providing information about syntax highlighting for string dumps or offering details about the project to which a dump should be linked.

Would you kindly consider adding support for custom context in the dump function

Thanks for PR!

@alexandre-daubois
Copy link
Contributor Author

Hey @butschster! Thank you for your interest in this PR! I kinda forgot about it, but I still think it is worth it to have this in the framework.

About the context feature you're talking about: while I understand it would help in your case, I find it weird to add some context here that won't be used apart from super precise use cases. Two solutions here:

  • If you have some strong reasons and use case, a complementary PR could be proposed ;
  • We may keep VarDumpOptions not final so you can extend it for all your needs. I think that's the quick win 😊

Anyway, I'll try finalize this PR. Thanks for digging it up!

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

Successfully merging this pull request may close these issues.

Add options to dump function
10 participants