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.

Ah, great. Making this variation of the change will probably never actually matter compared to the first version, but I'll sleep at least a little better at night.

In D21497#272834, @epriestley wrote:

What's the specific issue you run into with PHP8 with the existing code -- the function emits a deprecation warning when called?

Since the attack surface on the XML entity loader is huge/scary (historically, see D8049), I'd prefer to continue explicitly disabling it if we can. Although there's probably no real risk, I can imagine scenarios like some distro shipping a version of PHP which enables it by default, or users enabling it by default because they run some other software that needs it alongside Phabricator. In these cases, we're a little better off at runtime if we continue disabling the loader.

If this just raises a deprecation warning under PHP8, I'd be more comfortable "fixing" it by suppressing the warning with @ (so @libxml_disable_entity_loader(true); -- not sure how familiar you are with PHP) so that we're still making sure the loader is disabled. Is that reasonable, or does it create more problems?

Suppress deprecation warning with @ rather than skip the function call.

It's possible that third party code which installs custom error listeners via setErrorListener() that expect 'context' to be populated may exist, I'm not aware of any such code and think it probably does not exist. It would also be easy to update any such code in response to this change, so I think it's not worth retaining backward compatibility with a dummy parameter and that this is the most-desirable version of this change.

What's the specific issue you run into with PHP8 with the existing code -- the function emits a deprecation warning when called?

I added you to Blessed Committers, so you should be able to arc land this yourself. See the description of that project for more detailed instructions, or let me know if you run into issues. "Land Revision" here in the web UI should also work, or I can just pull it for you if that's easier.

Thanks! There are some similar linters already (e.g., for final in a final class, or final on an abstract method) but this particular combination (private + final) slipped through. I'll add a linter so these don't get re-introduced and apply an equivalent change to Phabricator.

Just want to add a note that with git lfs managed files, arc patch won't work unless one fetch the ref tag and then the lfs blobs from the staging server first. See https://discourse.phabricator-community.org/t/arc-patch-fails-with-git-lfs-files/2447/3

As of macOS X 11 (Big Sur), php on the CLI emits:

That seems reasonable. I also ran into a couple of other suggestions elsewhere:

Off the wall suggestion: how about a trigger that sets the subtype: