• Include fix for a "blame" callsite.

I'm not completely thrilled about maintaining PHP builtin webserver support...

In T13575#254280, @epriestley wrote:

I added M1, etc., to the ignored list in D21507.

I think this is now resolved. I'm not completely thrilled about maintaining PHP builtin webserver support because I think use is very limited, but since I'm currently using it I expect to support it at least until I summon the nerve to deal with Homebrew.

See T13575. This change should be obsoleted by D21506.

Guess I should probably add M1 to the remarkup blocklist now.

I switched to an M1 Mac Mini on Big Sur, which has motivated me somewhat to try to get this working since I suspect doing another install through Homebrew on M1 silicon will be more adventure than I have stomach for.

Rebased patch series

Rebased patch series

Rebased patch series

Catch Throwable in the test rather than converting to Exception inside phutil_utf8_convert

See D21500 for discussion, the test failure was an interaction between callbacks specified in C::m form and $callback() inside xsprintf() under old versions of PHP. Resolved by D21505.

So I don't forget:

Ah even better.

Oh, or the array('C', 'm') version appears to work properly when invoked as $callback in all versions of PHP since PHP 5.4:

https://3v4l.org/eYFoc does work for 5.2.2+ to pass by reference, provided which arguments are passed by reference is part of the contract. But probably best to keep it simple and define xsprintf_test_callback as you say.

Oh, I think the reason to use $callback(...) is the behavior of the reference parameters. Yikes.

Here's some evidence to support that theory:

Ah, I think the issue is that xsprintf() internally does this :

Stay under 80 characters (and format the other long preg_match a bit more nicely)

See T10038 for general context.

Hm, Harbormaster is failing with:

Fatal error: Call to undefined function ArcanistFormattedStringXHPASTLinterRule::processXsprintfCallback() in /core/data/drydock/workingcopy-70/repo/arcanist/src/xsprintf/xsprintf.php on line 70

Does referencing static functions not work in old PHP versions or something? I tested with 7.4 and it was fine.

Oh, I'm totally onboard with this change -- I think this behavior is generally better than the old behavior and the property of getting the input string out is useful/clever, just trying to save future-me a minute or two if this breaks and I end up here via git blame.

Sounds good. I think it's very unlikely anything is relying on the type of exception thrown, and we would (or, at least, should) normally throw a narrower exception (UTF8ConversionEncodingFailedVeryNarrowlyException) if really trying to make this part of the API.

🤷

preg_match() in all modern versions of PHP seems to be willing to accept an object with __toString() as the first parameter:

In D21500#272930, @epriestley wrote:

In the case of hypothetical zsprintf("A %XYZ B", ...), where %XYZ is some multi-character conversion like %Ls, the real zsprintf() would call sprintf("A %s B") internally, while this will call sprintf("A %sYZ B") -- that is, this callback can't know that %XYZ is a single conversion, rather than %X + YZ.

I can't think of any problems this will cause today or any theoretical problems it will cause in the future, and there's no easy way to future-proof it anyway, so I think this is the most reasonable fix.

In the case of hypothetical zsprintf("A %XYZ B", ...), where %XYZ is some multi-character conversion like %Ls, the real zsprintf() would call sprintf("A %s B") internally, while this will call sprintf("A %sYZ B") -- that is, this callback can't know that %XYZ is a single conversion, rather than %X + YZ.

In D21501#272924, @epriestley wrote:

Does this just break a test or something?

Does this just break a test or something?

Thanks!

With the four revisions I've just added, arc lint works with PHP 8 when run inside the arcanist repo, and arc unit --everything has no regressions compared with PHP 7.4 (both do have a few failures but they're the same and relate to pyflakes/jshint/hg, and look environment-specific so nothing to do with PHP 8).

Trivially rebased for landing

Feel free to test against secure within reason if that's easier. These patches might not seem like a big deal, but stuff like this which just makes my life easier is incredibly rare, so make yourself at home.

In trying to arc diff --update this I got an exception (but it worked with --head, which I had been using to submit each commit of the patch series). What's the best way to test out arc itself and reproduce that? Use admin.phacility.com to get a testing instance?

In D21496, declaring a method private final (which is redundant, as a private method may never be overridden) causes an issue in PHP8.