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.

From https://discourse.phabricator-community.org/t/sending-emails-causes-an-exception/4966, looks like the PHPMailer uses each which is removed in php 8.

See also T12404#256288 for a note on each removal.

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

In T13588#256950, @cspeckmim wrote:

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

Thanks, this should be fixed by D21825.

Thank you!

In T13588#256584, @epriestley wrote:

PHP 8.1 disallows strlen(null).
...
Broadly, Phabricator likely will not pass the empty string as a constraint in these cases...
...
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).
...
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.

Are you opposed to any automated changes for updating strlen() to phutil_nonempty_string() when used in if-expressions and not utilizing the numeric return value? I've been toying with semgrep to see how it could fare with ~semantically~ finding/updating these specific uses of strlen (edited for brevity):

pattern: if (strlen($X))
fix: if (phutil_nonempty_string($X))

pattern: if (!strlen($X))
fix: if (!phutil_nonempty_string($X))

pattern: if ($X && strlen($X))
fix: if (phutil_nonempty_string($X))

pattern: if (!$X || !strlen($X))
fix: if (!phutil_nonempty_string($X))

pattern: if ($A || strlen($X))
fix: if ($A || phutil_nonempty_string($X))

pattern: if ($A && strlen($X))
fix: if ($A && phutil_nonempty_string($X))

pattern: if ($A || !strlen($X))
fix: if ($A || !phutil_nonempty_string($X))

However from your comment above I wasn't sure if you believe it better to not do a blanket update like this, as an indicator in the code that empty string could not make it to that execution path. We recently updated from PHP 7 to 8 on our instance and haven't run into strlen(null) issues yet but most day-to-day only touches a small functionality of Phabricator. In anticipation of us running into one I'm wondering if addressing the occurrences as the crop up is preferred over a blanket update.

I favor dealing with them on a case-by-case basis since they don't seem especially pervasive and I think in most of the cases where I've fixed the issue, the fix I chose wasn't just to swap the call. I think these strlen() errors are often a correct/useful symptom of undesirable slop in type handling.

(It may or may not be useful, but PHPAST can semantically rewrite PHP -- the easiest way is to write a linter that raises a message and replacement, then arc lint --all the codebase. See also here.)

Gotcha. I'm going to take a swing at updating past 8.0 and see what crops up. Also thanks for the tip with PHPAST. I haven't looked too much into it other than trying to get it working on Windows a year or so back

As an example, given this code

private $name;

function setName($name) {
  $this->name = name;
}

function getName() {
  return $this->name;
}

public function doThing() {
  $name = $this->getName();
  if (strlen($name)) {
    ...
  }
}

Would the preferred update be this?

function setName(string $name) {
  $this->name = $name;
}

function getName() {
  return (string) $this->name;
}

As it indicates/enforces the expected type, and if $name is null then it ends up returning the empty string?

I think the string typehint isn't supported until recent-ish PHP, so its availability will depend on your minimum supported version.

I'm also not sure what the behavior of the string typehint is for "stringlike objects", i.e. objects which implement __toString(), so the string typehint might not be appropriate if you want to accept them (or don't want to accept them). Phabricator makes use of stringlike objects for URIs and HTML, so the suitability of the string typehint may differ for SomeUIElement->setName() (stringlikes probably expected) vs SomeInternalModule->setName() (stringlikes probably unexpected).

If $name should only ever have a string value and null is not a semantically different value from empty string, I tend to favor private $name = ''; instead of return (string)$this->name;, so that the property may never have a non-string value. Combined with a string typehint (or a similar sort of manual check + throw), this should get the same behavior as the version that uses a cast, but without introducing the inconsistency that $obj->name and $obj->getName() may sometimes subtly differ. This is also perhaps a clearer hint to readers that the property does not treat null as any sort of flag value distinct from empty string.

Also note that when you (string) an object that has a __toString() method, and that method throws an exception, PHP fatals immediately until PHP 7.1, so consider using phutil_string_cast() instead of (string) if you target versions before 7.1. It has essentially the same behavior as (string) but with significantly more reasonable error handling.