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

[Console] Add dumper #28898

Merged
merged 1 commit into from Mar 24, 2019

Conversation

Projects
None yet
8 participants
@ro0NL
Copy link
Contributor

commented Oct 17, 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 symfony/symfony-docs#10502

This PR adds a new Dumper helper in the Console component. As there are 2 types of dumps

  • debug purpose (e.g. dd(), dump())
  • output purpose (see #24208, #27684)

For the latter we cannot use the global system (debug) dumper, i.e. VarDumper::dump(), we need something tied to the current output and dependency free. Here it is:

$io = new SymfonyStyle($input, $output);
$dumper = new Dumper($io);

$io->writeln($dumper([-0.5, 0, 1]));
$io->writeln($dumper(new \stdClass()));
$io->writeln($dumper(123));
$io->writeln($dumper('foo'));
$io->writeln($dumper(null));
$io->writeln($dumper(true));

With VarDumper comonent:

image

Without:

image

#27684 (comment) var-dumper is not a mandatory dep of fwb, can we do without?

Now we can :)

@ro0NL ro0NL force-pushed the ro0NL:console-dump branch from d6ee11f to 5cd4832 Oct 17, 2018

@ro0NL ro0NL force-pushed the ro0NL:console-dump branch 2 times, most recently from 6aaa769 to 2834905 Oct 20, 2018

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 20, 2018

@ro0NL ro0NL force-pushed the ro0NL:console-dump branch 2 times, most recently from 4960c68 to 8fe2722 Oct 20, 2018

@nicolas-grekas
Copy link
Member

left a comment

(a test case would be nice)

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2018

for tests i like to embed/wait for #28931, so the fallback would be tested using @class-not-exists CliDumper

@ro0NL ro0NL force-pushed the ro0NL:console-dump branch 7 times, most recently from 96fcf47 to 42636cc Oct 27, 2018

@lyrixx

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

I already thought about something like that. IMHO the issue is somewhere else.
We taught us we should not let some debugging tools (var_dump / dump) in our code and I agree with that.

This PR solves this this issue only in CLI, and only in the main entry point (the Command class).
Almost all my commands have a very few line of code, because everything live in services. So now, I don't use Output::writeln anymore, but instead Logger::log. So I will not be able to leverage this new feature.

So here we go: Many other framework / language have a trace log level. I know we are a bit limited by PSR, but IMHO the way to go is to add a new log level, where we can dump many things

@lyrixx

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

BTW, for now you can aleady achieve this with $logger->debug('Foo var', ['foo' => $foo']).
See symfony/monolog-bundle#297 for more confiiguration

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

@lyrixx understood

Almost all my commands have a very few line of code, because everything live in services.

Also there might be e.g. "debug-commands" with lots of code / output control. My main goal here was to obtain the dump value so i can put it in a console table, and have DX friendly output (#24208, #27684)

for now you can aleady achieve this with $logger->debug('Foo var', ['foo' => $foo'])

Im not sure using the logger is possible in my case, im looking mostly for $pretty = $dumper($value);.

So maybe get rid of Dumper::dump + dumpln at least, to avoid coupling with output control here.

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

This PR solves this this issue only in CLI

True, if we get rid of dump() / dumpln() it practically becomes a util to convert arbitrary values into pretty printable ones (with fallback support! that's still the sold feature)

In that case putting it in VarDumper might be a better place 🤔

edit: no, if we move it to vardumper we dont need fallback support :D the whole idea was to NOT require var-dumper, but use it if available.

@lyrixx

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

#24208 and #27684 are valid use case for this PR. I'm 👍 with it. Thanks

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

Cool :) tend to keep dump() / dumpln() actually, it's tied to console output; but we need that to know about colorization (maybe keep that dependency optional?)

Unless we prefer $output->writeln($dumper($var)) over $dumper->dumpln($var). Let me know.

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

I think I prefer composition, so $output->writeln($dumper($var)) would be my personal preference.

@ro0NL ro0NL force-pushed the ro0NL:console-dump branch from 216c475 to e95041b Mar 22, 2019

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

i dont understand the failures yet, running a single test works as expected (--filter=testFallbackInvoke), but running the DumerTest fully it breaks the mocking.

In this case class_exists behaves very weird :/

var_dump(
    class_exists::class,
    class_exists(CliDumper::class),
    \Symfony\Component\Console\Helper\class_exists(CliDumper::class),
    class_exists(CliDumper::class)
);

// Symfony\Component\Console\Helper\class_exists"
// bool(true) <-- unexpected
// bool(false)
// bool(false)

edit: i've split the tests for now into 2 individual suites, this seems to overcome the state issue.

@ro0NL ro0NL force-pushed the ro0NL:console-dump branch from e61e79c to feb4a98 Mar 22, 2019

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

Green&ready 👍

status: needs review

@fabpot

fabpot approved these changes Mar 22, 2019

@ro0NL ro0NL force-pushed the ro0NL:console-dump branch from d701847 to 41c0ceb Mar 22, 2019

@fabpot fabpot force-pushed the ro0NL:console-dump branch from 41c0ceb to fc7465c Mar 24, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

Thank you @ro0NL.

@fabpot fabpot merged commit fc7465c into symfony:master Mar 24, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 24, 2019

feature #28898 [Console] Add dumper (ro0NL)
This PR was squashed before being merged into the 4.3-dev branch (closes #28898).

Discussion
----------

[Console] Add dumper

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#10502

This PR adds a new `Dumper` helper in the Console component. As there are 2 types of dumps

- debug purpose (e.g. `dd()`, `dump()`)
- output purpose (see #24208, #27684)

For the latter we cannot use the global system (debug) dumper, i.e. `VarDumper::dump()`, we need something tied to the current output and dependency free. Here it is:

```php
$io = new SymfonyStyle($input, $output);
$dumper = new Dumper($io);

$io->writeln($dumper([-0.5, 0, 1]));
$io->writeln($dumper(new \stdClass()));
$io->writeln($dumper(123));
$io->writeln($dumper('foo'));
$io->writeln($dumper(null));
$io->writeln($dumper(true));
```

With VarDumper comonent:

![image](https://user-images.githubusercontent.com/1047696/47069483-4cc26f80-d1ef-11e8-902e-2f9b0f040f25.png)

Without:

![image](https://user-images.githubusercontent.com/1047696/47069517-6663b700-d1ef-11e8-9328-ae1db0b83d7e.png)

> #27684 (comment) var-dumper is not a mandatory dep of fwb, can we do without?

Now we can  :)

Commits
-------

fc7465c [Console] Add dumper

@ro0NL ro0NL deleted the ro0NL:console-dump branch Mar 24, 2019

@ro0NL ro0NL referenced this pull request Mar 24, 2019

Merged

[Form][Console] Use dumper #30666

fabpot added a commit that referenced this pull request Mar 25, 2019

feature #30666 [Form][Console] Use dumper (ro0NL)
This PR was merged into the 4.3-dev branch.

Discussion
----------

[Form][Console] Use dumper

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Continuation of #28898 for `debug:form`

Commits
-------

a94228e [Form][Console] Use dumper

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

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.