Update DumpServer to add functional test for the ServerDumpCommand.
This PR will allow to use DumpServer in the ServerLogCommand and test this command that had regressions in the past.

It's a try following a comment in another closed PR (#53518 (comment) cc@nicolas-grekas)

carsonbot added Status: Needs Review VarDumper labels

Feb 2, 2024

carsonbot added this to the7.1 milestone

Feb 2, 2024

louismariegaborit force-pushed thevar_dumper_add_tests branch 2 times, most recently frome1f6536 to0573d8dCompare

February 2, 2024 22:34

Copy link

ContributorAuthor

louismariegaborit commentedFeb 2, 2024

@nicolas-grekas I tried to use DumpServer with the update in this PR and it allows testing ServerLogCommand. :)
I do a dedicated PR after this.

I wonder if it should be targeted 7.1 or 6.4

OskarStark changed the title~~[VarDumper] Add functional test for ServerDumpCommand~~[VarDumper] Add functional test forServerDumpCommand

Feb 5, 2024

nicolas-grekas reviewed

Feb 22, 2024

View reviewed changes

Copy link

Member

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.

Here are some comments to help move forward hopefully.

src/Symfony/Component/VarDumper/Server/DumpServer.php OutdatedShow resolvedHide resolved

src/Symfony/Component/VarDumper/Server/DumpServer.php

		[$data, $context] = $payload;

		$callback($data, $context, $clientId);
		$callback($clientId, $message);

Copy link

Member

Choose a reason for hiding this comment

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

that's a BC break as this changes the contract of the callback.
Also I'm not sure this makes sense it terms of responsibility.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

I did this followingyour comment in the PR#53518.
Do you have an idea ?
To help you, herea try of usage on the monolog bridge to see possibilities.

Copy link

Member

Choose a reason for hiding this comment

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

I'm sorry we should try to keep the existing signature callback.
It's fine to me if we don't factorize the code with ServerLogCommand
The motivation for this PR is testing. Can't we do an integration test instead, where we start a real process?