Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Silence warnings during tty detection #43149

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
Merged
merged 1 commit into from
Jan 26, 2022
Merged

Silence warnings during tty detection #43149

merged 1 commit into from
Jan 26, 2022

Conversation

neclimdul
Copy link
Contributor

@neclimdul neclimdul commented Sep 23, 2021 •
edited by nicolas-grekas
Loading

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets n/a
License MIT
Doc PR

When the DeprecationErrorHandler checks if it should colorize the output, in certain circumstances it can trigger the following warning:

 Warning: stream_isatty(): cannot cast a filtered stream on this system in /app/vendor/symfony/phpunit-bridge/DeprecationErrorHandler.php on line 411

This is triggered by some admittedly poor behavior inside of php. stream_isatty checks by doing a cast and looking for a failure and one of those failures logs a warning. You generally don't want to trigger warnings during a capabilities check(that's why I'm filing this) but PHP is doing just that.

This can lead to test suite failures since warnings are generally treated as failures by most test suites.

I think a test case to trigger the deprecation handler might be tricky but a simplified test case to trigger the warning in php looks like this.

class strtoupper_filter extends php_user_filter {
  #[\ReturnTypeWillChange]
  function filter($in, $out, &$consumed, $closing)
  {
    while ($bucket = stream_bucket_make_writeable($in)) {
      $bucket->data = strtoupper($bucket->data);
      $consumed += $bucket->datalen;
      stream_bucket_append($out, $bucket);
    }
    return PSFS_PASS_ON;
  }
}

$stdout = fopen('php://stdout', 'wb');
stream_filter_register('capture', strtoupper_filter::class);
stream_filter_append($stdout, 'capture');
var_export(stream_isatty($stdout));

https://3v4l.org/0EHT4

A real world example of a test suite that is wrapping stdout with a stream wrapper during testing is Drupal. Feature addition

References:
stream_isatty definition
cast line that triggers warning

@carsonbot carsonbot added this to the 5.3 milestone Sep 23, 2021
@carsonbot
Copy link

Hey!

I think @greg0ire has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@stof
Copy link
Member

stof commented Sep 24, 2021

@neclimdul please also report that to PHP. I think the behavior deserved being fixed in PHP, even if we can work around it.

@neclimdul
Copy link
Contributor Author

Sure thing. Not sure this sort of thing will get a lot of attention but it would be a good fix in some future version of PHP.

https://bugs.php.net/bug.php?id=81475

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 26, 2022 •
edited
Loading

Can you please target 4.4, and fix all calls to the function (there are more in the codebase)?

@nicolas-grekas nicolas-grekas changed the title Silence warnings during DeprecationErrorHandler tty detection Silence warnings during tty detection Jan 26, 2022
@nicolas-grekas nicolas-grekas modified the milestones: 5.3, 4.4 Jan 26, 2022
@nicolas-grekas nicolas-grekas changed the base branch from 5.3 to 4.4 January 26, 2022 16:15
@nicolas-grekas
Copy link
Member

Thank you @neclimdul.

@nicolas-grekas nicolas-grekas merged commit 67de62f into symfony:4.4 Jan 26, 2022
@neclimdul
Copy link
Contributor Author

sorry i didn't see the note about the target. thanks for taking care of this!

This was referenced Jan 28, 2022
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@dunglas dunglas Awaiting requested review from dunglas

@lyrixx lyrixx Awaiting requested review from lyrixx

@wouterj wouterj Awaiting requested review from wouterj

@xabbuh xabbuh Awaiting requested review from xabbuh

@yceruto yceruto Awaiting requested review from yceruto

Assignees
No one assigned
Labels
Bug Status: Needs Review
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
4 participants
@neclimdul @carsonbot @stof @nicolas-grekas
Morty Proxy This is a proxified and sanitized view of the page, visit original site.