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] Introduce a new way to collect dumps through a server dumper #23831

Merged
merged 3 commits into from Mar 23, 2018

Conversation

@ogizanagi
Member

ogizanagi commented Aug 8, 2017

Q A
Branch? 4.1
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #22987
License MIT
Doc PR TODO symfony/symfony-docs#9474

Could also be interesting as alternate solution for #23442.

Inspired by the ServerLogHandler and @nicolas-grekas's comment in #22987.


Description

This PR introduces a new server:dump command available in the VarDumper component.
Conjointly with a new ServerDumper data dumper, it allows to send serialized Data clones to a single centralized server, in charge of dumping them on CLI output, or in an file in HTML format.

Showcase

HTTP calls

For instance, when working on an API and dumping something, you might end up with a mix of your response and the CLI dumped version of the data you asked:

class HelloController extends AbstractController
{
    /**
     * @Route("/hello")
     */
    public function hello(Request $request, UserInterface $user)
    {
        dump($request->attributes, $user);

        return JsonResponse::create([
            'status' => 'OK',
            'message' => "Hello {$user->getUsername()}"
        ]);
    }
}

screenshot 2017-08-08 a 16 44 24

You might even get the HTML dump version under some conditions.

Dumping to a server allows collecting dumps in a separate place:

server-dump-http-to-cli

⚠️ By swallowing the previous dumpers output, the dump data collector is not active when running the dump server. Disable swallowing if you want both. ➜ Dumps are still collected in the profiler thanks to f24712e

CLI calls

The best probably is (to me) that you can also debug CLI applications...

server-dump-cli

...and get HTML formatted dumps:

server-dumper-cli-to-html-output

hence, benefit from all the features of this format (collapse, search, ...)

HTML output at a glance

screenshot 2017-08-11 a 19 28 25

The HTML output benefits from all the HtmlDumper features and contains extra informations about the context (sources, requests, command line, ...). It doesn't aim to replace the profiler for HTTP calls with the framework, but is really handy for CLI apps or by wiring it in your own web app, out of the framework usage.

CLI output at a glance

screenshot 2017-08-11 a 19 52 57

Usage within the framework

Config

For instance, in dev config:

#config/packages/dev/debug.yaml
debug:
    dump_destination: "tcp://%env(VAR_DUMPER_SERVER)%"

