Fix PHP 8.1 "preg_match(null)" exception when cloning a repository with no URI set
ClosedPublic
Actions

Authored by aklapper on May 30 2023, 18:29.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15435: PHP 8.1 "preg_match(null)" exception cloning a repository with no URI set

Commits

rP6b8ec5014890: Fix PHP 8.1 "preg_match(null)" exception when cloning a repository with no URI…

Summary

preg_match() does not accept passing null as the $subject string parameter in PHP 8.1.

Thus add a phutil_nonempty_string() check if the $subject parameter is a non-empty string.

Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.

EXCEPTION: (RuntimeException) preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=arcpatch-D25250, ref.master=18554ea76ceb, ref.arcpatch-D25250=fade4603a799), phorge(head=diffusionEmptyBranchHistory, ref.master=e11c5486c92b, ref.diffusionEmptyBranchHistory=76f042e4b969)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<arcanist>/src/error/PhutilErrorHandler.php:261]
  #1 <#2> preg_match(string, NULL) called at [<phorge>/src/applications/diffusion/request/DiffusionGitRequest.php:6]

Closes T15435

Test Plan

After applying this change, try to "Clone" a Repository with no URI set shows the overlay dialog Clone Repository - Repository has no URIs set. as expected.

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.May 30 2023, 18:29

Owners added a reviewer: O1: Blessed Committers.May 30 2023, 18:29

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 30 2023, 18:29

aklapper requested review of this revision.May 30 2023, 18:29

Harbormaster completed remote builds in B492: Diff 857.May 30 2023, 18:29

Thanks for this short-circuit. +1

Since this is just a private method, probably we can just rely on $symbol === null. I will add another +1 in that case since it's less risky.

(Believe me or not but my tips are not picked using a dice ihih)

use a simple $symbol === null check as requested

Harbormaster completed remote builds in B493: Diff 858.May 31 2023, 06:01

Thanks for your change

NOTE: I'm aware that preg_match() returns 0 on no-match and 1 on match and false on failure, that is why here we return false on failure, since that was the legacy behavior and it has sense. Also, obviously both zero and false are falsy, so this seems the minimal change here, with 100% backward compatibility.

It seems the only usage is here:

./src/applications/diffusion/request/DiffusionRequest.php-  public function getStableCommit() {
./src/applications/diffusion/request/DiffusionRequest.php-    if (!$this->stableCommit) {
./src/applications/diffusion/request/DiffusionRequest.php:      if ($this->isStableCommit($this->symbolicCommit)) {
./src/applications/diffusion/request/DiffusionRequest.php-        $this->stableCommit = $this->symbolicCommit;
./src/applications/diffusion/request/DiffusionRequest.php-        $this->symbolicType = 'commit';
./src/applications/diffusion/request/DiffusionRequest.php-      } else {
./src/applications/diffusion/request/DiffusionRequest.php-        $this->queryStableCommit();
./src/applications/diffusion/request/DiffusionRequest.php-      }
./src/applications/diffusion/request/DiffusionRequest.php-    }
./src/applications/diffusion/request/DiffusionRequest.php-    return $this->stableCommit;
./src/applications/diffusion/request/DiffusionRequest.php-  }
./src/applications/diffusion/request/DiffusionRequest.php-

So, it's just a simple if() check that checks against truly and falsy.

Thanks again

sgtm