-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1e2f0ae
to
fe1ddd3
Compare
Hey! I think @bhujagendra-ishaya has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
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)? |
@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: |
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 -v
PHP 8.1.12 (cli) (built: Oct 25 2022 18:20:48) (NTS Visual C++ 2019 x64)
Copyright (c) The PHP Group
Zend Engine v4.1.12, Copyright (c) Zend Technologies
... and ...
$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 22.04.1 LTS
Release: 22.04
Codename: jammy
$ php -v
PHP 8.1.12 (cli) (built: Oct 28 2022 17:39:57) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.12, Copyright (c) Zend Technologies
with Zend OPcache v8.1.12, Copyright (c), by Zend Technologies
I can confirm that in both environments, ...
- the issue happens with
symfony/console v6.1.7
, - the issue is fixed with
symfony/console v6.1.7 + fix applied
.
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.
@nicolas-grekas the |
Thanks for improving sections support, @maxbeckers. That's much appreciated. |
PR welcome on 5.4 if you find a way to have the same behavior there. |
@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. |
@maxbeckers I guess we can live with that. Thanks! |
@maxbeckers would you still have time to check the failures on branch 6.2 that came up after merging this PR up? |
@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 |
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 commit caffee8, reversing changes made to f14901e. | 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/a This 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 a fix #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#L181 A 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
return
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 the
SymfonyStyle
does not have the property$output
in this versions.