The configuration also allows to set a host option, but there is already a sensible default value (tcp://0.0.0.0:9912) so you don't have to deal with it.
In case you want to change the default host value used, simply use the VAR_DUMPER_SERVER env var.

When the server is running, all dumps are collected by the server and previous dumpers ones are "swallowed" by default. If you want both to collect dumps on the server AND keep previous dumpers on regular outputs, you can disable swallowing:

When the server isn't running or in case of failure to send the data clones to the server, the server dumper will delegates to the configured wrapped dumper, so dumps are displayed and collected as usual.

Running the server

bin/console server:dump [--format=cli|html]

Options

  • The output option defaults to null which will display dumps on CLI. It accepts a file path in which dumps will be collected in HTML format.

  • The format option allows to switch format to use. For instance, use the html format and redirect the output to a file in order to open it in your browser and inspect dumps in HTML format.

  • The default host value is the same as the one configured under the debug.server_dump.host config option, so you don't have to deal with it in most cases.
    In case you want to change the default host value used, simply use the VAR_DUMPER_SERVER env var (when using the debug bundle integration):

    VAR_DUMPER_SERVER=0.0.0.0:9204 bin/console server:dump

Manual wiring

If you want to wire it yourself in your own project or using it to inspect dumps as html before the kernel is even boot for instance:

$host = '0.0.0.0:9912';
$cloner = new VarCloner();
$dumper = new ServerDumper($host, new CliDumper());

VarDumper::setHandler(function ($var) use ($dumper, $cloner) {
    $dumper->dump($cloner->cloneVar($var));
});

Create your own server app

The component already provides a default server app by means of the ServerDumpCommand, but
you could also build your own by using the DumpServer:

$host = '0.0.0.0:9912';

$server = new DumpServer($host);

$server->start();

$server->listen(function (Data $data, array $context, $clientId) {
    // do something
});
@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Aug 8, 2017

Member

disabling dumps from the data collector by default may not be a good idea.

Member

stof commented Aug 8, 2017

disabling dumps from the data collector by default may not be a good idea.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Aug 8, 2017

Member

disabling dumps from the data collector by default may not be a good idea.

f24712e might be a solution to this.

Member

ogizanagi commented Aug 8, 2017

disabling dumps from the data collector by default may not be a good idea.

f24712e might be a solution to this.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Aug 11, 2017

Member

Last updates from yesterday (I've update the PR description & screens) with features I expected to add in a another PR, but here we are:

  • extracted the DumpServer in a dedicated class. So ServerDumpCommand is a server app consuming it and everyone could create its own app easily if he wants. So someone could imagine a full-featured app starting a dump server and exposing an http server to display the live stream of dumps, with more capabilities than the the simple html file provided in this PR.
  • introduced context providers as simple callables, which allows to add context to collected dumps.
  • enhanced the cli & html outputs with these new informations (cf screens).
  • mentioned how to manually wire the server dumper in the PR description.
Member

ogizanagi commented Aug 11, 2017

Last updates from yesterday (I've update the PR description & screens) with features I expected to add in a another PR, but here we are:

  • extracted the DumpServer in a dedicated class. So ServerDumpCommand is a server app consuming it and everyone could create its own app easily if he wants. So someone could imagine a full-featured app starting a dump server and exposing an http server to display the live stream of dumps, with more capabilities than the the simple html file provided in this PR.
  • introduced context providers as simple callables, which allows to add context to collected dumps.
  • enhanced the cli & html outputs with these new informations (cf screens).
  • mentioned how to manually wire the server dumper in the PR description.
Show outdated Hide outdated src/Symfony/Bundle/DebugBundle/DependencyInjection/Configuration.php
@@ -51,6 +51,15 @@ public function getConfigTreeBuilder()
->example('php://stderr')
->defaultNull()
->end()
->arrayNode('server_dump')->canBeEnabled()
->children()
->scalarNode('host')->defaultValue('tcp://0.0.0.0:9912')->end()

This comment has been minimized.

@fabpot

fabpot Aug 12, 2017

Member

I would not make this configurable in the config but as an env var. Like for the web server, imagine a scenario where you are working on many projects in parallel. It's easier to tweak an env var from the outside. Also, if the server is not a Symfony app, having the server address as an env var helps as well :)

@fabpot

fabpot Aug 12, 2017

Member

I would not make this configurable in the config but as an env var. Like for the web server, imagine a scenario where you are working on many projects in parallel. It's easier to tweak an env var from the outside. Also, if the server is not a Symfony app, having the server address as an env var helps as well :)

This comment has been minimized.

@ogizanagi

ogizanagi Aug 12, 2017

Member

Indeed, great idea. Done in b002175

@ogizanagi

ogizanagi Aug 12, 2017

Member

Indeed, great idea. Done in b002175

Show outdated Hide outdated src/Symfony/Component/VarDumper/Command/ServerDumpCommand.php
{
$this
->setName('server:dump')
->addOption('host', null, InputOption::VALUE_REQUIRED, 'The server host', $this->defaultHost)

This comment has been minimized.

@fabpot

fabpot Aug 12, 2017

Member

If you agree with the config removal, this should also read from the env var.

@fabpot

fabpot Aug 12, 2017

Member

If you agree with the config removal, this should also read from the env var.

This comment has been minimized.

@ogizanagi

ogizanagi Aug 12, 2017

Member

Option removed in favor of using the env var.

@ogizanagi

ogizanagi Aug 12, 2017

Member

Option removed in favor of using the env var.

Show outdated Hide outdated src/Symfony/Component/VarDumper/Dumper/ServerDumper.php
* use
* @param callable[] $contextProviders Callables indexed by name returning a context array
*/
public function __construct($host, DataDumperInterface $wrappedDumper = null, $swallows = true, array $contextProviders = array())

This comment has been minimized.

@fabpot

fabpot Aug 12, 2017

Member

$host would then accept an empty string, which would make it read the env var.

@fabpot

fabpot Aug 12, 2017

Member

$host would then accept an empty string, which would make it read the env var.

This comment has been minimized.

@ogizanagi

ogizanagi Aug 12, 2017

Member

I used null instead of an empty string for now. Any reason to favor the empty string?

@ogizanagi

ogizanagi Aug 12, 2017

Member

I used null instead of an empty string for now. Any reason to favor the empty string?

This comment has been minimized.

@fabpot

fabpot Aug 12, 2017

Member

null is perfect ofc

@fabpot

fabpot Aug 12, 2017

Member

null is perfect ofc

Show outdated Hide outdated src/Symfony/Component/VarDumper/Server/DumpServer.php
*/
class DumpServer
{
const HOST_ENV_VAR = 'VAR_DUMPER_SERVER';

This comment has been minimized.

@ro0NL

ro0NL Aug 12, 2017

Contributor

why not inject with %env()% and keep this at the config level..

@ro0NL

ro0NL Aug 12, 2017

Contributor

why not inject with %env()% and keep this at the config level..

This comment has been minimized.

@ogizanagi

ogizanagi Aug 12, 2017

Member

Yes, we could leverage Symfony's env support for this, hence no need for the previous commit. But the rational here to me is that you probably don't need to make the env var itself configurable and when working with multiple projects (even ones not using the DebugBundle), you don't have to ensure the same env var is used. Just set it and it works out of the box.

Still, that'll be the most flexible and would avoid hardcoding this behavior, while we could just configure in the standard-edition and flex recipe:

parameters:
    env(VAR_DUMPER_SERVER): 'tcp://0.0.0.0:9912'

debug:
    server_dump:
        host: '%env(VAR_DUMPER_SERVER)%'

(or directly in DebugBundle's services.xml, as stated below indeed. No need for the debug.server_dump.host option. But that'll still only work out of the box if you use the DebugBundle)

@fabpot : What do you think?

@ogizanagi

ogizanagi Aug 12, 2017

Member

Yes, we could leverage Symfony's env support for this, hence no need for the previous commit. But the rational here to me is that you probably don't need to make the env var itself configurable and when working with multiple projects (even ones not using the DebugBundle), you don't have to ensure the same env var is used. Just set it and it works out of the box.

Still, that'll be the most flexible and would avoid hardcoding this behavior, while we could just configure in the standard-edition and flex recipe:

parameters:
    env(VAR_DUMPER_SERVER): 'tcp://0.0.0.0:9912'

debug:
    server_dump:
        host: '%env(VAR_DUMPER_SERVER)%'

(or directly in DebugBundle's services.xml, as stated below indeed. No need for the debug.server_dump.host option. But that'll still only work out of the box if you use the DebugBundle)

@fabpot : What do you think?

This comment has been minimized.

@ro0NL

ro0NL Aug 12, 2017

Contributor

note we dont need debug.server_dump.host per se, it can be done directly in services.xml

@ro0NL

ro0NL Aug 12, 2017

Contributor

note we dont need debug.server_dump.host per se, it can be done directly in services.xml

This comment has been minimized.

@fabpot

fabpot Aug 14, 2017

Member

I don't see the need to change the env var name.

@fabpot

fabpot Aug 14, 2017

Member

I don't see the need to change the env var name.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Aug 21, 2017

Member

I've brought some new changes and added some tests. The PR is ready on my side.
Tough part is the wiring with the framework and the DumpDataCollector. Hopefully I did it right, as much as possible. 🤞

deps=high failure is expected.
I might need some hints on the HHVM & AppVeyor failures, somehow related to the PhpProcess usage (and wrong working dir)?

Fixed on HHVM build, probably not the best way. The failure still remains on AppVeyor but is different:

"stream_socket_client(): unable to connect to tcp://0.0.0.0:9912 (The requested address is not valid in its context.)"

Fixed on AppVeyor by setting VAR_DUMPER_SERVER to use 127.0.0.1 as AppVeyor seems to dislike 0.0.0.0

Member

ogizanagi commented Aug 21, 2017

I've brought some new changes and added some tests. The PR is ready on my side.
Tough part is the wiring with the framework and the DumpDataCollector. Hopefully I did it right, as much as possible. 🤞

deps=high failure is expected.
I might need some hints on the HHVM & AppVeyor failures, somehow related to the PhpProcess usage (and wrong working dir)?

Fixed on HHVM build, probably not the best way. The failure still remains on AppVeyor but is different:

"stream_socket_client(): unable to connect to tcp://0.0.0.0:9912 (The requested address is not valid in its context.)"

Fixed on AppVeyor by setting VAR_DUMPER_SERVER to use 127.0.0.1 as AppVeyor seems to dislike 0.0.0.0

@nicolas-grekas

that's interesting, I need to checkout and play with it for better feedback :)

Show outdated Hide outdated src/Symfony/Component/VarDumper/composer.json
@@ -21,7 +21,8 @@
},
"require-dev": {
"ext-iconv": "*",
"twig/twig": "~1.34|~2.4"
"twig/twig": "~1.34|~2.4",
"symfony/process": "~3.4|~4.0"

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Aug 23, 2017

Member

alpha order :)

@nicolas-grekas

nicolas-grekas Aug 23, 2017

Member

alpha order :)

Show outdated Hide outdated src/Symfony/Component/VarDumper/Server/DumpServer.php
*
* @author Maxime Steinhausser <maxime.steinhausser@gmail.com>
*
* @final since version 3.4

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Aug 23, 2017

Member

just @final (since version 3.4 suggests is existed before, which is not the case)

@nicolas-grekas

nicolas-grekas Aug 23, 2017

Member

just @final (since version 3.4 suggests is existed before, which is not the case)

Show outdated Hide outdated src/Symfony/Component/VarDumper/Server/DumpServer.php
if ($this->logger) {
$this->logger->warning('Unable to decode a message from {clientId} client.', array(
'clientId' => $clientId,
));

This comment has been minimized.

@fabpot

fabpot Aug 24, 2017

Member

Should be on one line (I haven't checked if you have other occurrences of similar lines).

@fabpot

fabpot Aug 24, 2017

Member

Should be on one line (I haven't checked if you have other occurrences of similar lines).

This comment has been minimized.

@ogizanagi

ogizanagi Aug 24, 2017

Member

Fixed

@ogizanagi
Show outdated Hide outdated src/Symfony/Component/VarDumper/Server/DumpServer.php
$payload = unserialize(base64_decode($message));
// Impossible to decode the message, give up.
if (false === $payload) {

This comment has been minimized.

@fabpot

fabpot Aug 24, 2017

Member

That does not work if you send a message that is not base64 encoded. PHP will vail out on line 65.

@fabpot

fabpot Aug 24, 2017

Member

That does not work if you send a message that is not base64 encoded. PHP will vail out on line 65.

This comment has been minimized.

@ogizanagi

ogizanagi Aug 24, 2017

Member

base64_decode won't fail on a non base64 encoded string AFAIK, but unserialize will trigger a notice. I think silencing it is enough but let me know.

@ogizanagi

ogizanagi Aug 24, 2017

Member

base64_decode won't fail on a non base64 encoded string AFAIK, but unserialize will trigger a notice. I think silencing it is enough but let me know.

Show outdated Hide outdated src/Symfony/Component/VarDumper/Command/ServerDumpCommand.php
{
$this
->setName('server:dump')
->addOption('output', null, InputOption::VALUE_REQUIRED, 'An file path to save dumped data in html format, or null to display on CLI output.')

This comment has been minimized.

@fabpot

fabpot Aug 24, 2017

Member

A file ...

@fabpot

fabpot Aug 24, 2017

Member

A file ...

This comment has been minimized.

@fabpot

fabpot Aug 24, 2017

Member

What about allowing to dump on STDOUT? Which means that everything else should be sent to STDERR.

@fabpot

fabpot Aug 24, 2017

Member

What about allowing to dump on STDOUT? Which means that everything else should be sent to STDERR.

This comment has been minimized.

@fabpot

fabpot Aug 24, 2017

Member

HTML, not html

@fabpot

fabpot Aug 24, 2017

Member

HTML, not html

This comment has been minimized.

@ogizanagi

ogizanagi Aug 24, 2017

Member

What about allowing to dump on STDOUT? Which means that everything else should be sent to STDERR.

Using STDERR for command titles & comments is a fair point, I did it.

For dumps, the CliDumper will already use STDOUT. I also kept the context informations on STDOUT.

Or did you actually mean removing the --output option in favor of automatically using the HtmlDumper in the case of the output being redirected to a file or piped?
AFAIK, the only simple way to tell that is by using posix_isatty, which won't be available on Windows (or perhaps fstat mode, but not sure about this being reliable on Windows neither).

@ogizanagi

ogizanagi Aug 24, 2017

Member

What about allowing to dump on STDOUT? Which means that everything else should be sent to STDERR.

Using STDERR for command titles & comments is a fair point, I did it.

For dumps, the CliDumper will already use STDOUT. I also kept the context informations on STDOUT.

Or did you actually mean removing the --output option in favor of automatically using the HtmlDumper in the case of the output being redirected to a file or piped?
AFAIK, the only simple way to tell that is by using posix_isatty, which won't be available on Windows (or perhaps fstat mode, but not sure about this being reliable on Windows neither).

This comment has been minimized.

@ogizanagi

ogizanagi Aug 26, 2017

Member

What about replacing the --output option by a --format option and extract processing in proper descriptors? It would let the user redirect or pipe the output and may allow other streamable formats like json-text-sequence to be implemented next.
Extracting descriptors from the command might also answer #23831 (comment) by allowing reuse and easing tests.

@ogizanagi

ogizanagi Aug 26, 2017

Member

What about replacing the --output option by a --format option and extract processing in proper descriptors? It would let the user redirect or pipe the output and may allow other streamable formats like json-text-sequence to be implemented next.
Extracting descriptors from the command might also answer #23831 (comment) by allowing reuse and easing tests.

This comment has been minimized.

@ogizanagi

ogizanagi Sep 24, 2017

Member

Done.

@ogizanagi
Show outdated Hide outdated src/Symfony/Component/VarDumper/Command/ServerDumpCommand.php
$this->filesystem->appendToFile($outputFile, sprintf('<style>%s</style><script>%s</script>', self::$style, self::$javascript));
}
$io->comment(sprintf('Consult collected dumps in html format by opening "%s" in your browser.', realpath($outputFile)));

This comment has been minimized.

@fabpot

fabpot Aug 24, 2017

Member

I let you change html to HTML everywhere :)

@fabpot

fabpot Aug 24, 2017

Member

I let you change html to HTML everywhere :)

This comment has been minimized.

@ogizanagi

ogizanagi Aug 24, 2017

Member

Fixed 😄

@ogizanagi

ogizanagi Aug 24, 2017

Member

Fixed 😄

Show outdated Hide outdated src/Symfony/Component/VarDumper/Command/ServerDumpCommand.php
*/
class ServerDumpCommand extends Command
{
private static $style = <<<'CSS'

This comment has been minimized.

@fabpot

fabpot Aug 24, 2017

Member

I would store the styles in a file under Resources/ so that it's more easily reusable.

@fabpot

fabpot Aug 24, 2017

Member

I would store the styles in a file under Resources/ so that it's more easily reusable.

This comment has been minimized.

@ogizanagi

ogizanagi Aug 24, 2017

Member

Still something useful considering #23831 (comment) ?

@ogizanagi

ogizanagi Aug 24, 2017

Member

Still something useful considering #23831 (comment) ?

This comment has been minimized.

@ogizanagi

ogizanagi Sep 24, 2017

Member

Done.

@ogizanagi
Show outdated Hide outdated src/Symfony/Component/VarDumper/Command/ServerDumpCommand.php
}
CSS;
private static $javascript = <<<'JS'

This comment has been minimized.

@fabpot

fabpot Aug 24, 2017

Member

Same for this piece of JS.

@fabpot

fabpot Aug 24, 2017

Member

Same for this piece of JS.

Show outdated Hide outdated src/Symfony/Component/VarDumper/Command/ServerDumpCommand.php
});
}
private function displayDumpCli(SymfonyStyle $io, Data $data, array $context, $clientId)

This comment has been minimized.

@fabpot

fabpot Aug 24, 2017

Member

If I'm creating my own server (like explained in the PR description), I would probably like to be able to reuse the logic in those 2 private methods, right?

@fabpot

fabpot Aug 24, 2017

Member

If I'm creating my own server (like explained in the PR description), I would probably like to be able to reuse the logic in those 2 private methods, right?

This comment has been minimized.

@ogizanagi

ogizanagi Aug 24, 2017

Member

I consider this command as a final application, not meant to be extended. That's why I also made it @final.

If someone wants to create his own server app, he'll probably want to integrate dumps his own way in his application and won't reuse the ServerDumpCommand output. He'll create his own script collecting & processing dumps (perhaps by dumping it into a json file to be consumed by a ReactJS application for instance) using the DumpServer class.

@ogizanagi

ogizanagi Aug 24, 2017

Member

I consider this command as a final application, not meant to be extended. That's why I also made it @final.

If someone wants to create his own server app, he'll probably want to integrate dumps his own way in his application and won't reuse the ServerDumpCommand output. He'll create his own script collecting & processing dumps (perhaps by dumping it into a json file to be consumed by a ReactJS application for instance) using the DumpServer class.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Sep 24, 2017

Member

Last update: remove the --output option in favor of using --format=html|cli and redirecting the output to a file.

Feature freeze is coming very soon.
Would you have some time to play with this @nicolas-grekas ? :)

Member

ogizanagi commented Sep 24, 2017

Last update: remove the --output option in favor of using --format=html|cli and redirecting the output to a file.

Feature freeze is coming very soon.
Would you have some time to play with this @nicolas-grekas ? :)

