NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Console] progress bar fix#19012

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 5 commits intosymfony:masterfromfabpot:console-progress-bar-fix

Jun 17, 2016

Merged

[Console] progress bar fix#19012

fabpot merged 5 commits intosymfony:masterfromfabpot:console-progress-bar-fix

Jun 17, 2016

Conversation

Copy link

Member

fabpot commentedJun 9, 2016•
edited
Loading

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#13019
License	MIT
Doc PR	-

This is#16490 where I've simplified the code as much as possible and added a test for the bug we're trying to fix.

The main change is the renaming of theTerminalDimensionsProvider to justTerminal. The new class can probably be useful to add more about the terminal.

carsonbot added Status: Needs Review Console Bug labels

Jun 9, 2016

fabpot mentioned this pull request

Jun 9, 2016

[Console] progress bar fix#16490

Closed

fabpot force-pushed theconsole-progress-bar-fix branch from85ee0c8 todb12bb6Compare

June 9, 2016 15:14

Copy link

Member

stof commentedJun 9, 2016

There is still 1 drawback here: as the ProgressBar uses its own Terminal instance, it will ignore any explicit overwrite done in the Application.

Copy link

Member

stof commentedJun 9, 2016

do you want to merge this in 3.2 only or in older branches as a bugfix ?

Copy link

MemberAuthor

fabpot commentedJun 9, 2016

I'm aware of the drawback. I don't see how to fix this and TBH, I don't think it matters too much as playing with the dimensions should only be done when testing an app.

Only for 3.2.

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

please add the deprecation warnings too

Copy link

Member

javiereguiluz commentedJun 9, 2016

The newSymfony\Component\Console\Terminal class is missing in the PR.

fabpot force-pushed theconsole-progress-bar-fix branch 5 times, most recently froma095d0b to8d71affCompare

June 9, 2016 15:41

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

missing param name, makign the phpdoc invalid

fabpot force-pushed theconsole-progress-bar-fix branch 2 times, most recently from60a2d62 toa38bb97Compare

June 9, 2016 16:26

Copy link

Member

nicolas-grekas commentedJun 9, 2016

It could be useful to configure width+height with an env var, especially in tests.
This would allow testing commands that hardcodenew SymfonyStyle and would replace thestty cols 120 in the .travis.yml file (and could replace thegetTerminal extension points and replace related mocks).

Copy link

MemberAuthor

fabpot commentedJun 10, 2016

@nicolas-grekas Done. I've kept the config in .travis.yml

Copy link

Member

nicolas-grekas commentedJun 10, 2016

LGTM, except a test failure

stof reviewed

Jun 10, 2016

View reviewed changes

src/Symfony/Component/Console/Application.php Outdated

		$this->terminalDimensions =array($width,$height);
		@trigger_error(sprintf('%s is deprecated as of 3.2 and will be removed in 4.0. Set the ANSICON env var instead.',__METHOD__, ArgumentResolverInterface::class),E_USER_DEPRECATED);

		putenv('ANSICON='.$width.'x'.$height);

Copy link

Member

stofJun 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this is a bad idea, as it will also impact the detection of color support on Windows

Copy link

Member

stofJun 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We should support a different env var following the same format if we want to go this way (but keeping support for the ANSICON one for auto-detection)

Copy link

Member

stofJun 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

btw, this code is broken, as it does not invalidate the Terminal cache

Copy link

Member

nicolas-grekasJun 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What about using COLUMNS and LINES? These are what bash uses (they are magic variables that can be overriden, tryecho $COLUMNS).

fabpot force-pushed theconsole-progress-bar-fix branch from6e105a8 to2eeb7bdCompare

June 13, 2016 19:50

fabpot force-pushed theconsole-progress-bar-fix branch 2 times, most recently from534d825 toc92b2b4Compare

June 16, 2016 13:11

Copy link

MemberAuthor

fabpot commentedJun 16, 2016

Broken test on HHVM not related.

Copy link

Member

nicolas-grekas commentedJun 16, 2016

👍 with this patch:

