Skip to content

[Console] Suppress proc_open errors within Terminal::readFromProcess#58332

Merged
nicolas-grekas merged 1 commit intosymfony:5.4from
fritzmg:suppress-read-from-process-errors
Sep 28, 2024
Merged

[Console] Suppress proc_open errors within Terminal::readFromProcess#58332
nicolas-grekas merged 1 commit intosymfony:5.4from
fritzmg:suppress-read-from-process-errors

Conversation

@fritzmg
Copy link
Copy Markdown
Contributor

@fritzmg fritzmg commented Sep 20, 2024 •
edited
Loading

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

When instantiating SymfonyStyle in a command it will try to determine the maximum width of the current console interface.

symfony/src/Symfony/Component/Console/Style/SymfonyStyle.php

Line 53 in 6687e4e

$width = (new Terminal())->getWidth() ?: self::MAX_LINE_LENGTH;

This will execute stty -a | grep columns down the line. Access to stty might be disallowed however, resulting in the following error:

ErrorException: Warning: proc_open(): Exec failed: Permission denied
#16 /vendor/symfony/console/Terminal.php(220): Symfony\Component\Console\Terminal::readFromProcess
#15 /vendor/symfony/console/Terminal.php(204): Symfony\Component\Console\Terminal::getSttyColumns
#14 /vendor/symfony/console/Terminal.php(170): Symfony\Component\Console\Terminal::initDimensionsUsingStty
#13 /vendor/symfony/console/Terminal.php(153): Symfony\Component\Console\Terminal::initDimensions
#12 /vendor/symfony/console/Terminal.php(94): Symfony\Component\Console\Terminal::getWidth
#11 /vendor/symfony/console/Style/SymfonyStyle.php(55): Symfony\Component\Console\Style\SymfonyStyle::__construct
#10 /vendor/symfony/messenger/Command/ConsumeMessagesCommand.php(136): Symfony\Component\Messenger\Command\ConsumeMessagesCommand::interact
#9 /vendor/symfony/console/Command/Command.php(311): Symfony\Component\Console\Command\Command::run

(Stack Trace actually from Symfony 6)

The phpDoc of Terminal::getSttyColumns states

Runs and parses stty -a if it's available, suppressing any error output.

The latter might refer to ['suppress_errors' => true] (though I am not sure) - which is a Windows only functionality. In any case, since Terminal::readFromProcess already checks for

if (!$process = proc_open(…)) {
    return null;
}

and

if (!\is_resource($process)) {
    return null;
}

upstream in Symfony 6/7, indicating that proc_open might fail - this error can additionally be suppressed using @. Besides, Process::start also uses @proc_open (added in 099481f "Prevent warning in proc_open()").

@fritzmg fritzmg requested a review from chalasr as a code owner September 20, 2024 10:49
@carsonbot carsonbot added this to the 7.2 milestone Sep 20, 2024
@carsonbot
Copy link
Copy Markdown

carsonbot commented Sep 20, 2024

Hey!

Thanks for your PR. You are targeting branch "7.2" but it seems your PR description refers to branch "5.4".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@fritzmg fritzmg changed the base branch from 7.2 to 5.4 September 20, 2024 10:50
@fritzmg fritzmg mentioned this pull request Sep 20, 2024
Closed
@fritzmg
Copy link
Copy Markdown
Contributor Author

fritzmg commented Sep 22, 2024

Instead of using proc_open directly, we could also use Process, which would likely solve the issue. wdyt?

@nicolas-grekas nicolas-grekas modified the milestones: 7.2, 5.4 Sep 28, 2024
@nicolas-grekas
Copy link
Copy Markdown
Member

nicolas-grekas commented Sep 28, 2024

Thank you @fritzmg.

@nicolas-grekas nicolas-grekas merged commit 2be812d into symfony:5.4 Sep 28, 2024
@fritzmg
Copy link
Copy Markdown
Contributor Author

fritzmg commented Sep 28, 2024 •
edited
Loading

@nicolas-grekas should I do a separate PR for Symfony 6, as the code is different there?

@fritzmg fritzmg deleted the suppress-read-from-process-errors branch September 28, 2024 11:22
@xabbuh
Copy link
Copy Markdown
Member

xabbuh commented Sep 28, 2024

@fritzmg I think merging 5.4 up solved this. But please send a PR if you think that something's still missing.

@fritzmg
Copy link
Copy Markdown
Contributor Author

fritzmg commented Sep 28, 2024

Ah yes, all good 👍

This was referenced Oct 27, 2024
Merged
Merged
Merged
@blumilk-renovate blumilk-renovate bot mentioned this pull request Apr 4, 2025
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xabbuh xabbuh xabbuh approved these changes

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees

No one assigned

Labels

Bug Console Status: Reviewed

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

4 participants

@fritzmg @carsonbot @nicolas-grekas @xabbuh