Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedNov 4, 2022
Hey! I think@bhujagendra-ishaya has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
bhujagendra-ishaya commentedNov 4, 2022
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 commentedNov 4, 2022
@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: Just run on CLI: |
bhujagendra-ishaya left a comment
There was a problem hiding this 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 ...
- with your provided vendor dir
- 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 TechnologiesI can confirm thatin both environments, ...
- the issuehappens with
symfony/console v6.1.7, - the issueis fixed with
symfony/console v6.1.7 + fix applied.
nicolas-grekas left a comment
There was a problem hiding this 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 commentedDec 14, 2022
@nicolas-grekas the |
chalasr commentedDec 14, 2022
Thanks for improving sections support,@maxbeckers. That's much appreciated. |
chalasr commentedDec 14, 2022
PR welcome on 5.4 if you find a way to have the same behavior there. |
maxbeckers commentedDec 14, 2022
@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 commentedDec 14, 2022
@maxbeckers I guess we can live with that. Thanks! |
nicolas-grekas commentedDec 14, 2022
@maxbeckers would you still have time to check the failures on branch 6.2 that came up after merging this PR up? |
maxbeckers commentedDec 14, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 there |
chalasr commentedDec 14, 2022
Thanks for the hint. I'll have a look so we can decide depending on the result. Enjoy your vacation :) |
…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)"
* 6.1: [HttpKernel] fix merge [HttpKernel] fix merge Revert "bug#48089 [Console] Fix clear line with question in section (maxbeckers)"
* 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
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:Output loop 1: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

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 the
returnso 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 the
SymfonyStyledoes not have the property$outputin this versions.