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.

Revisions and Commits

rARC Arcanist
Needs Review
Closed
Closed
D21825
D21825
D21822
D21798
D21762
D21758
D21743
D21742
D21741
D21740
D21551
D21542
D21542
D21541
D21539
D21502
D21501
AuditedD21500
D21499
D21498
rP Phabricator
D21852
D21852
D21821
D21795
D21775
D21761
D21757
D21752
D21744
D21701
D21549
D21540

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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.

See also T13232. Here's a possible build strategy for PHP 8.1 under macOS Mojave. Anyone reasonable should probably give up and use brew instead.

  • In theory, php can build without pkg-config by adjusting environmental variables, but I couldn't figure it out.
  • Download pkg-config and build it with:
pkg-config-0.29.2/ $ ./configure --with-internal-glib
pkg-config-0.29.2/ $ make
  • Put it in bin/ or whatever.
  • Download (and build?) zlib (?). I have no idea what pkg-config does, e.g. PKG_CONFIG_PATH=/custom/path/to/zlib pkg-config --any-flag -- zlib seems to just emit /usr/local/include no matter what and none of the build output suggests it passed the custom path to the compiler or linker.
  • Get PHP working by disabling a bunch of stuff:
#!/bin/sh

set -e
set -x

export PKG_CONFIG_PATH="/Users/epriestley/src/zlib-1.2.11"

./configure \
  --with-mysqli \
  --enable-pcntl \
  --enable-mbstring \
  --without-iconv \
  --disable-mbregex \
  --disable-xml \
  --without-libxml \
  --without-sqlite3 \
  --disable-dom \
  --disable-pdo \
  --disable-simplexml \
  --disable-xmlreader \
  --disable-xmlwriter

This fatals:

PEAR package PHP_Archive not installed: generated phar will require PHP's phar extension be enabled.

Fatal error: Uncaught InvalidArgumentException: RegexIterator::__construct(): Allocation of JIT memory failed, PCRE JIT will be disabled. This is likely caused by security restrictions. Either grant PHP permission to allocate executable memory, or set pcre.jit=0 in /Users/epriestley/src/php-8.1.0/ext/phar/phar.php:1145
Stack trace:
#0 /Users/epriestley/src/php-8.1.0/ext/phar/phar.php(1145): RegexIterator->__construct(Object(RecursiveIteratorIterator), '/\\.svn/')
#1 /Users/epriestley/src/php-8.1.0/ext/phar/phar.php(1089): PharCommand::phar_add(Object(Phar), 0, '/Users/epriestl...', NULL, '/\\.svn/', Object(SplFileInfo), NULL, false)
#2 /Users/epriestley/src/php-8.1.0/ext/phar/phar.php(225): PharCommand->cli_cmd_run_pack(Array)
#3 /Users/epriestley/src/php-8.1.0/ext/phar/phar.php(2101): CLICommand->__construct(19, Array)
#4 {main}
  thrown in /Users/epriestley/src/php-8.1.0/ext/phar/phar.php on line 1145
make: *** [ext/phar/phar.phar] Error 255

...but emits a php binary first.


  • PHP 8.1 requires classes which implement Iterator to have all Iterator methods declared with appropriate return types or annotated with #[\ReturnTypeWillChange]. We can't declare return types while remaining backward-compatible, so the most practical fix is to annotate.
