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
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
Unknown Object (File)
Sun, Nov 10, 9:58 AM
Unknown Object (File)
Sun, Nov 10, 8:58 AM
Unknown Object (File)
Sun, Nov 10, 7:05 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
Branch
up-utf8decode
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 15224
Build 20012: Run Core Tests
Build 20011: arc lint + arc unit

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.