Skip to content

[Console] Restore SHELL_VERBOSITY after a command is ran #61033

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: 7.3
Choose a base branch
from
Open

[Console] Restore SHELL_VERBOSITY after a command is ran #61033

wants to merge 1 commit into from
+125 −30

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Jul 4, 2025 •
edited by OskarStark
Loading

Q A
Branch? 7.2
Bug fix? yes
New feature? no
Deprecations? no
Issues
License MIT

Considereing the following code:

require __DIR__.'/vendor/autoload.php';

use Symfony\Component\Console\Application;
use Symfony\Component\Console\Attribute\AsCommand;
use Symfony\Component\Console\Input\ArgvInput;
use Symfony\Component\Console\Output\OutputInterface;

#[AsCommand('a')]
class A
{
    public function __invoke(OutputInterface $output)
    {
        $output->writeln('Command A executed');

        return 0;
    }
}

#[AsCommand('b')]
class B
{
    public function __invoke(OutputInterface $output)
    {
        $output->writeln('Command B executed');

        return 0;
    }
}

#[AsCommand('main')]
class Main
{
    public function __construct(
        private Application $app
    ) {}

    public function __invoke(OutputInterface $output)
    {
        $this->app->setAutoExit(false);

        $this->app->run(new ArgvInput([__FILE__, 'a', '--quiet']));

        $this->app->run(new ArgvInput([__FILE__, 'b']));

        $output->writeln('Main command executed');

        return 0;
    }
}

$app = new Application();
$app->addCommand(new A());
$app->addCommand(new B());
$app->addCommand(new Main($app));

$app->run();

Without this patch,

  • the output of B command is not displayed. It should be!
  • But the output Main command executed is. It should be, it's correct. But it's hard to understand. the current $output has already been configured, and it's not silenced by --quiet yet

@lyrixx lyrixx requested a review from chalasr as a code owner July 4, 2025 08:54
@carsonbot carsonbot added this to the 7.2 milestone Jul 4, 2025
@OskarStark OskarStark changed the title [Console] Restore SHELL_VERBOSITY after a command is ran [Console] Restore SHELL_VERBOSITY after a command is ran Jul 4, 2025
welcoMattic
welcoMattic reviewed Jul 4, 2025
View reviewed changes
@lyrixx lyrixx force-pushed the shell-verbosity branch from 236c06a to 1d0837c Compare July 4, 2025 15:42
GromNaN
GromNaN requested changes Jul 4, 2025
View reviewed changes
src/Symfony/Component/Console/Application.php
Comment on lines +239 to +242
if (\function_exists('putenv')) {
@putenv('SHELL_VERBOSITY='.$prevShellVerbosity);
}
$_ENV['SHELL_VERBOSITY'] = $prevShellVerbosity;
$_SERVER['SHELL_VERBOSITY'] = $prevShellVerbosity;
Copy link
Member

Choose a reason for hiding this comment

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

I try to figure out what could cause an issue. Here, we assume that all 3 have the same value initially, so we reset all 3 to the same value.
That works for me.

@lyrixx lyrixx force-pushed the shell-verbosity branch from 1d0837c to 570e67d Compare July 11, 2025 09:35
@lyrixx lyrixx requested a review from GromNaN July 25, 2025 10:30
@lyrixx lyrixx requested a review from nicolas-grekas July 25, 2025 10:30
@lyrixx
Copy link
Member Author

lyrixx commented Aug 6, 2025

ping @GromNaN & @chalasr

@chalasr
Copy link
Member

chalasr commented Aug 6, 2025

Looks good. Can you think of a test case that would help avoiding regressions?

@lyrixx lyrixx changed the base branch from 7.2 to 7.3 August 6, 2025 16:14
@lyrixx
Copy link
Member Author

lyrixx commented Aug 6, 2025

I added tests + rebased + targeted branch 7.3

@chalasr
Copy link
Member

chalasr commented Aug 6, 2025

👍 push needed to re-trigger the pipeline

@lyrixx lyrixx force-pushed the shell-verbosity branch 4 times, most recently from d53e95b to 3531989 Compare August 7, 2025 08:48
@lyrixx
Copy link
Member Author

lyrixx commented Aug 7, 2025

All green 🎉 Thanks for the review

@xabbuh xabbuh modified the milestones: 7.2, 7.3 Aug 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GromNaN GromNaN GromNaN approved these changes

@welcoMattic welcoMattic welcoMattic approved these changes

@chalasr chalasr chalasr approved these changes

@nicolas-grekas nicolas-grekas Awaiting requested review from nicolas-grekas

@xabbuh xabbuh Awaiting requested review from xabbuh

@yceruto yceruto Awaiting requested review from yceruto

@kbond kbond Awaiting requested review from kbond

@dunglas dunglas Awaiting requested review from dunglas

@OskarStark OskarStark Awaiting requested review from OskarStark

@jderusse jderusse Awaiting requested review from jderusse

Assignees
No one assigned
Labels
Bug Console Status: Reviewed
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
6 participants
@lyrixx @chalasr @GromNaN @welcoMattic @xabbuh @carsonbot