Page MenuHomePhabricator

Fall back to from utf8_decode to phutil_utf8v for codepoint counting
ClosedPublic

Authored by alexmv on Jan 12 2017, 3:02 PM.
Tags
None
Referenced Files
F14172029: D17188.diff
Sun, Dec 8, 2:52 AM
Unknown Object (File)
Thu, Dec 5, 1:42 AM
Unknown Object (File)
Thu, Nov 28, 5:43 PM
Unknown Object (File)
Fri, Nov 22, 11:41 AM
Unknown Object (File)
Fri, Nov 22, 10:21 AM
Unknown Object (File)
Thu, Nov 21, 5:37 PM
Unknown Object (File)
Sun, Nov 17, 5:32 PM
Unknown Object (File)
Thu, Nov 14, 12:48 AM
Subscribers

Details

Summary

The php7.0 package in Ubuntu 16.04 splits out the php7.0-xml package, which
provides the utf8_decode function. As such, parts of Arcanist throw
exceptions -- most notably PhutilArgumentSpellingCorrector:

$ arc dieff
[2017-01-12 14:57:25] EXCEPTION: (Error) Call to undefined function utf8_decode() at [<phutil>/src/utils/utf8.php:259]
arcanist(head=master, ref.master=d6e112aecf93), phutil(head=master, ref.master=ab80dcf99c6b)
  #0 phutil_utf8_strlen(string) called at [<phutil>/src/parser/argument/PhutilArgumentSpellingCorrector.php:120]
  #1 PhutilArgumentSpellingCorrector::correctSpelling(string, array) called at [<arcanist>/src/configuration/ArcanistConfiguration.php:148]
  #2 ArcanistConfiguration::selectWorkflow(string, array, ArcanistConfigurationManager, PhutilConsole) called at [<arcanist>/scripts/arcanist.php:193]

Since spelling correction is a core feature, and working out-of-the-box is
desirable, fall back to implementing phutil_utf8_strlen in terms of
phutil_utf8v if decode_utf8 is not present.

Fixes: T11713.

Test Plan

Ran arc dieff on an Ubuntu 16.04 machine after `apt-get
install php7.0-cli`, as well as on PHP 5.5.

Diff Detail

Repository
rPHU libphutil
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Just remove the line 599 assertEqual() on the error message to make the test suite pass? You could replace the values of $invalid_cases with a human-readable description, or remove them entirely. I don't think we should be testing exact error message text, as this is something we can reasonably expect to be fairly fragile across versions.

This revision now requires changes to proceed.Jan 12 2017, 3:09 PM

I think that's unrelated to this change, and purely based on the yacc version in the environment. Mind if I split that out into a separate diff?

This revision is now accepted and ready to land.Jan 12 2017, 3:13 PM

Rebase and re-run unit tests.

This revision was automatically updated to reflect the committed changes.