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] handles multi-byte characters in autocomplete #30354

Merged
merged 1 commit into from Feb 23, 2019

Conversation

jls-esokia
Copy link
Contributor

fixes #29966

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #29966
License MIT
Doc PR -

I used the mb_ord to detect whether the amount of bytes read is valid before proceeding. I limit the number of bytes read to 4 before giving up because characters can use at most 4 bytes.
The test passes with or without the fix though.

@nicolas-grekas
Copy link
Member

Using mb_ord() requires PHP 7.2, but the lowest supported version for branch 3.4 is PHP 5.5.
We could use the 7.2 polyfill to make this work, but I don't think the polyfill implements the case where it should return false on invalid characters.
That'd be great if someone could have a look at the polyfill to fix it.

For this PR, we could use that map:
$lengthMask = array("\xC0" => 2, "\xD0" => 2, "\xE0" => 3, "\xF0" => 4);
Then, for a byte $b, knowing the length of the utf8 character it starts is done with:
$length = $b < "\x80" ? 1 : $lengthMask[$b & "\xF0"];

@jls-esokia
Copy link
Contributor Author

My bad, I saw mb_ord was listed in the polyfill-mbstring library which is a dependency of the console component. Ok made the necessary correction.

@nicolas-grekas nicolas-grekas changed the title handles multi-byte characters in autocomplete [Console] handles multi-byte characters in autocomplete Feb 23, 2019
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Feb 23, 2019
$question = new ChoiceQuestion('Please select a character', $possibleChoices);
$question->setMaxAttempts(1);

$this->assertSame($character, $dialog->ask($this->createStreamableInputInterfaceMock($inputStream), $this->createOutputInterface(), $question));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$this->assertSame($character, $dialog->ask($this->createStreamableInputInterfaceMock($inputStream), $this->createOutputInterface(), $question));
$this->assertSame(
$character,
$dialog->ask($this->createStreamableInputInterfaceMock($inputStream), $this->createOutputInterface(), $question)
);

Copy link
Member

Choose a reason for hiding this comment

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

we usually prefer long lines so not sure about that :)

@nicolas-grekas
Copy link
Member

Thank you @jls-esokia.

@nicolas-grekas nicolas-grekas merged commit 47320a6 into symfony:3.4 Feb 23, 2019
nicolas-grekas added a commit that referenced this pull request Feb 23, 2019
…ls-esokia)

This PR was merged into the 3.4 branch.

Discussion
----------

[Console] handles multi-byte characters in autocomplete

fixes #29966

| Q             | A
| ------------- | ---
| Branch?       | 3.4 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #29966   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | - <!-- required for new features -->

I used the `mb_ord` to detect whether the amount of bytes read is valid before proceeding.  I limit the number of bytes read to 4 before giving up because characters can use at most 4 bytes.
The test passes with or without the fix though.

Commits
-------

47320a6 handles multi-byte characters in autocomplete
This was referenced Mar 3, 2019
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.

None yet

4 participants