NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.3k

[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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:3.4fromderrabus:bugfix/windows-stdout

Nov 13, 2019

Merged

[Console] Constant STDOUT might be undefined#34344

fabpot merged 1 commit intosymfony:3.4fromderrabus:bugfix/windows-stdout

Nov 13, 2019

Conversation

Copy link

Member

derrabus commentedNov 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

carsonbot added Status: Needs Review Console Bug labels

Nov 12, 2019

derrabus force-pushed thebugfix/windows-stdout branch from1b564cb todd79978Compare

November 12, 2019 20:54

derrabus mentioned this pull request

Nov 12, 2019

[Console] Use of undefined constant STDOUT#34341

Closed

nicolas-grekas reviewed

Nov 12, 2019

View reviewed changes

Copy link

Member

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')?

Copy link

MemberAuthor

derrabus commentedNov 12, 2019

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 infb4065a

	if (!\defined('STDOUT')) {
	returnfalse;
	}

Copy link

Member

nicolas-grekas commentedNov 12, 2019

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.

Copy link

MemberAuthor

derrabus commentedNov 12, 2019

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 infb4065a

	privatestaticfunctionhasColorSupport()
	{
	if (!\defined('STDOUT')) {
	returnfalse;
	}

	if ('Hyper' ===getenv('TERM_PROGRAM')) {
	returntrue;
	}

	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')) {
	returnstream_isatty(STDOUT);
	}

	if (\function_exists('posix_isatty')) {
	returnposix_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. 😕

Copy link

Member

nicolas-grekas commentedNov 12, 2019

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 force-pushed thebugfix/windows-stdout branch fromdd79978 toe1bfb48Compare

November 12, 2019 22:01

Copy link

MemberAuthor

derrabus commentedNov 12, 2019

All right, here you go.

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

Copy link

lazosweb commentedNov 13, 2019

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

lazosweb mentioned this pull request

Nov 13, 2019

[6.x] Use of undefined constant STDOUTlaravel/framework#30571

Closed

chalasr approved these changes

Nov 13, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Nov 13, 2019

chalasr added this to the3.4 milestone

Nov 13, 2019

Copy link

Member

chalasr commentedNov 13, 2019

Should be merged after#34346 and rebased on 3.4.

Copy link

MemberAuthor

derrabus commentedNov 13, 2019

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 changed the base branch from4.3 to3.4

November 13, 2019 07:07

chalasr force-pushed thebugfix/windows-stdout branch frome1bfb48 to4225009Compare

November 13, 2019 07:10

[Console] Constant STDOUT might be undefined.

bb8c82c

chalasr force-pushed thebugfix/windows-stdout branch from4225009 tobb8c82cCompare

November 13, 2019 07:12

chalasr mentioned this pull request

Nov 13, 2019

Backport bug #34085 [Console] Detect dimensions using mode CON if vt100 is supported#34346

Closed

nicolas-grekas approved these changes

Nov 13, 2019

View reviewed changes

fabpot pushed a commit that referenced this pull request

Nov 13, 2019

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/ACommits-------bb8c82c [Console] Constant STDOUT might be undefined.