Requested changes done

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 10, 2017

Member

I'd like to test it more thoroughly. Let's move it to 4.1.

Member

fabpot commented Oct 10, 2017

I'd like to test it more thoroughly. Let's move it to 4.1.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Mar 11, 2018

Member

I may be a bit late to the party considering the upcoming 4.1 feature freeze. But in any case, this is now rebased, using PHP 7.1 features and tested on a 4.1 app. Everything still working fine :)

Tests failures are unrelated (see #26489).

Member

ogizanagi commented Mar 11, 2018

I may be a bit late to the party considering the upcoming 4.1 feature freeze. But in any case, this is now rebased, using PHP 7.1 features and tested on a 4.1 app. Everything still working fine :)

Tests failures are unrelated (see #26489).

@antonmedv

This comment has been minimized.

Show comment
Hide comment
@antonmedv

antonmedv Mar 12, 2018

Will be cool to have it. Like output on rails or elixir/phoenix apps.

antonmedv commented Mar 12, 2018

Will be cool to have it. Like output on rails or elixir/phoenix apps.

@chalasr

Tested, great work 👍

@yceruto

A big thanks! wonderful and very useful.

Show outdated Hide outdated ...ony/Component/VarDumper/Dumper/ContextProvider/SourceContextProvider.php
$this->fileLinkFormatter = $fileLinkFormatter;
}
public function __invoke(): array