--- a/src/Symfony/Bundle/SecurityBundle/Tests/Functional/UserPasswordEncoderCommandTest.php+++ b/src/Symfony/Bundle/SecurityBundle/Tests/Functional/UserPasswordEncoderCommandTest.php@@ -28,6 +28,8 @@ class UserPasswordEncoderCommandTest extends WebTestCase    public function testEncodePasswordEmptySalt()    {+        putenv('COLUMNS=120');+        $this->passwordEncoderCommandTester->execute(array(            'command' => 'security:encode-password',

Copy link

Member

stof commentedJun 16, 2016

@fabpot are you sure ? the failing test is related to line wrapping in commands.

I think one of your Console test does not properly reset the env variables on teardown, and so it impacts subsequent tests. Non-hhvm builds are unaffected because they use a separate PHP process per component to run things in parallel

stof reviewed

Jun 16, 2016

View reviewed changes

src/Symfony/Component/Console/Application.php

		@trigger_error(sprintf('%s is deprecated as of 3.2 and will be removed in 4.0. Set the COLUMNS and LINES env vars instead.',__METHOD__, ArgumentResolverInterface::class),E_USER_DEPRECATED);

		putenv('COLUMNS='.$width);
		putenv('LINES='.$height);

Copy link

Member

stofJun 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

this does not work in case the dimensions were already retrieved previously, because of the Terminal internal cache

Copy link

MemberAuthor

fabpotJun 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

fixed

fabpot force-pushed theconsole-progress-bar-fix branch 4 times, most recently from660b6e8 tob233974Compare

June 16, 2016 15:54

Copy link

MemberAuthor

fabpot commentedJun 16, 2016

should be good now :)

fabpot force-pushed theconsole-progress-bar-fix branch fromb233974 to355ad6cCompare

June 16, 2016 16:11

nicolas-grekas reviewed

Jun 17, 2016

View reviewed changes

src/Symfony/Component/Console/Terminal.php Outdated

		}
		}

		if (0 === self::$width) {

Copy link

Member

nicolas-grekasJun 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

not sure this check works: when is this zero?

Copy link

MemberAuthor

fabpotJun 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

that's old code, removed.

fabpot force-pushed theconsole-progress-bar-fix branch from355ad6c todba73e7Compare

June 17, 2016 07:00

TomasVotrubaand others added5 commits

June 17, 2016 19:36

[Console] ProgressBar - adjust to the window width (static)

b030c24

fixed CS, simplified code

8f206c8

deprecated some Console Application methods

a589635

fixed logic

bf7a5c5

switched to use COLUMNS and LINES env vars to change terminal dimensions

2f81247

fabpot force-pushed theconsole-progress-bar-fix branch fromdba73e7 to2f81247Compare

June 17, 2016 17:36

Copy link

MemberAuthor

fabpot commentedJun 17, 2016

Thank you@TomasVotruba.

fabpot merged commit2f81247 intosymfony:master

Jun 17, 2016

fabpot added a commit that referenced this pull request

Jun 17, 2016

feature#19012[Console] progress bar fix (TomasVotruba, fabpot)

afed2f8

This PR was merged into the 3.2-dev branch.Discussion----------[Console] progress bar fix| Q             | A| ------------- | ---| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#13019| License       | MIT| Doc PR        | -This is#16490 where I've simplified the code as much as possible and added a test for the bug we're trying to fix.The main change is the renaming of the `TerminalDimensionsProvider` to just `Terminal`. The new class can probably be useful to add more about the terminal.Commits-------2f81247 switched to use COLUMNS and LINES env vars to change terminal dimensionsbf7a5c5 fixed logica589635 deprecated some Console Application methods8f206c8 fixed CS, simplified codeb030c24 [Console] ProgressBar - adjust to the window width (static)

Copy link

Contributor

TomasVotruba commentedJun 17, 2016

Thank you@fabpot for finishing!

fabpot mentioned this pull request

Oct 27, 2016

Release v3.2.0-BETA1#20317

Merged

chalasr mentioned this pull request

May 20, 2017

[Console] Remove deprecated features#22799

Merged

nicolas-grekas added a commit that referenced this pull request

May 21, 2017

feature#22799[Console] Remove deprecated features (chalasr)

79e880c

This PR was merged into the 4.0-dev branch.Discussion----------[Console] Remove deprecated features| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | yes| Deprecations? | no| Tests pass?   | yes| Fixed tickets | ~~#18999#19012~~| License       | MIT| Doc PR        | n/aCommits-------4cde3b9 [Console] Remove deprecated features

derrabus mentioned this pull request

Dec 14, 2023

[Console] ProgressBar behavior (question)#13019

Closed

Labels

Bug Console Status: Needs Review

		protectedfunctiongetTerminalWidth()
		{
		$dimensions =$this->getTerminalDimensions();

		/**
		* Sets the terminal height.
		*
		* @param int

Movatterモバイル変換

Uh oh!

[Console] progress bar fix#19012

[Console] progress bar fix#19012

Uh oh!

Conversation

fabpot commentedJun 9, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

stof commentedJun 9, 2016

Uh oh!

stof commentedJun 9, 2016

Uh oh!

fabpot commentedJun 9, 2016

Uh oh!

fabpot commentedJun 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiereguiluz commentedJun 9, 2016

Uh oh!

fabpot commentedJun 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedJun 9, 2016

Uh oh!

fabpot commentedJun 10, 2016

Uh oh!

nicolas-grekas commentedJun 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commentedJun 16, 2016

Uh oh!

nicolas-grekas commentedJun 16, 2016

Uh oh!

stof commentedJun 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commentedJun 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commentedJun 17, 2016

Uh oh!

TomasVotruba commentedJun 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fabpot commentedJun 9, 2016•
edited
Loading