Movatterモバイル変換


[0]ホーム

URL:


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

Sign up
Appearance settings

[Console] Fix clear line with question in section#48089

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

Merged
chalasr merged 1 commit intosymfony:6.1frommaxbeckers:patch-47411
Dec 14, 2022

Conversation

@maxbeckers
Copy link
Contributor

QA
Branch?6.1
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#47411
LicenseMIT
Doc PR-

In the issue#47411 is the current behavior described (with videos).

The problem is in a section using a question and a clear. Then one line is not cleared because of thereturn so submit the input.

NOTICE: This bug might be as well in the versions 4.4+, but in the versions < 6.1 it would be more complicated to fix, because theSymfonyStyle does not have the property$output in this versions.

@carsonbot
Copy link

Hey!

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

Cheers!

Carsonbot

@bhujagendra-ishaya
Copy link
Contributor

Hi@maxbeckers

Thanks for the provided fix.

To be able to easier review the fix, would you or@bigfoot90 mind adding a full working code (complete file with imports and stuff) to here or the original issue#47411? - I'd rather also run the code, than just the tests.

Also, did you consider to include your change mentioned in comment#47411 (comment)?

@maxbeckers
Copy link
ContributorAuthor

@bhujagendra-ishaya thanks for your comment.

The change mentioned in the comment can be igored, because it's not a problem. It was my fault because of any issues in my vendor.

here is the reproducer with the fix in vendors:
test-48089.zip

Just run on CLI:php testcommand.php in the root.

Copy link
Contributor

@bhujagendra-ishayabhujagendra-ishaya left a comment

Choose a reason for hiding this comment

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

Ok, I've tested your provided scirpt ...

  1. with your provided vendor dir
  2. with a freshly installed vendor dir

... on the following two environments:

Microsoft Windows [Version 10.0.19044.2130]> php -vPHP 8.1.12 (cli) (built: Oct 25 2022 18:20:48) (NTS Visual C++ 2019 x64)Copyright (c) The PHP GroupZend Engine v4.1.12, Copyright (c) Zend Technologies

... and ...

$ lsb_release -aNo LSB modules are available.Distributor ID:UbuntuDescription:Ubuntu 22.04.1 LTSRelease:22.04Codename:jammy$ php -vPHP 8.1.12 (cli) (built: Oct 28 2022 17:39:57) (NTS)Copyright (c) The PHP GroupZend Engine v4.1.12, Copyright (c) Zend Technologies    with Zend OPcache v8.1.12, Copyright (c), by Zend Technologies

I can confirm thatin both environments, ...

  • the issuehappens withsymfony/console v6.1.7,
  • the issueis fixed withsymfony/console v6.1.7 + fix applied.

maxbeckers reacted with heart emoji
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

Not sure I precisely understand what's going on, but LGTM if you tested and it fixes the issue.

@maxbeckers
Copy link
ContributorAuthor

@nicolas-grekas theConsoleSectionOutput is counting it's lines. But when there is a input in the section and the input is confirmed with ENTER, the new line of the ENTER was not added to the section. This is what this fix is doing. Hope this helps to understand this fix.

@chalasr
Copy link
Member

Thanks for improving sections support,@maxbeckers. That's much appreciated.

@chalasrchalasr merged commitcaffee8 intosymfony:6.1Dec 14, 2022
@chalasr
Copy link
Member

PR welcome on 5.4 if you find a way to have the same behavior there.

@maxbeckers
Copy link
ContributorAuthor

@chalasr I could try a fix after my vacation time in the beginning of the next year, but i don't know if it's worth the effort. Because the fix will then be a way more complex.

@chalasr
Copy link
Member

@maxbeckers I guess we can live with that. Thanks!

@nicolas-grekas
Copy link
Member

@maxbeckers would you still have time to check the failures on branch 6.2 that came up after merging this PR up?

@maxbeckers
Copy link
ContributorAuthor

maxbeckers commentedDec 14, 2022
edited
Loading

@nicolas-grekas sorry, but since today im in vacation and have only my smartphone. Feel free to revert the change and i'll check it after holiday.

UPDATE: This is the new test failing ... but the test output is looking to me a bit strange. Why is therefoo twice? Maybe a bug in 6.2? I'd like to investigate some time but i can only do that afer my vacation in the beginning of next year. Maybe someone has the time to have a deeper look into and fixes it or you revert and i'll do it when im back.
20221214_195058