This comment has been minimized.

@stof

stof Mar 21, 2018

Member

Rather than making this a callable class, forcing to assign a local variable before calling the logic, I would rather give a name to this method, and call it directly on $this->sourceContextProvider in the data collector.

@stof

stof Mar 21, 2018

Member

Rather than making this a callable class, forcing to assign a local variable before calling the logic, I would rather give a name to this method, and call it directly on $this->sourceContextProvider in the data collector.

This comment has been minimized.

@ogizanagi

ogizanagi Mar 21, 2018

Member

By design, this has to be a callable as a context provider used by the server dumper. But actually, no need to use a local variable anymore since this is not targeting the 3.4 branch anymore. So we don't require PHP 5.x support. :)

Thanks!

@ogizanagi

ogizanagi Mar 21, 2018

Member

By design, this has to be a callable as a context provider used by the server dumper. But actually, no need to use a local variable anymore since this is not targeting the 3.4 branch anymore. So we don't require PHP 5.x support. :)

Thanks!

Show outdated Hide outdated ...ony/Component/VarDumper/Dumper/ContextProvider/SourceContextProvider.php
$name = substr($name, strrpos($name, '/') + 1);
}
$context = compact('name', 'file', 'line');

This comment has been minimized.

@stof

stof Mar 21, 2018

Member

please avoid using compact. It makes it harder to see where a variable is used.

@stof

stof Mar 21, 2018

Member

please avoid using compact. It makes it harder to see where a variable is used.

This comment has been minimized.

@ogizanagi

ogizanagi Mar 21, 2018

Member

At least PhpStorm handles it quite well:

screenshot 2018-03-21 a 17 57 58

and it was already used in the collector. But anyway, I'll change this.

@ogizanagi

ogizanagi Mar 21, 2018

Member

At least PhpStorm handles it quite well:

screenshot 2018-03-21 a 17 57 58

and it was already used in the collector. But anyway, I'll change this.

@nicolas-grekas

nicolas-grekas requested changes Mar 21, 2018 edited

Looks great, here are a few comments.

If I may dream a bit, I'd like to be able to run server:run-all and have server:run, server:log and server:dump in the same console (we could add webpack dev server to the list :) )
I would also love to replace the --format=html option by a proper webserver I could point my browser at, and see the live feed of dumps (and logs, etc.) in this tab.

Btw, you don't use the source context in the CLI/HTML. Don't you want to add some source code excerpt there?

