Skip to content

[Console] Constant STDOUT might be undefined #34344

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
Nov 13, 2019
Merged

[Console] Constant STDOUT might be undefined #34344

merged 1 commit into from
Nov 13, 2019

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Nov 12, 2019 •
edited by chalasr
Loading

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #34341
License MIT
Doc PR N/A

@derrabus derrabus force-pushed the bugfix/windows-stdout branch from 1b564cb to dd79978 Compare November 12, 2019 20:54
@derrabus derrabus mentioned this pull request Nov 12, 2019
Closed
nicolas-grekas
nicolas-grekas reviewed Nov 12, 2019
View reviewed changes
Copy link
Member

@nicolas-grekas 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.

fopen('php://stdout', 'w')?

@derrabus
Copy link
Member Author

fopen('php://stdout', 'w')?

Would be overkill here. The constant is defined by the CLI SAPI. Thus, if the constant is not there, we're not on CLI, thus we're not on a terminal with VT100 support.

We already have a similar check in PhpUnitBridge:

symfony/src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

Lines 355 to 357 in fb4065a

if (!\defined('STDOUT')) {
return false;
}

@nicolas-grekas
Copy link
Member

I'm not sure I agree. If the check targets the sapi, let's make it crystal clear. Checks for color support all use php://stdout. STDOUT is not used anywhere else in the codebase as far as I checked. I think we should check what we want to check in a portable way, using php://stdout Sorry my previous comment was too laconic.

@derrabus
Copy link
Member Author

STDOUT is not used anywhere else in the codebase as far as I checked.

Well yes, the method I've linked in my previous comment uses the constant to check for color support as well.

symfony/src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php

Lines 353 to 383 in fb4065a

private static function hasColorSupport()
{
if (!\defined('STDOUT')) {
return false;
}
if ('Hyper' === getenv('TERM_PROGRAM')) {
return true;
}
if (\DIRECTORY_SEPARATOR === '\\') {
return (\function_exists('sapi_windows_vt100_support')
&& sapi_windows_vt100_support(STDOUT))
|| false !== getenv('ANSICON')
|| 'ON' === getenv('ConEmuANSI')
|| 'xterm' === getenv('TERM');
}
if (\function_exists('stream_isatty')) {
return stream_isatty(STDOUT);
}
if (\function_exists('posix_isatty')) {
return posix_isatty(STDOUT);
}
$stat = fstat(STDOUT);
// Check if formatted mode is S_IFCHR
return $stat ? 0020000 === ($stat['mode'] & 0170000) : false;
}

Also, I wouldn't want to change the way stdout is accessed without a Windows machine to test on. 😕

@nicolas-grekas
Copy link
Member

That's the bridge, it's not a piece of reusability, it's a tool :)
No worries about php://stdout, it does the very same job.

@derrabus derrabus force-pushed the bugfix/windows-stdout branch from dd79978 to e1bfb48 Compare November 12, 2019 22:01
@derrabus
Copy link
Member Author

All right, here you go.

@lazosweb Can you please test again? Sorry for the hassle. 😃

@lazosweb
Copy link

@derrabus . All good. No issue. That will work too.

@lazosweb lazosweb mentioned this pull request Nov 13, 2019
Closed
@chalasr chalasr added this to the 3.4 milestone Nov 13, 2019
@chalasr
Copy link
Member

chalasr commented Nov 13, 2019

Should be merged after #34346 and rebased on 3.4.

@derrabus
Copy link
Member Author

Should be merged after #34346 and rebased on 3.4.

@chalasr Feel free to squash my change into your PR. I don't think, we need to backport a buggy solution first. 😉

@chalasr chalasr changed the base branch from 4.3 to 3.4 November 13, 2019 07:07
@chalasr chalasr force-pushed the bugfix/windows-stdout branch from e1bfb48 to 4225009 Compare November 13, 2019 07:10
@chalasr chalasr force-pushed the bugfix/windows-stdout branch from 4225009 to bb8c82c Compare November 13, 2019 07:12
@chalasr chalasr mentioned this pull request Nov 13, 2019
Closed
fabpot pushed a commit that referenced this pull request Nov 13, 2019
@nicolas-grekas
bug #34344 [Console] Constant STDOUT might be undefined (nicolas-grekas)
32bde39 
This PR was merged into the 3.4 branch.

Discussion
----------

[Console] Constant STDOUT might be undefined

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #34341
| License       | MIT
| Doc PR        | N/A

Commits
-------

bb8c82c [Console] Constant STDOUT might be undefined.
@fabpot fabpot merged commit bb8c82c into symfony:3.4 Nov 13, 2019
@nicolas-grekas
Copy link
Member

Thank you @derrabus

@fabpot fabpot mentioned this pull request Nov 13, 2019
Merged
@derrabus derrabus deleted the bugfix/windows-stdout branch November 13, 2019 08:53
This was referenced Nov 13, 2019
Merged
Merged
Merged
@dependabot dependabot bot mentioned this pull request Mar 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@chalasr chalasr chalasr approved these changes

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

Successfully merging this pull request may close these issues.
6 participants
@derrabus @nicolas-grekas @lazosweb @chalasr @fabpot @carsonbot