Fall back to from utf8_decode to phutil_utf8v for codepoint counting
ClosedPublic
Actions

Authored by alexmv on Jan 12 2017, 3:02 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Commits

rPHU6cd38ce75fea: Fall back to from utf8_decode to phutil_utf8v for codepoint counting

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

alexmv created this revision.Jan 12 2017, 3:02 PM

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJan 12 2017, 3:02 PM

Herald added a subscriber: epriestley. · View Herald Transcript

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?