Page MenuHomePhabricator

PHP 8 Compatibility
Open, LowPublic

Description

PHP 8 was released on November 26, 2020 and Arcanist and Phabricator have at least a few compatibility issues, although the changes in PHP 8 and the required adjustments seem likely to be minor.

At time of writing, customers haven't begun upgrading to PHP 8 yet, so fixing these issues isn't a priority. It took about a year before customers began deploying PHP 7 and only a few hours to fix the bulk of the issues; the process for PHP 8 may be similar.

Previously, see T12101 for PHP 7 compatibility.

Event Timeline

epriestley triaged this task as Low priority.EditedJan 10 2021, 10:23 PM
epriestley created this task.

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

  • A new linter should be introduced to detect this so new instances are not introduced. Similar linters currently exist for abstract final and final in a final class, but private final slipped through.
  • After the linter is available, a similar change should be applied to the Phabricator repository (D21496 covers Arcanist).

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).

So I don't forget:

  • From D21501, a linter rule for blocks that catch Exception but do not catch Throwable is likely desirable.
  • From D21500, xsprintf() has an unusual $callback(...) invocation, where call_user_func[_array]() is usually used. This does not work with C::m (a static method callback specified as a string) in older PHP. It does work with array('C', 'm') since PHP 5.4. Some kind of pointer to the explanation (that this is to simplify reference parameter behavior) would be nice.

A new linter should be introduced to detect [private final]...

Resolved in D21539.

...a similar change should be applied to [remove private final from] the Phabricator repository...

Resolved in D21540.

From D21500, xsprintf() has an unusual $callback(...) invocation...

Resolved in D21541.

From D21501, a linter rule for blocks that catch Exception...

Resolved in D21542.

I haven't attempted to identify/fix any exception-without-throwable blocks, but they should converge toward extinction now.

I'm going to leave this task open since I haven't actually run Phabricator under PHP8 yet and there may well be other issues, but these changes likely resolve all current known issues.

See https://discourse.phabricator-community.org/t/daemon-fails-on-php-8-0-2-in-utils-php-array-merge-call-w-fix/4568.

PHP8 supports named arguments. At a minimum, the builtin function call_user_func_array() has changed behavior. This script:

<?php

function f($a, $b) {
    if ($a > $b) {
        echo 'A';
    } else {
        echo 'B';
    }
}

call_user_func_array('f', array('b' => 1, 'a' => 2));

...emits "A" in PHP8 and newer and "B" in older versions of PHP:

https://3v4l.org/GV4v5

I'm not thrilled about the implementation of this change. Although I don't have full context, I would likely have implemented a new call_user_func_map() or similar for this new behavior, and kept the semantics of call_user_func_array() unchanged. I think the behavior of this script (the relative ease with which you can get dramatically different behavior) is a bit troubling.

That said, a saving grace is that PHP8 emits a fatal error if you pass an unknown parameter (in the case of the builtin array_merge, at least, you get a different generic fatal than you do with a PHP function, so it's possible this behavior isn't universal across builtin functions). Although we likely have at least some code which can reach call_user_func_array() with a list of parameters that have arbitrary keys, it's exceedingly unlikely that any of those keys will be valid parameter names, and this should virtually always fatal safely rather than misbehaving. This makes the implementation choice much more palatable.

In the specific case of array_mergv(...), I'm likely to just add a call to array_values().

We have about 30 calls to call_user_func_array() in phabricator/. Most of these seem obviously correct (from trivial inspection, they can never reach call_user_func_array() with anything but a "natural" list, most often because they call $args = func_get_args(); to build the list). The others seem very difficult to assess.

There are also about 30 calls in arcanist/. The overwhelming majority (all but one or two) seem obviously correct; the others seem very difficult to assess.

I also don't see any way to really make useful assertions about this parameter statically.

We could statically identify most callsites as certainly safe, when the parameter is:

  • set to a natural array literal or the result of an expression which is known to produce only natural array literals, like func_get_args(); and
  • unmodified, or modified only with natural array appends ($args[] = ...); and
  • then passed to call_user_func_array(...).

...it is certainly safe. However, this leaves a bunch of callsites that we couldn't really raise any useful message about: it would almost always be a false positive ("this is probably correct as written, but it might be an error") and I think this kind of lint message is generally not worth implementing or maintaining.