Fatal error: Uncaught Exception: Error while loading file "/Users/epriestley/dev/core/lib/arcanist/src/object/Phobject.php": Return type of Phobject::rewind() should either be compatible with Iterator::rewind(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /Users/epriestley/dev/core/lib/arcanist/src/init/lib/PhutilBootloader.php:275
  • The function strftime() is deprecated. There's one call in error handling that can be trivially replaced with date().
epriestley@orbital ~/src/php-8.1.0 $ ./sapi/cli/php -r "echo strftime('%Y-%m-%d %H:%M:%S');" 

Deprecated: Function strftime() is deprecated in Command line code on line 1
2021-12-09 20:09:55
epriestley@orbital ~/src/php-8.1.0 $ ./sapi/cli/php -r "echo date('Y-m-d H:i:s');"          
2021-12-09 20:09:58
  • Phabricator has several strftime() calls in Releeph. These should normally be unreachable, and Releeph should just be deleted anyway.
  • Passing null to strlen() is no longer supported. In many cases, a noncompliant strlen($var) check can be more correctly replaced with a $var !== null check. In other cases, some more surgical replacement is more appropriate.
  • Passing null to preg_replace() as a subject is no longer supported. This is strictly incorrect anyway.

  • You can build libcurl from source with ./configure --without-ssl.

PHP 8.1 changed semantics of static variables in methods. See:

https://wiki.php.net/rfc/static_variable_inheritance

See also T13637.

Phabricator uses Reflection to compile lists of class properties in several cases. The recommended replacement is static $class_property_map; $class_property_map[static::class] = .... In most relevant cases, static::class should be equivalent to get_class($this), but requires PHP 5.5 (see https://3v4l.org/7iHID). This is fine because PHP 5.5 is already the minimum version thanks to the move to yield.

Another possibility is to do all this work at library map time, although I'd like compelling evidence that this is motivated by a performance argument before pursuing it. Opcache preloading might reveal such an argument.

PHP 8.1 disallows strlen(null).

In many cases (like searching for macros containing the substring "dog"), Phabricator currently uses a test like this:

if (strlen($this->nameContainsSubstringConstraint)) {
  // ...
}

This omits the constraint if the value is null (the default), or if it is '' (the empty string).

Broadly, Phabricator likely will not pass the empty string as a constraint in these cases, and it would almost certainly be reasonable to treat the empty string as a nonempty value (and either add it to the query conditions, or, most correctly, emit an error).

However, a lot of code like this is reachable from the API or from the web UI, and third party callers may possibly get an empty string in here by passing JSON (this may be difficult for modern API methods, but is likely not difficult for older methods).

In a perfect world, we'd likely prefer this replacement:

if ($this->constraint !== null) {
  if (!strlen($this->constraint)) {
    throw new Exception('Empty string is not a valid constraint, use null to omit the constraint.');
  }
  // ...
}

...but this is a mild compatibility break and a pain to test. This replacement is faithful to compatibility:

if ($this->constraint !== null && strlen($this->constraint)) {
  // ...
}

..but cumbersome and difficult to revisit.

D21762 introduces phutil_nonempty_string(), which is like this test but slightly stricter (it raises an exception for wonky values) and easier to revisit later (i.e., use of this function is a hint that the code can be made more strict about value types).

The companions phutil_nonempty_stringlike() (which also accepts objects with __toString()) and phutil_nonempty_scalar() (which accepts all stringlikes, plus integers and floats) are provided for completeness, but these functions are both increasingly strong hints about a lack of type strictness.

Upshot:

  • if (strlen($maybe_null)) may be replaced in all cases with if (phutil_nonempty_string($maybe_null)) to work in PHP 8.1 and not break any code which wasn't already extremely suspicious (...stringlike() may be required if the value may be an object, usually a URI object).
  • This is usually not the most desirable replacement, but probably 95% of these aren't realistically worth being surgical about.
  • In new code, prefer increasing type strictness over these more-flexible tests.

Trying to fresh install using PHP 7.4 or PHP 8.1, have the same result when executing:

  • ./bin/config set phd.user root
[2022-05-10 02:01:12] EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=fc5b228db537), phabricator(head=master, ref.master=698ada2470b1)
  #0 PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phabricator>/src/infrastructure/env/PhabricatorEnv.php:128]
  #1 PhabricatorEnv::initializeCommonEnvironment(boolean) called at [<phabricator>/src/infrastructure/env/PhabricatorEnv.php:75]
  #2 PhabricatorEnv::initializeScriptEnvironment(boolean) called at [<phabricator>/scripts/init/lib.php:26]
  #3 init_phabricator_script(array) called at [<phabricator>/scripts/init/init-setup.php:11]
  #4 require_once(string) called at [<phabricator>/scripts/setup/manage_config.php:5]
  • ./bin/storage upgrade --force
[2022-05-10 02:51:50] EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=stable, ref.stable=e1db75547897), phabricator(head=stable, ref.stable=8659a50383bb)
  #0 PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phabricator>/src/infrastructure/env/PhabricatorEnv.php:128]
  #1 PhabricatorEnv::initializeCommonEnvironment(boolean) called at [<phabricator>/src/infrastructure/env/PhabricatorEnv.php:75]
  #2 PhabricatorEnv::initializeScriptEnvironment(boolean) called at [<phabricator>/scripts/init/lib.php:26]
  #3 init_phabricator_script(array) called at [<phabricator>/scripts/init/init-setup.php:11]
  #4 require_once(string) called at [<phabricator>/scripts/sql/manage_storage.php:5]