In the CLI, what about displaying the link to the file? cannot be an IDE link, but still a web link is possible (e.g. http://127.0.0.1:8000/_profiler/open?file=src/Controller/HelloController.php&line=15#line15)

Show outdated Hide outdated src/Symfony/Bundle/DebugBundle/DependencyInjection/Configuration.php
@@ -51,6 +51,14 @@ public function getConfigTreeBuilder()
->example('php://stderr')
->defaultNull()
->end()
->arrayNode('server_dump')->canBeEnabled()
->children()
->booleanNode('swallow')

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

can we remove this option? that's reduce the complexity of the feature
(if yes, server_dump could be turned to a boolean?)

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

can we remove this option? that's reduce the complexity of the feature
(if yes, server_dump could be turned to a boolean?)

This comment has been minimized.

@ogizanagi

ogizanagi Mar 22, 2018

Member

Done

@ogizanagi
Show outdated Hide outdated src/Symfony/Component/HttpKernel/DataCollector/DumpDataCollector.php
@@ -55,6 +59,8 @@ public function __construct(Stopwatch $stopwatch = null, $fileLinkFormat = null,
&$this->isCollected,
&$this->clonesCount,
);
$this->sourceContextProvider = new SourceContextProvider(8, $this->charset);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

$serverDumper->getContextProviders()['source'] ?? new SourceContextProvider(8, $this->charset);?

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

$serverDumper->getContextProviders()['source'] ?? new SourceContextProvider(8, $this->charset);?

This comment has been minimized.

@ogizanagi

ogizanagi Mar 22, 2018

Member

I'm not convinced this is worth exposing the context providers.

@ogizanagi

ogizanagi Mar 22, 2018

Member

I'm not convinced this is worth exposing the context providers.

Show outdated Hide outdated src/Symfony/Bundle/DebugBundle/DependencyInjection/DebugExtension.php
;
} else {
$container->removeDefinition('var_dumper.server_dumper');
$container->removeDefinition('var_dumper.command.server_dump');

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

Instead of removing the command, we could make it display a friendly message/instructions when things are disabled? would ease discovery.

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

Instead of removing the command, we could make it display a friendly message/instructions when things are disabled? would ease discovery.

This comment has been minimized.

@ogizanagi

ogizanagi Mar 22, 2018

Member

The command is standalone and doesn't really need anything to be enabled to run. I'm removing it so it doesn't appear in the list if the server dumper isn't enabled, which would be confusing as nothing would be collected by the server.
If we really want the user to be able to run server:dump even if the server dumper isn't enabled and display him a message to suggest him enabling it, how would you proceed? I don't think adding a dedicated argument in ServerDumpCommand would make sense, as the command can be used out of the framework & debug bundle. So, a proxy command in the debug bundle?

@ogizanagi

ogizanagi Mar 22, 2018

Member

The command is standalone and doesn't really need anything to be enabled to run. I'm removing it so it doesn't appear in the list if the server dumper isn't enabled, which would be confusing as nothing would be collected by the server.
If we really want the user to be able to run server:dump even if the server dumper isn't enabled and display him a message to suggest him enabling it, how would you proceed? I don't think adding a dedicated argument in ServerDumpCommand would make sense, as the command can be used out of the framework & debug bundle. So, a proxy command in the debug bundle?

Show outdated Hide outdated src/Symfony/Component/HttpKernel/DataCollector/DumpDataCollector.php
public function __construct(Stopwatch $stopwatch = null, $fileLinkFormat = null, string $charset = null, RequestStack $requestStack = null, DataDumperInterface $dumper = null)
public function __construct(Stopwatch $stopwatch = null, $fileLinkFormat = null, string $charset = null, RequestStack $requestStack = null, DataDumperInterface $dumper = null, ServerDumper $serverDumper = null)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

why do we need a new argument? can't we pass the ServerDumper instance using $dumper?
this would require the serverdumper to accept a fallback dumper as argument to be used when the server is down
would allow better design than isServerUp, WDYT?

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

why do we need a new argument? can't we pass the ServerDumper instance using $dumper?
this would require the serverdumper to accept a fallback dumper as argument to be used when the server is down
would allow better design than isServerUp, WDYT?

This comment has been minimized.

@ogizanagi

ogizanagi Mar 22, 2018

Member

Honestly I don't remember enough all the implications of these changes. What I remember is that I did them to keep the exact current behavior when not enabling the server dumper. I tried again, but I'm unable to get it right by using $dumper.
I'd appreciate if you want to give it a try yourself (playing with both server_dump enabled/disabled) but I hope it won't be a blocker as the current behavior looks satisfying to me. Thanks!

@ogizanagi

ogizanagi Mar 22, 2018

Member

Honestly I don't remember enough all the implications of these changes. What I remember is that I did them to keep the exact current behavior when not enabling the server dumper. I tried again, but I'm unable to get it right by using $dumper.
I'd appreciate if you want to give it a try yourself (playing with both server_dump enabled/disabled) but I hope it won't be a blocker as the current behavior looks satisfying to me. Thanks!

Show outdated Hide outdated src/Symfony/Component/HttpKernel/DataCollector/DumpDataCollector.php
--$i;
}
// If the server dumper is set and server is up, dump were already sent to it.
if (!$this->serverDumper || !$this->serverDumper->isServerUp()) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

talking my previous comments into account, not sure we need this "if": this code handles a very special situation, namely errors that break correct collection of data. Not a big issue if we keep the fallback logic here.

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

talking my previous comments into account, not sure we need this "if": this code handles a very special situation, namely errors that break correct collection of data. Not a big issue if we keep the fallback logic here.

Show outdated Hide outdated ...ony/Component/VarDumper/Dumper/ContextProvider/SourceContextProvider.php
private $projectDir;
private $fileLinkFormatter;
public function __construct(int $limit = 9, string $charset = null, string $projectDir = null, FileLinkFormatter $fileLinkFormatter = null)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

the purpose $limit is quite obscure without reading the code
can't we move it as the last argument, and always use the same default value in practice?
(do we really need to create instances with 9, or 8, or etc or could 9 fit all the cases?)

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

the purpose $limit is quite obscure without reading the code
can't we move it as the last argument, and always use the same default value in practice?
(do we really need to create instances with 9, or 8, or etc or could 9 fit all the cases?)

This comment has been minimized.

@ogizanagi

ogizanagi Mar 22, 2018

Member

done

@ogizanagi
Show outdated Hide outdated src/Symfony/Component/VarDumper/Dumper/ServerDumper.php
* @param DataDumperInterface|null $wrappedDumper A wrapped instance used whenever we failed contacting the
* server, or if swallow is false
* @param bool $swallow Whether to use or not the wrapped dumper instance on normal
* use

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

not sure we need a dedicated line for one word ;) (same above btw)

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

not sure we need a dedicated line for one word ;) (same above btw)

This comment has been minimized.

@ogizanagi

ogizanagi Mar 22, 2018

Member

fixed

@ogizanagi
Show outdated Hide outdated src/Symfony/Component/VarDumper/Dumper/ServerDumper.php
}
}
public function isServerUp(): bool

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

isServerListening?

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

isServerListening?

This comment has been minimized.

@ogizanagi

ogizanagi Mar 22, 2018

Member

done, thanks!

@ogizanagi

ogizanagi Mar 22, 2018

Member

done, thanks!

