Skip to content

[Console] Add dumper #28898

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

Merged
merged 1 commit into from
Mar 24, 2019
Merged

[Console] Add dumper #28898

merged 1 commit into from
Mar 24, 2019

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL 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

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

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 20, 2018
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.

(a test case would be nice)

@ro0NL
Copy link
Contributor Author

ro0NL commented Oct 20, 2018

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

fabpot added a commit that referenced this pull request Dec 10, 2018
This PR was squashed before being merged into the 4.3-dev branch (closes #28931).

Discussion
----------

[PhpUnitBridge] Added ClassExistsMock

| 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#10528

I've thought about this before, and bumped into it again when trying to test #28898

This PR allows to mock `class|interface|trait_exists` to enable specific code path testing

Commits
-------

62caec1 [PhpUnitBridge] Added ClassExistsMock
@fabpot
Copy link
Member

fabpot commented Mar 4, 2019

#28931 was merged a while ago, @ro0NL can we move forward here?

@lyrixx
Copy link
Member

lyrixx 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
Copy link
Member

lyrixx 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
Copy link
Contributor Author

ro0NL 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
Copy link
Contributor Author

ro0NL 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
Copy link
Member

lyrixx commented Mar 4, 2019

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

@ro0NL
Copy link
Contributor Author

ro0NL 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
Copy link
Member

fabpot commented Mar 22, 2019

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

@ro0NL
Copy link
Contributor Author

ro0NL 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
Copy link
Contributor Author

ro0NL commented Mar 22, 2019

Green&ready 👍

status: needs review

@fabpot
Copy link
Member

fabpot commented Mar 24, 2019

Thank you @ro0NL.

@fabpot fabpot merged commit fc7465c into symfony:master Mar 24, 2019
fabpot added a commit that referenced this pull request Mar 24, 2019
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 console-dump branch March 24, 2019 10:34
fabpot added a commit that referenced this pull request Mar 25, 2019
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 mentioned this pull request May 9, 2019
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.

8 participants