@chalasr
Copy link
Member

Thanks for the hint. I'll have a look so we can decide depending on the result. Enjoy your vacation :)

maxbeckers reacted with thumbs up emoji

chalasr added a commit to chalasr/symfony that referenced this pull requestDec 16, 2022
…ection (maxbeckers)"This reverts commitcaffee8, reversingchanges made tof14901e.
nicolas-grekas added a commit that referenced this pull requestDec 16, 2022
…in section (maxbeckers) (chalasr)This PR was merged into the 6.1 branch.Discussion----------[Console] Revert "bug#48089  Fix clear line with question in section (maxbeckers)This reverts commitcaffee8, reversing changes made tof14901e.| Q             | A| ------------- | ---| Branch?       | 6.1| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       | /| License       | MIT| Doc PR        | -The tests added in#48089 break on branch 6.2, and after checking the fix on both 6.1 and 6.2 based on the reproducer from#47411, I feel like the behavior is still not the expected one.I propose to revert for now, hope we can fix this bug homogeneously in another PR.Commits-------30caa8d Revert "bug#48089 [Console] Fix clear line with question in section (maxbeckers)"
nicolas-grekas added a commit that referenced this pull requestDec 16, 2022
* 6.1:  [HttpKernel] fix merge  [HttpKernel] fix merge  Revert "bug#48089 [Console] Fix clear line with question in section (maxbeckers)"
nicolas-grekas added a commit that referenced this pull requestDec 16, 2022
* 6.2:  [HttpKernel] fix merge  [HttpKernel] fix merge  Revert "bug#48089 [Console] Fix clear line with question in section (maxbeckers)"  [Cache] fix lazyness of redis when using RedisTagAwareAdapter
@fabpotfabpot mentioned this pull requestDec 16, 2022
@fabpotfabpot mentioned this pull requestDec 28, 2022
chalasr added a commit that referenced this pull requestFeb 19, 2023
This PR was merged into the 6.2 branch.Discussion----------[Console] fix clear of section with question| Q             | A| ------------- | ---| Branch?       |  6.2| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       |Fix#47411| License       | MIT| Doc PR        | n/aThis PR fixes the problems to clear a section with a question included.Example Code:```    protected function execute(InputInterface $input, OutputInterface $output)    {        $section1 = $output->section();        $io = new SymfonyStyle($input, $section1);        $output->writeln("foo");        $countdown = 3;        while ($countdown > 0) {            $section1->clear();            $io->writeln('start ' . $countdown);            $io->write('foo');            $io->write(' and bar'.\PHP_EOL);            $givenAnswer = $io->ask('Dummy question?');            $section1->write('bar');            $countdown--;        }        return self::SUCCESS;    }```Output loop 1:![Screenshot 2023-01-06 142630](https://user-images.githubusercontent.com/11738128/211021767-e4109951-0519-4763-bdd0-6504ee875b46.png)Output loop 1:![Screenshot 2023-01-06 142653](https://user-images.githubusercontent.com/11738128/211021793-db987c4a-1ac5-422d-a8b9-f6c3f4a23c7f.png)There was already afix#48089 to be merged in 6.1, but the problem was that there were some changes in 6.2, so it was not possible to merge it into 6.2.So this fix is only working for 6.2, but perhaps we could find a solution as well for the older versions. But because of the changes of console it was not possible to find a solution working for all versions.`@chalasr` this fix is still with the newline always `true`https://github.com/symfony/symfony/blob/4cf9855debc26e4323429ac8d87f02df582e2893/src/Symfony/Component/Console/Output/ConsoleSectionOutput.php#L181A change of the newline to `$newline` would change the behavior. Maybe we could change that in symfony 7.To make it easier to test is here a zip with 2 testcommands in the root and the changed vendors.[test-48089.zip](https://github.com/symfony/symfony/files/10360423/test-48089.zip)Commits-------f4c5518 [Console] fix clear of section with question
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@chalasrchalasrchalasr approved these changes

+1 more reviewer

@bhujagendra-ishayabhujagendra-ishayabhujagendra-ishaya approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

6.1

Development

Successfully merging this pull request may close these issues.

5 participants

@maxbeckers@carsonbot@bhujagendra-ishaya@chalasr@nicolas-grekas

[8]ページ先頭

©2009-2025 Movatter.jp