Show outdated Hide outdated src/Symfony/Component/VarDumper/Server/DumpServer.php
/**
* @param string|null $host The server host or null to read it from the VAR_DUMPER_SERVER env var
* @param LoggerInterface|null $logger

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

I think the docblock can be removed altogether

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

I think the docblock can be removed altogether

This comment has been minimized.

@ogizanagi

ogizanagi Mar 22, 2018

Member

The docblock above still is useful to me to highlight the VAR_DUMPER_SERVER env var, but the $logger one could be removed. However, we didn't agree on partial docblock in the core, right?

@ogizanagi

ogizanagi Mar 22, 2018

Member

The docblock above still is useful to me to highlight the VAR_DUMPER_SERVER env var, but the $logger one could be removed. However, we didn't agree on partial docblock in the core, right?

Show outdated Hide outdated src/Symfony/Component/VarDumper/Server/DumpServer.php
public function __construct(string $host = null, LoggerInterface $logger = null)
{
if (null === $host) {
$host = getenv(static::HOST_ENV_VAR) ?: static::DEFAULT_HOST;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

not sure we need handling of env here - can't we use DI based env injection instead?

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

not sure we need handling of env here - can't we use DI based env injection instead?

This comment has been minimized.

@ogizanagi

ogizanagi Mar 22, 2018

Member

As discussed in #23831 (comment) & #23831 (comment), I'd like to keep it for using it easily from almost anywhere, even without the debug bundle and for multiple projects at the same time.

@ogizanagi

ogizanagi Mar 22, 2018

Member

As discussed in #23831 (comment) & #23831 (comment), I'd like to keep it for using it easily from almost anywhere, even without the debug bundle and for multiple projects at the same time.

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

Oh btw, the HTML view is missing a date (and maybe the controller also, as displayed on the CLI):

image

Member

nicolas-grekas commented Mar 21, 2018

Oh btw, the HTML view is missing a date (and maybe the controller also, as displayed on the CLI):

image

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 21, 2018

Member

@nicolas-grekas Regarding your dream, let's keep that for another time :) I like the Unix philosophy.

Member

fabpot commented Mar 21, 2018

@nicolas-grekas Regarding your dream, let's keep that for another time :) I like the Unix philosophy.

@sroze

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Mar 21, 2018

Member
Member

sroze commented Mar 21, 2018

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Mar 22, 2018

Member

If I may dream a bit, I'd like to be able to run server:run-all and have server:run, server:log and server:dump in the same console (we could add webpack dev server to the list :) )
I would also love to replace the --format=html option by a proper webserver I could point my browser at, and see the live feed of dumps (and logs, etc.) in this tab.

When I started this PR, I also have in mind to provide a full featured web & cli app to centralize dumps & logs, but didn't take enough time yet to experiment it. Anyway this PR is already a big step to me, and I already have some other small improvements to come first in order to help using it easily on any context.

Btw, you don't use the source context in the CLI/HTML. Don't you want to add some source code excerpt there?

Currently, code excerpts are only retrieved by the SourceContextProvider for twig template. Being able to always getting the excerpt would require duplicating some logic from the twig bridge CodeExtension (or allow the HtmlDescriptor to use Twig and this extension fi available). I didn't want to pollute more this PR. So I'd suggest to try adding it into another PR :)

In the CLI, what about displaying the link to the file? cannot be an IDE link, but still a web link is possible (e.g. http://127.0.0.1:8000/_profiler/open?file=src/Controller/HelloController.php&line=15#line15)

Actually, it's the reverse: cannot be a web link for a cli command (no context available). But IDE link works great for both web requests & cli commands:

capture d ecran 2018-03-22 a 16 35 16

Oh btw, the HTML view is missing a date (and maybe the controller also, as displayed on the CLI):

I guess you've missed it 😄 It's already displayed on the top right:

capture d ecran 2018-03-22 a 16 36 51

About the controller, I'd suggest to polish and add it into another PR. Right now I'm not sure where to display it and don't want to pollute more this PR.

Thanks everyone for your reviews.

Member

ogizanagi commented Mar 22, 2018

If I may dream a bit, I'd like to be able to run server:run-all and have server:run, server:log and server:dump in the same console (we could add webpack dev server to the list :) )
I would also love to replace the --format=html option by a proper webserver I could point my browser at, and see the live feed of dumps (and logs, etc.) in this tab.

When I started this PR, I also have in mind to provide a full featured web & cli app to centralize dumps & logs, but didn't take enough time yet to experiment it. Anyway this PR is already a big step to me, and I already have some other small improvements to come first in order to help using it easily on any context.

Btw, you don't use the source context in the CLI/HTML. Don't you want to add some source code excerpt there?

Currently, code excerpts are only retrieved by the SourceContextProvider for twig template. Being able to always getting the excerpt would require duplicating some logic from the twig bridge CodeExtension (or allow the HtmlDescriptor to use Twig and this extension fi available). I didn't want to pollute more this PR. So I'd suggest to try adding it into another PR :)

In the CLI, what about displaying the link to the file? cannot be an IDE link, but still a web link is possible (e.g. http://127.0.0.1:8000/_profiler/open?file=src/Controller/HelloController.php&line=15#line15)

Actually, it's the reverse: cannot be a web link for a cli command (no context available). But IDE link works great for both web requests & cli commands:

capture d ecran 2018-03-22 a 16 35 16

Oh btw, the HTML view is missing a date (and maybe the controller also, as displayed on the CLI):

I guess you've missed it 😄 It's already displayed on the top right:

capture d ecran 2018-03-22 a 16 36 51

About the controller, I'd suggest to polish and add it into another PR. Right now I'm not sure where to display it and don't want to pollute more this PR.

Thanks everyone for your reviews.

Show outdated Hide outdated src/Symfony/Component/HttpKernel/DataCollector/DumpDataCollector.php
@@ -55,6 +59,8 @@ public function __construct(Stopwatch $stopwatch = null, $fileLinkFormat = null,
&$this->isCollected,
&$this->clonesCount,
);
$this->sourceContextProvider = new SourceContextProvider($this->charset);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 22, 2018

Member

this defeats dependency injection, that's why I proposed to get it through $serverDumper

@nicolas-grekas

nicolas-grekas Mar 22, 2018

Member

this defeats dependency injection, that's why I proposed to get it through $serverDumper

@symfony symfony deleted a comment from ogizanagi Mar 22, 2018

Show outdated Hide outdated ...ny/Component/VarDumper/Dumper/ContextProvider/RequestContextProvider.php
*/
final class RequestContextProvider implements ContextProviderInterface
{
/** @var RequestStack */

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 22, 2018

Member

can be removed

@nicolas-grekas

nicolas-grekas Mar 22, 2018

Member

can be removed

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Mar 22, 2018

Member

@ogizanagi FYI I've forked your branch in https://github.com/nicolas-grekas/symfony/tree/feature/server_dump - the first commit is your PR squashed, the second are some tweaks I'd like to merge here when ready. They break the feature for now, but I think you can get the idea. If you want to take over, please tell :)