I am pretty sure that this error message does not exist prior to PHP 8.1, and PHP 7.4 can not possibly emit it. See here for some evidence that this is true -- note that the error message is not present in the script output until PHP 8.1:

https://3v4l.org/0s1Gs

I think you may have installed PHP 7.4 incorrectly, or uninstalled PHP 8.1 incorrectly, or not completely restarted services, or have some other environmental problem. If you believe I'm mistaken, please provide reproduction steps for emitting this error from PHP 7.4 (for example, you could modify the 3v4l.org snippet so that PHP 7.4 emits this error).

Phabricator does not support PHP 8.1 yet, so I expect that you may encounter errors (particularly, this error) under PHP 8.1. I have been fixing these as I come across them, but ran into some as recently as a week ago and very much doubt I have fixed all of them. I am running only the CLI under PHP 8.1 locally, not the web interface, so it may trivially fail to run under PHP 8.1.

I expect Phabricator should largely work correctly under PHP 7.4.

Please do not use secure.phabricator.com to ask for setup help: this is not an appropriate forum for this kind of question. Because Phabricator is no longer actively maintained, there is no longer any way to ask for setup help from the upstream, and the upstream no longer maintains a community forum. If you or anyone else continues to ask for setup help here, I'll disable your account -- so if you choose to report bugs here, be absolutely certain you're reporting a real bug and not an environmental problem if you don't want to risk your rare collector's edition secure.phabricator.com account.

A user just ran into this exception with Arcanist (current master changeset) while running arc diff (using Mercurial) with PHP 8.1. I haven't looked too in depth but it seems similar to other PHP 8.1 issues.

[2022-05-17 20:49:47] EXCEPTION: (RuntimeException) substr(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=3cc486d5c156)
  #0 PhutilErrorHandler::handleError(integer, string, string, integer)
  #1 substr(NULL, integer, integer) called at [<arcanist>/src/repository/api/ArcanistRepositoryAPI.php:807]
  #2 ArcanistRepositoryAPI::getDisplayHash(NULL) called at [<arcanist>/src/repository/marker/ArcanistRepositoryMarkerQuery.php:67]
  #3 ArcanistRepositoryMarkerQuery::execute() called at [<arcanist>/src/repository/query/ArcanistRepositoryQuery.php:20]
  #4 ArcanistRepositoryQuery::executeOne() called at [<arcanist>/src/repository/api/ArcanistMercurialAPI.php:988]
  #5 ArcanistMercurialAPI::getActiveBookmark() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:2253]
  #6 ArcanistDiffWorkflow::buildDiffSpecification() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:380]
  #7 ArcanistDiffWorkflow::run() called at [<arcanist>/scripts/arcanist.php:427]
<<< [1] (+8,567) <exec> 8,567,426 us

A user just ran into this exception with Arcanist...

Thanks, this should be fixed by D21825.