8000 [Process] Regression: Error output-handling is affecting Drupal core development by TravisCarden · Pull Request #57317 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Process] Regression: Error output-handling is affecting Drupal core development #57317

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
Closed
wants to merge 3 commits into from
Closed

[Process] Regression: Error output-handling is affecting Drupal core development #57317

wants to merge 3 commits into from

Conversation

TravisCarden
Copy link
Contributor
@TravisCarden TravisCarden commented Jun 4, 2024 •
edited
Loading
Q A
Branch? 7.1
Bug fix? yes
New feature? no
Deprecations? no
Issues n/a
License MIT

tl;dr: Since #52409, \Symfony\Component\Process\Process no longer captures error output when attempting to run a non-existent command on Linux--behavior Drupal core was relying on. The regression was first released in v7.1.0-BETA1 and persists into 7.2.

Details

#52409 changed how the commandline is passed to proc_open(), apparently triggering a bug in PHP (php/php-src#12589) whereby invoking a non-existent command on Linux no longer provides any error output. That's important behavior for a major error condition. Hence my surprise when I discovered that it was a known issue before the PR was merged--that it was explicitly called out and that tests were simply updated to avoid it.

As a consequence of the change, tests started failing on Drupal core component Composer Stager. This has caused a ripple effect and taken me away from other Drupal core work. I would really like to see it fixed quickly.

Steps to Reproduce

This PR demonstrates the problem. It does not yet fix it. (Since the regression was part of a performance enhancement, not a functional change, perhaps it wouldn't be out of the question to just revert it, if necessary.) This begins by temporarily reverting the regression and adding a test that we can see passing on CI[1]. Then it puts the regression back in place so we can see it failing.

This...

$p = new Process(['invalid_command']);
$p->run();
var_dump($p->getErrorOutput());

🟢 Used to print this on Linux:

string(45) "sh: line 0: exec: invalid_command: not found
"

🔴 Now it prints this:

string(0) ""

To-do

  • Fix the regression
  • Ensure adequate test coverage
  • Update src/**/CHANGELOG.md files

[1] Well, not very obviously, I guess, since a bunch of tests were already failing.

@carsonbot

This comment was marked as outdated.

Sign in to view
@TravisCarden
Put back regression in \Symfony\Component\Process\Process::start().
540f1dc 
Revert "Temporarily revert regression in \Symfony\Component\Process\Process::start()."

This reverts commit 869daa4.
@TravisCarden TravisCarden marked this pull request as ready for review June 4, 2024 18:15
@carsonbot carsonbot added this to the 7.1 milestone Jun 4, 2024
@carsonbot

This comment was marked as resolved.

Sign in to view
This was referenced Jun 6, 2024
Merged
Closed
@TravisCarden TravisCarden changed the title [Process] Regression: Error output isn't always captured anymore [Process] Regression: Error output-handling is affecting Drupal core development Jun 7, 2024
@nicolas-grekas
Copy link
Member

Reverting #52409 would be a step backward. What I don't understand from the description is how this is affecting you?
Were you relying on the details of the error output? If yes, then I'd argue this is too fragile and I would seek for an alternative way to achieve what you do with this sniffing. If not, then please expand.

@nicolas-grekas
Copy link
Member

Using new Process(['sh', '-c', 'invalid_command']); might be a way forward for you.
Using the PhpExecutableFinder might be another one, depending on what you need.

@nicolas-grekas
Copy link
Member

Let me close here since we won't merge this patch.

@TravisCarden
8FD9 Copy link
Contributor Author

Thanks, @nicolas-grekas. Just for a little parting clarity as we close this, I wasn't depending on what the output was. I was depending on the fact that there was output. When I initially filed this, I thought Process:: getErrorOutput() had just plain stopped working. Only as I continued debugging did I discover the more limited scope of the problem, i.e., it only occurs when the invoked command doesn't exist. At least it's clearly documented now, in case someone else runs into the same problem. Thanks.

@TravisCarden TravisCarden deleted the feature/getErrorOutput-regression branch June 26, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug Process Status: Needs Review
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.
3 participants
@TravisCarden @carsonbot @nicolas-grekas
0