Member

nicolas-grekas commented Mar 22, 2018

@ogizanagi FYI I've forked your branch in https://github.com/nicolas-grekas/symfony/tree/feature/server_dump - the first commit is your PR squashed, the second are some tweaks I'd like to merge here when ready. They break the feature for now, but I think you can get the idea. If you want to take over, please tell :)

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Mar 22, 2018

Member

@nicolas-grekas : Here you are.

The changes made to the data collector were indeed the ones I tried today, so let me explain:
What was bothering me was the inconsistency between the behaviors with API calls when you set the debug.dump_destination to tcp://* and anything else:
In the first case, even if the dump server isn't running, no more dump output appears in the response, while with debug.dump_destination set to anything else or null (i.e no server dumper <=> current behavior) cli dumps appears in the output.
As stated in the PR description, getting a mix of your response and the cli dumped version of the data you asked is a mess, but at least you know it's there.
Probably that I'm overthinking this and this behavior should have been considered undesirable in the first place...

I also added a placeholder command in the debug bundle to easy discovery of the feature as you asked.

Finally, I'm not really satisfied by the new VAR_DUMPER_SERVER env var handling. But you already heard the arguments here.

Member

ogizanagi commented Mar 22, 2018

@nicolas-grekas : Here you are.

The changes made to the data collector were indeed the ones I tried today, so let me explain:
What was bothering me was the inconsistency between the behaviors with API calls when you set the debug.dump_destination to tcp://* and anything else:
In the first case, even if the dump server isn't running, no more dump output appears in the response, while with debug.dump_destination set to anything else or null (i.e no server dumper <=> current behavior) cli dumps appears in the output.
As stated in the PR description, getting a mix of your response and the cli dumped version of the data you asked is a mess, but at least you know it's there.
Probably that I'm overthinking this and this behavior should have been considered undesirable in the first place...

I also added a placeholder command in the debug bundle to easy discovery of the feature as you asked.

Finally, I'm not really satisfied by the new VAR_DUMPER_SERVER env var handling. But you already heard the arguments here.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 23, 2018

Member

@ogizanagi Do you think you will have time (perhaps this week-end) to take comments into account (if not, someone can probably take over)? I would love to merge it for 4.1.

Member

fabpot commented Mar 23, 2018

@ogizanagi Do you think you will have time (perhaps this week-end) to take comments into account (if not, someone can probably take over)? I would love to merge it for 4.1.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Mar 23, 2018

Member

@fabpot : I've already taken all the comments into account & cherry-picked Nicolas' commit. This is already ready (at least for another round of reviews).

Member

ogizanagi commented Mar 23, 2018

@fabpot : I've already taken all the comments into account & cherry-picked Nicolas' commit. This is already ready (at least for another round of reviews).

@fabpot

fabpot approved these changes Mar 23, 2018

with minor comments

