Skip to content
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

[Console] Fix bug with utf-8 bug, change writePrompt to use one function prepareChoices #33911

Open
wants to merge 6 commits into
base: 3.4
from

Conversation

@proggga
Copy link

proggga commented Oct 8, 2019

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets #33928
License MIT
Doc PR

Refactor QuestionHelper and SymfonyQuestionHelper to use one function that prepare choices question. Remove duplicate code.

@@ -30,6 +30,7 @@
*/
class QuestionHelper extends Helper
{
const DEFAULT_PROMPT = ' > ';

This comment has been minimized.

Copy link
@stof

stof Oct 8, 2019

Member

should it be a public const ?

This comment has been minimized.

Copy link
@proggga

proggga Oct 8, 2019

Author

Yea, moved to Question class

@proggga proggga force-pushed the proggga:progga/refactor_questions_helper branch from b36d10b to cf9efe9 Oct 8, 2019
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Oct 8, 2019
@proggga

This comment has been minimized.

Copy link
Author

proggga commented Oct 8, 2019

Please help, why 'TranslationFilesTest.php:42' is failing?
I didn't touch that code

@proggga

This comment has been minimized.

Copy link
Author

proggga commented Oct 15, 2019

Anyone?

@ostrolucky

This comment has been minimized.

Copy link
Contributor

ostrolucky commented Oct 15, 2019

It doesn't seem related, don't worry about it. But perhaps you could get rid of it by rebasing on top of latest 3.4?

@proggga proggga force-pushed the proggga:progga/refactor_questions_helper branch from 0e1edd9 to 913f8c3 Oct 15, 2019
@proggga

This comment has been minimized.

Copy link
Author

proggga commented Oct 15, 2019

Rebased on top 3.4

proggga added 6 commits Oct 8, 2019
@proggga proggga force-pushed the proggga:progga/refactor_questions_helper branch from 913f8c3 to b5e1596 Oct 24, 2019
@proggga

This comment has been minimized.

Copy link
Author

proggga commented Oct 24, 2019

Rebase on top 3.4,
anybody?

@proggga

This comment has been minimized.

Copy link
Author

proggga commented Oct 29, 2019

Are you serious?

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Oct 29, 2019

@proggga because of how Symfony releases new versions, October-November of each year is a very stressful moment for us. Moreover, every two years, the stress is even more intense because we release a new major version. This year is one of those years. So, we're ultra busy with millions of things and we may respond to you a bit late. Thanks for understanding! (and thanks for your contribution too!)

@proggga

This comment has been minimized.

Copy link
Author

proggga commented Oct 29, 2019

Sorry for maybe being aggressive, It was strange, very fast people came and gave alot of changes, but after it disappeared

@nicolas-grekas nicolas-grekas changed the title Refactor writePrompt to use one function prepareChoices [Console ]Refactor writePrompt to use one function prepareChoices Dec 2, 2019
@nicolas-grekas nicolas-grekas changed the title [Console ]Refactor writePrompt to use one function prepareChoices [Console] Refactor writePrompt to use one function prepareChoices Dec 2, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 2, 2019

There is a mismatch here: the PR title tells about a refacto, but the linked issue is about UTF-8 length computation. refacto go on master - fixes on 3.4. Could you please update the PR according to what it is? I didn't look better before asking :)

@proggga proggga changed the title [Console] Refactor writePrompt to use one function prepareChoices [Console] Fix bug with utf-8 writePrompt to use one function prepareChoices Dec 2, 2019
@proggga proggga changed the title [Console] Fix bug with utf-8 writePrompt to use one function prepareChoices [Console] Fix bug with utf-8 bug, change writePrompt to use one function prepareChoices Dec 2, 2019
@proggga

This comment has been minimized.

Copy link
Author

proggga commented Dec 3, 2019

Fixed

{
$choices = $question->getChoices();
$maxWidth = max(array_map([$this, 'strlen'], array_keys($choices)));

This comment has been minimized.

Copy link
@fancyweb

fancyweb Dec 4, 2019

Contributor

self::strlen?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 4, 2019

Member

[static::class, 'strlen'] (but honestly I've no issue with using $this)

This comment has been minimized.

Copy link
@fancyweb

fancyweb Dec 4, 2019

Contributor

We currently use self:: everywhere, we don't expect those methods to be overidden.

$messages = [];
foreach ($choices as $key => $value) {
$width = $maxWidth - $this->strlen($key);

This comment has been minimized.

Copy link
@fancyweb

fancyweb Dec 4, 2019

Contributor

self::strlen

foreach ($choices as $key => $value) {
$width = $maxWidth - $this->strlen($key);
// We using strlen + str_repeat to fix sprintf whitespace padding problem with UTF-8)
$messages[] = sprintf(' [<%s>%s%s</%s>] %s', $tag, $key, str_repeat(' ', $width), $tag, $value);

This comment has been minimized.

Copy link
@fancyweb

fancyweb Dec 4, 2019

Contributor

@nicolas-grekas sprintf left justify does not work with multibytes. Shouldn't we create a sprintf method in the abstract Helper class like we did for strlen, substr, etc.? Or using str_repeat for all strings is ok?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 4, 2019

Member

So, you want to implement sprintf in pure PHP.
Yes, sure... in theory :)
in practice, I don't know :)

This comment has been minimized.

Copy link
@fancyweb

fancyweb Dec 4, 2019

Contributor

I mean add a sprintf helper method to handle the left justify. If the string is not utf-8 we use the normal sprintf. If it is, we use the str_repeat strategy.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Dec 4, 2019

Member

OK, sure!

This comment has been minimized.

Copy link
@fancyweb

fancyweb Dec 4, 2019

Contributor

@proggga Would you have time to take care of this?

This comment has been minimized.

Copy link
@proggga

proggga Dec 4, 2019

Author

Hi, thanks for reviewing this,
Yea, Maybe I can, but I'm not true symphonist, so I don't know rules for creating new helper/naming/best place for it

This comment has been minimized.

Copy link
@proggga

proggga Dec 7, 2019

Author

@proggga Would you have time to take care of this?

May be you can give instructions, and after may be I can do it

This comment has been minimized.

Copy link
@fancyweb

fancyweb Dec 9, 2019

Contributor

In your proposed changes, you replaces the sprintf left justify feat by strlen + str_repeat everytime. Firstly, this alternative should only be used for multibytes strings. Secondly, it should be done in a new helper method in the abstract Helper.php class (like we did for strlen and sbustr).

}
$output->writeln($messages);
// ChoiceQuestion can have any prompt
$output->write($question->getPrompt() ?: Question::DEFAULT_PROMPT);

This comment has been minimized.

Copy link
@fancyweb

fancyweb Dec 4, 2019

Contributor

getPrompt() will always return a string. The constant is also useless. Just write the prompt.

This comment has been minimized.

Copy link
@fancyweb

fancyweb Dec 4, 2019

Contributor

That's another bug fixed by this PR btw. Unrelated to the linked issue.

// ChoiceQuestion can have any prompt
$output->write($question->getPrompt() ?: Question::DEFAULT_PROMPT);
} else {
$output->write(Question::DEFAULT_PROMPT);

This comment has been minimized.

Copy link
@fancyweb

fancyweb Dec 4, 2019

Contributor

And here keep the old behavior?

This comment has been minimized.

Copy link
@fancyweb

fancyweb Dec 4, 2019

Contributor

Maybe we should actually move the prompt from the ChoiceQuestion to the Question class? 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.