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

Projects
None yet
4 participants
@jls-esokia
Copy link
Contributor

jls-esokia commented Feb 23, 2019

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

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 23, 2019

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 jls-esokia force-pushed the jls-esokia:3.4 branch from 136b72f to 0d31b5d Feb 23, 2019

@jls-esokia

This comment has been minimized.

Copy link
Contributor Author

jls-esokia commented Feb 23, 2019

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

@jls-esokia jls-esokia force-pushed the jls-esokia:3.4 branch from 0d31b5d to 8ce1314 Feb 23, 2019

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Feb 23, 2019

@jls-esokia jls-esokia force-pushed the jls-esokia:3.4 branch from 8ce1314 to 47320a6 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));

This comment has been minimized.

Copy link
@OskarStark

OskarStark Feb 23, 2019

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 23, 2019

Member

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

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 23, 2019

Thank you @jls-esokia.

@nicolas-grekas nicolas-grekas merged commit 47320a6 into symfony:3.4 Feb 23, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Feb 23, 2019

bug #30354 [Console] handles multi-byte characters in autocomplete (j…
…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
You can’t perform that action at this time.