Show outdated Hide outdated src/Symfony/Bundle/DebugBundle/Command/ServerDumpPlaceholderCommand.php
protected function execute(InputInterface $input, OutputInterface $output)
{
(new SymfonyStyle($input, $output))->getErrorStyle()->warning('In order to use the VarDumper server, set the debug.dump_destination config option to "tcp://%env(VAR_DUMPER_SERVER)%"');

This comment has been minimized.

@fabpot

fabpot Mar 23, 2018

Member

Can you quote (") debug.dump_destination?

@fabpot

fabpot Mar 23, 2018

Member

Can you quote (") debug.dump_destination?

Show outdated Hide outdated src/Symfony/Bundle/DebugBundle/DependencyInjection/Configuration.php
@@ -48,7 +48,7 @@ public function getConfigTreeBuilder()
->end()
->scalarNode('dump_destination')
->info('A stream URL where dumps should be written to')
->example('php://stderr')
->example('php://stderr, or tcp://%env(VAR_DUMPER_SERVER)% when using the `server:dump` command')

This comment has been minimized.

@fabpot

fabpot Mar 23, 2018

Member

Can you use " instead of `?

@fabpot

fabpot Mar 23, 2018

Member

Can you use " instead of `?

Show outdated Hide outdated src/Symfony/Component/VarDumper/CHANGELOG.md
* added a `ServerDumper` to send serialized Data clones to a server
* added a `ServerDumpCommand` and `DumpServer` to run a server collecting
and displaying dumps on a single place with multiple formats support
* added `CliDescriptor` and `HtmlDescriptor` descriptors for `server:dump` cli and html formats support

This comment has been minimized.

@fabpot

fabpot Mar 23, 2018

Member

CLI and HTML formats

@fabpot

fabpot Mar 23, 2018

Member

CLI and HTML formats

Show outdated Hide outdated src/Symfony/Component/VarDumper/Command/ServerDumpCommand.php
$availableFormats = implode(', ', array_keys($this->descriptors));
$this
->addOption('format', null, InputOption::VALUE_REQUIRED, "The output format ($availableFormats)", 'cli')

This comment has been minimized.

@fabpot

fabpot Mar 23, 2018

Member

you should use sprintf here.

@fabpot

fabpot Mar 23, 2018

Member

you should use sprintf here.

@ogizanagi

This comment has been minimized.

Show comment
Hide comment
@ogizanagi

ogizanagi Mar 23, 2018

Member

@fabpot : Fixed & squashed (I've kept Nicolas' commit and the next one finishing it).

Member

ogizanagi commented Mar 23, 2018

@fabpot : Fixed & squashed (I've kept Nicolas' commit and the next one finishing it).

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 23, 2018

Member

Thank you @ogizanagi.

Member

fabpot commented Mar 23, 2018

Thank you @ogizanagi.

@fabpot fabpot merged commit 138dad6 into symfony:master Mar 23, 2018

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 23, 2018

feature #23831 [VarDumper] Introduce a new way to collect dumps throu…
…gh a server dumper (ogizanagi, nicolas-grekas)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[VarDumper] Introduce a new way to collect dumps through a server dumper

| Q             | A
| ------------- | ---
| Branch?       | 4.1 <!-- see comment below -->
| Bug fix?      | no
| New feature?  | yes <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | #22987 <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | todo <!--highly recommended for new features-->

Could also be interesting as alternate solution for #23442.

Inspired by the [`ServerLogHandler`](#21080) and @nicolas-grekas's [comment](#22987 (comment)) in #22987.

---

## Description

This PR introduces a new `server:dump` command available in the `VarDumper` component.
Conjointly with a new `ServerDumper` data dumper, it allows to send serialized `Data` clones to a single centralized server, in charge of dumping them on CLI output, or in an file in HTML format.

## Showcase

### HTTP calls

For instance, when working on an API and dumping something, you might end up with a mix of your response and the CLI dumped version of the data you asked:

```php
class HelloController extends AbstractController
{
    /**
     * @route("/hello")
     */
    public function hello(Request $request, UserInterface $user)
    {
        dump($request->attributes, $user);

        return JsonResponse::create([
            'status' => 'OK',
            'message' => "Hello {$user->getUsername()}"
        ]);
    }
}
```

<img width="732" alt="screenshot 2017-08-08 a 16 44 24" src="https://user-images.githubusercontent.com/2211145/29077731-0b2152d6-7c59-11e7-99dd-2d060a906d48.PNG">

You might even get the HTML dump version [under some conditions](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/DataCollector/DumpDataCollector.php#L146-L157).

Dumping to a server allows collecting dumps in a separate place:

<!--![server-dumper-http-to-cli-output](https://user-images.githubusercontent.com/2211145/29077977-8ace19ce-7c59-11e7-998e-ee9c49e67958.gif)-->

![server-dump-http-to-cli](https://user-images.githubusercontent.com/2211145/29226044-dcc95f12-7ed0-11e7-8343-40aeb7b18dd5.gif)

~⚠️ By swallowing the previous dumpers output, the dump data collector is not active when running the dump server. Disable swallowing if you want both.~ ➜ Dumps are still collected in the profiler thanks to f24712e

### CLI calls

The best probably is (to me) that you can also debug CLI applications...

![server-dump-cli](https://user-images.githubusercontent.com/2211145/29225951-84e29098-7ed0-11e7-8067-abaa9c50d169.gif)

<!--![server-dumper-cli-to-cli-output](https://user-images.githubusercontent.com/2211145/29078261-1eb950ea-7c5a-11e7-94ee-aa3ae3bf7fb0.gif)-->

...and get HTML formatted dumps:

![server-dumper-cli-to-html-output](https://user-images.githubusercontent.com/2211145/30784609-d7dc19b8-a158-11e7-9b11-88ae819cfcca.gif)

<!--![server-dump-cli-to-html](https://user-images.githubusercontent.com/2211145/29225866-2b5675e4-7ed0-11e7-98eb-339611bd94a7.gif)-->

<!--![server-dumper-cli-to-html-output](https://user-images.githubusercontent.com/2211145/29078247-17e33e5c-7c5a-11e7-94f7-47de6774e0e8.gif)-->

hence, benefit from all the features of this format (collapse, search, ...)

### HTML output at a glance

<img width="831" alt="screenshot 2017-08-11 a 19 28 25" src="https://user-images.githubusercontent.com/2211145/29225513-eae349f2-7ece-11e7-9861-8cda9e80ba7f.PNG">

The HTML output benefits from all the `HtmlDumper` features and contains extra informations about the context (sources, requests, command line, ...). It doesn't aim to replace the profiler for HTTP calls with the framework, but is really handy for CLI apps or by wiring it in your own web app, out of the framework usage.

### CLI output at a glance

<img width="829" alt="screenshot 2017-08-11 a 19 52 57" src="https://user-images.githubusercontent.com/2211145/29225482-c24afe18-7ece-11e7-8e83-d019b0d8303e.PNG">

## Usage within the framework

### Config

For instance, in your `config_dev.yml`:

```yml
#config_dev.yml
debug:
    server_dump: true
```

or in your `config.yml`:

```yml
#config.yml
debug:
    server_dump: '%kernel.debug%'
```

~~The configuration also allows to set a `host` option, but there is already a sensible default value (`tcp://0.0.0.0:9912`) so you don't have to deal with it.~~ Since b002175, in case you want to change the default host value used, simply use the `VAR_DUMPER_SERVER` env var.

When the server is running, all dumps are collected by the server and previous dumpers ones are "swallowed" ~~by default. If you want both to collect dumps on the server AND keep previous dumpers on regular outputs, you can disable swallowing:~~

<!--
```yml
debug:
    server_dump:
        swallow: false
```
-->

When the server isn't running or in case of failure to send the data clones to the server, the server dumper will delegates to the configured wrapped dumper, so dumps are displayed and collected as usual.

### Running the server

```bash
bin/console server:dump [--format=cli|html]
```

#### Options

- ~~The `output` option defaults to `null` which will display dumps on CLI. It accepts a file path in which dumps will be collected in HTML format.~~
- The `format` option allows to switch format to use. For instance, use the `html` format and redirect the output to a file in order to open it in your browser and inspect dumps in HTML format.
- ~~The default `host` value is the same as the one configured under the `debug.server_dump.host` config option, so you don't have to deal with it in most cases.~~
    Since b002175, in case you want to change the default host value used, simply use the `VAR_DUMPER_SERVER` env var:

    ```bash
    VAR_DUMPER_SERVER=0.0.0.0:9204 bin/console server:dump
    ```

## Manual wiring

If you want to wire it yourself in your own project or using it to inspect dumps as html before the kernel is even boot for instance:

```php
$host = 'tcp://0.0.0.0:9912'; // use null to read from the VAR_DUMPER_SERVER env var
$cloner = new VarCloner();
$dumper = new ServerDumper($host, new CliDumper());

VarDumper::setHandler(function ($var) use ($dumper, $cloner) {
    $dumper->dump($cloner->cloneVar($var));
});
```

## Create your own server app

The component already provides a default server app by means of the `ServerDumpCommand`, but
 you could also build your own by using the `DumpServer`:

```php
$host = 'tcp://0.0.0.0:9912'; // use null to read from the VAR_DUMPER_SERVER env var

$server = new DumpServer($host);

$server->start();

$server->listen(function (Data $data, array $context, $clientId) {
    // do something
});
```

Commits
-------

138dad6 [VarDumper] Some tweaks after Nicolas' commit & ServerDumperPlaceholderCommand
088c52e [VarDumper] Review config to use debug.dump_destination & tweak data collector
3db1404 [VarDumper] Introduce a new way to collect dumps through a server dumper

@ogizanagi ogizanagi deleted the ogizanagi:feature/server_dump branch Mar 23, 2018

fabpot added a commit that referenced this pull request Mar 27, 2018

feature #26654 [VarDumper] Provide binary, allowing to start a server…
… at any time (ogizanagi)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[VarDumper] Provide binary, allowing to start a server at any time

as soon as the "symfony/var-dumper" & "symfony/console" components are available.

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see below -->
| Bug fix?      | no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | N/A   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | TODO symfony/symfony-docs#9474

Now that #23831 is merged, there is still room for improvements we can bring during the stabilization months.

Here is a first one: providing a simple CLI binary allows to easily start a server from any project (or even globally), just by having the var dumper & console packages in your deps.

Commits
-------

eef10b1 [VarDumper] Provide binary, allowing to start a server at any time

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

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