[VarDumper] Easy server dumper registration #26695

ogizanagi · 2018-03-28T13:06:11Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	N/A

Following #26654, this is another step aiming to ease usage of the server dumper feature in any situation.

The new ServerDumper::register() method allows to easily configure & set the ServerDumper as main handler in one line.

The new $lock flag might not be the ideal solution, but is answering issues when registering the server dumper as main handler but being overwritten by another process (like in tests for instance. PR acting as a real use-case incoming => #26696).
Anyway, I'm open to any suggestion :)

ogizanagi · 2018-03-28T13:08:44Z

src/Symfony/Component/VarDumper/VarDumper.php

@@ -38,11 +40,27 @@ public static function dump($var)
        return call_user_func(self::$handler, $var);
    }

-    public static function setHandler(callable $callable = null)
+    public static function setHandler(callable $callable = null, bool $lock = false): ?callable


Right now, this is a BC break because the method isn't final. Just waiting more feedback about the process.

kardagan · 2018-03-30T12:26:36Z

src/Symfony/Component/VarDumper/Dumper/ServerDumper.php

+        }
+
+        $fileLinkFormatter = class_exists(FileLinkFormatter::class) ? new FileLinkFormatter(null, $requestStack ?? null, $projectDir) : null;
+        $contextProviders = array(


There is a bug, you erase the $contextProviders that you define on line 63

you must do $contextProviders += array(

Oops. Good catch. Refactoring issue. Thank you 👍

kardagan · 2018-04-03T12:35:40Z

@ogizanagi there is a fail on the test :/ (yes I want to use the content of your PR ^^)

ogizanagi · 2018-04-03T14:38:22Z

@kardagan : I rebased the PR on master to resolve a conflict, but previous failures were unrelated actually. Current build is passing.

However, I do get a failure on HtmlDumperTest & ExceptionCasterTest locally, but once again looks unrelated (not specific to this commit). I don't really get it nor can dig it much right now :/

EDIT: Hmm, actually it's just failing when setting the xdebug.file_link_format ini value (see #26776).

ogizanagi · 2018-04-16T17:40:50Z

So I'd suggest to mark the VarDumper:: setHandler method final and comment the $lock argument unless there is a better option considering the feature.

@kardagan : Did you happen to try this PR? Seems useful to you?

kardagan · 2018-04-17T07:07:51Z

@ogizanagi I don't use your PR, but I copy/past the code in your register method to init my conf.
So yes really useful and I would prefer to just have a VarDumper::register than all the code i have.

My lone comment is on the paramters, I think it's would be more simple to give the Request in parameter than ask a contextProvider array. And in the registier do that.

[..]
if ($request) 
{ 
  $requestStack = new RequestStack();
  $requestStack->push($request);
  $contextProviders['request'] = new RequestContextProvider($requestStack);
}
[...]

ogizanagi · 2018-04-20T06:42:20Z

Yes, I guess we need to find the right balance between flexibility and ease to register it.
But right now, you'll always get pre-configured providers when using register, so what you only need to use your own Request object would be something like this:

ServerDumper::register($host, true, [
    'request' => ServerDumper::getDefaultContextProviders($request)['request'],
])

which still appears to be the most flexible to me for now 🤔
We could also provide a RequestProvider::createFromRequest(Request $request) named constructor, but that's not something very common in core.

nicolas-grekas · 2018-04-22T09:55:44Z

src/Symfony/Component/VarDumper/VarDumper.php

    {
+        $lock = \func_num_args() > 1 ? func_get_arg(1) : false;


I think we can do without this change: just call the method twice to restore

Not sure to understand what you meant?

I meant we should be able to make this work without changing this method at all.

ogizanagi · 2018-05-01T12:44:24Z

After discussing a bit with @nicolas-grekas , I've chosen to remove the lock flag from this PR.
It was a bit out of the scope of this PR and foreseeing more advanced use-cases like in #26696. We may find another solution or another way to expose this behavior.
For now, let's focus on providing the easiest way to register the server dumper as handler to get the maximum relevant context information with minimum code.

This is ready (to me).

nicolas-grekas

This is quite dirty code, with a behavior that varies depending on the dependencies you have. This is glue code, so why not. But I feel like I'd need more explanation, example, doc(blocks) to explain what's the purpose, how to use, so that we get convinced we need this is core (vs e.g. putting this as is in the doc and tell ppl to copy/paste.)

nicolas-grekas · 2018-05-01T14:34:57Z

src/Symfony/Component/VarDumper/Dumper/ServerDumper.php

+     */
+    public static function register(string $host = null, array $contextProviders = array()): ?callable
+    {
+        $contextProviders += self::getDefaultContextProviders();


Having getDefaultContextProviders called with self:: and with no argument here + that the method is final means there is no way to override the default context providers here. This might be fine (is it?) but then, what's the prupose of a standalone getDefaultContextProviders method?

ogizanagi · 2018-07-13T10:46:27Z

Closing for now as this is not getting as much traction as I thought it would for small PHP projects.
I'll complete the docs to highlight how this can be wired manually, despite I find a bit lame requiring developers to copy/paste such boilerplates.

fabpot · 2018-07-13T11:09:54Z

Much traction from whom? If this is from the core team, that's just because it does not seem to be ready.

ogizanagi · 2018-07-13T11:25:09Z

@fabpot : Not at all :) (Sorry if it was misinterpreted). Just considering we do not have much feedback from outside people regarding this for now.

…nagi) This PR was squashed before being merged into the 5.2-dev branch. Discussion ---------- [VarDumper] Add VAR_DUMPER_FORMAT=server format | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | Deprecations? | no  | Tickets | Fix #35801  | License | MIT | Doc PR | TODO  This PR follows discussion in #35801 and adds support for a `server` value for the existing `VAR_DUMPER_FORMAT` env var. It comes as well with two more things: - ~~the handler is registered as soon as the `VAR_DUMPER_FORMAT` env var is detected~~ we prevent registering another handler as soon as the `VAR_DUMPER_FORMAT` env var is set, instead of checking if there was a previous handler (which could make this env var useless in some conditions where the handler was already set by another "process") - the handler registered this way cannot be replaced. This prevents another "process" to takeover dump handling while undesired. E.g: a phpunit functional test booting the kernel to call an endpoint => the handler is replaced. It's (in a sense) a satisfying alternative to #26696 This PR means anyone can use dump with a server in any context, without changing a single line of code in the project by: - starting the server using `./vendor/bin/var-dump-server --format=html > dumps.html` - using the env var: `VAR_DUMPER_FORMAT=server [your-cli-command]` --- Previous related PRs: - #26695 - #26696 Commits ------- 82df6db [VarDumper] Add VAR_DUMPER_FORMAT=server format

ogizanagi mentioned this pull request Mar 28, 2018

[VarDumper][PhpUnitBridge] VarDumperServerListener #26696

Closed

ogizanagi commented Mar 28, 2018

View reviewed changes

javiereguiluz added Feature VarDumper labels Mar 28, 2018

javiereguiluz added this to the 4.1 milestone Mar 28, 2018

carsonbot added the Status: Needs Work label Mar 30, 2018

kardagan suggested changes Mar 30, 2018

View reviewed changes

ogizanagi added Status: Needs Review and removed Status: Needs Work labels Mar 30, 2018

nicolas-grekas reviewed Apr 22, 2018

View reviewed changes

nicolas-grekas modified the milestones: 4.1, next Apr 29, 2018

[VarDumper] Easy server dumper registration

4345647

nicolas-grekas reviewed May 1, 2018

View reviewed changes

ogizanagi closed this Jul 13, 2018

ogizanagi deleted the feature/register_server_dumper branch July 13, 2018 10:46

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

This was referenced Feb 28, 2020

[VarDumper] Add ddf() and dumpf() functions to dump to a file #35801

Closed

[VarDumper] Add VAR_DUMPER_FORMAT=server format #35967

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VarDumper] Easy server dumper registration #26695

[VarDumper] Easy server dumper registration #26695

ogizanagi commented Mar 28, 2018 •

edited

Loading

ogizanagi Mar 28, 2018 •

edited

Loading

kardagan Mar 30, 2018

ogizanagi Mar 30, 2018

kardagan commented Apr 3, 2018

ogizanagi commented Apr 3, 2018 •

edited

Loading

ogizanagi commented Apr 16, 2018

kardagan commented Apr 17, 2018

ogizanagi commented Apr 20, 2018 •

edited

Loading

nicolas-grekas Apr 22, 2018 •

edited

Loading

ogizanagi Apr 29, 2018

nicolas-grekas Apr 29, 2018

ogizanagi commented May 1, 2018

nicolas-grekas left a comment

nicolas-grekas May 1, 2018

ogizanagi commented Jul 13, 2018

fabpot commented Jul 13, 2018

ogizanagi commented Jul 13, 2018 •

edited

Loading

[VarDumper] Easy server dumper registration #26695

[VarDumper] Easy server dumper registration #26695

Conversation

ogizanagi commented Mar 28, 2018 • edited Loading

ogizanagi Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

kardagan Mar 30, 2018

Choose a reason for hiding this comment

ogizanagi Mar 30, 2018

Choose a reason for hiding this comment

kardagan commented Apr 3, 2018

ogizanagi commented Apr 3, 2018 • edited Loading

ogizanagi commented Apr 16, 2018

kardagan commented Apr 17, 2018

ogizanagi commented Apr 20, 2018 • edited Loading

nicolas-grekas Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

ogizanagi Apr 29, 2018

Choose a reason for hiding this comment

nicolas-grekas Apr 29, 2018

Choose a reason for hiding this comment

ogizanagi commented May 1, 2018

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas May 1, 2018

Choose a reason for hiding this comment

ogizanagi commented Jul 13, 2018

fabpot commented Jul 13, 2018

ogizanagi commented Jul 13, 2018 • edited Loading

ogizanagi commented Mar 28, 2018 •

edited

Loading

ogizanagi Mar 28, 2018 •

edited

Loading

ogizanagi commented Apr 3, 2018 •

edited

Loading

ogizanagi commented Apr 20, 2018 •

edited

Loading

nicolas-grekas Apr 22, 2018 •

edited

Loading

ogizanagi commented Jul 13, 2018 •

edited

Loading