Skip to content

[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

Merged
merged 1 commit into from
Dec 14, 2022

Conversation

maxbeckers
Copy link
Contributor

Q A
Branch? 6.1
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #47411
License MIT
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 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.

@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
Contributor Author

@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-ishaya bhujagendra-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 -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.

Copy link
Member

@nicolas-grekas 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.

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

@maxbeckers
Copy link
Contributor Author

@nicolas-grekas the ConsoleSectionOutput 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

chalasr commented Dec 14, 2022

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

@chalasr chalasr merged commit caffee8 into symfony:6.1 Dec 14, 2022
@chalasr
Copy link
Member

chalasr commented Dec 14, 2022

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

@maxbeckers
Copy link
Contributor Author

@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

chalasr commented Dec 14, 2022

@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
Contributor Author

maxbeckers commented Dec 14, 2022

@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 foo 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

chalasr commented Dec 14, 2022

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

chalasr added a commit to chalasr/symfony that referenced this pull request Dec 16, 2022
…ection (maxbeckers)"

This reverts commit caffee8, reversing
changes made to f14901e.
nicolas-grekas added a commit that referenced this pull request Dec 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 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)"
nicolas-grekas added a commit that referenced this pull request Dec 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 request Dec 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
@fabpot fabpot mentioned this pull request Dec 16, 2022
@fabpot fabpot mentioned this pull request Dec 28, 2022
chalasr added a commit that referenced this pull request Feb 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/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:
![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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants