Page MenuHomePhorge

Fix PHP 8.1 "strlen(null)" exceptions trying to View Branches of a Diffusion repository
ClosedPublic

Authored by aklapper on May 23 2023, 21:45.

Details

Summary

strlen() was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts phutil_nonempty_string() as a replacement.

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) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=e4fd31ec024e), phorge(head=D25241, ref.master=b1edfea09bad, ref.D25241=b1edfea09bad)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/diffusion/controller/DiffusionBranchTableController.php:29]
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=e4fd31ec024e), phorge(head=D25241, ref.master=b1edfea09bad, ref.D25241=b1edfea09bad)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/diffusion/conduit/DiffusionBranchQueryConduitAPIMethod.php:33]

Closes T15414

Test Plan

Applied these two changes and at least got to (FilesystemException) Filesystem path "/var/repo/1/" does not exist. as an error message, instead of being stuck at (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated.

Diff Detail

Repository
rP Phorge
Branch
D25241 (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 449
Build 449: arc lint + arc unit

Event Timeline

sgtm

src/applications/diffusion/conduit/DiffusionBranchQueryConduitAPIMethod.php
23

✅ ↑ The contains parameter must be a string

32–33

✅ It seems this is a Conduit API parameter that only accepts strings

https://we.phorge.it/conduit/method/diffusion.branchquery/

This is useful to generate stuff like this:

git branch --verbose --no-abbrev --contains $contains --

From git branch --help:

With --contains, shows only the branches that contain the named commit (in other words, the branches whose tip commits are descendants of the named
commit), --no-contains inverts it. With --merged, only branches merged into the named commit (i.e. the branches whose tip commits are reachable
from the named commit) will be listed. With --no-merged only branches not merged into the named commit will be listed. If the <commit> argument is
missing it defaults to HEAD (i.e. the tip of the current branch).

So, this indeed must be a string.

src/applications/diffusion/controller/DiffusionBranchTableController.php
29

✅ The variable must be null or a string

/**
 * Get the symbolic commit associated with this request.
 *
 * A symbolic commit may be a commit hash, an abbreviated commit hash, a
 * branch name, a tag name, or an expression like "HEAD^^^". The symbolic
 * commit may also be absent.
 *
 * This method always returns the symbol present in the original request,
 * in unmodified form.
 *
 * See also @{method:getStableCommit}.
 *
 * @return string|null  Symbolic commit, if one was present in the request.
 */
public function getSymbolicCommit() {
  return $this->symbolicCommit;
}
This revision is now accepted and ready to land.Jun 2 2023, 17:54