Fix PhabricatorRepository generateURI PHP 8.1 strlen(null) errors
ClosedPublic
Actions

Authored by Sten on Jul 5 2023, 14:35.

Details

Reviewers

avivey

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15532: PhabricatorRepository generateURI generates multiple strlen(null) errors under PHP 8.1

Commits

rP849e18ccbbea: Fix PhabricatorRepository generateURI PHP 8.1 strlen(null) errors

Summary

When viewing a repository in Diffusion, clicking on a folder (eg https://my.phorge.site/source/myrepo/browse/master/myfolder/) will generate multiple strlen(null) exceptions under PHP 8.1

Fix is to replace all the strlen() calls with phutil_nonempty_string()

Fixes T15532

Test Plan

View a folder in a repo in Diffusion. Eg https://my.phorge.site/source/myrepo/browse/master/myfolder/

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Sten created this revision.Jul 5 2023, 14:35

Owners added a reviewer: O1: Blessed Committers.Jul 5 2023, 14:35

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptJul 5 2023, 14:35

Sten requested review of this revision.Jul 5 2023, 14:35

Harbormaster completed remote builds in B646: Diff 1081.Jul 5 2023, 14:35

valerio.bozzolan added inline comments.Jul 6 2023, 19:18

src/applications/repository/storage/PhabricatorRepository.php
755	Interestingly, `$commit` here is considered a `_scalar()` and not a `_string()`

Thanks. Also here I honestly need more help

Happy to change those phutil_nonempty_string() into phutil_nonempty_scalar() because

The only place it matters is in DiffusionExternalController.php where this is called with:

$redirect = $repository->generateURI(
   array(
     'action' => 'browse',
     'branch' => $repository->getDefaultBranch(),
     'commit' => $id,
   ));

Maybe $id might be an int or float under some non-git VCS.

Values from this are passed through phutil_http_parameter_pair() which "Typecheck and cast an HTTP key-value parameter pair. Scalar values are converted to strings."

So - it's safe to check for nonempty scalar instead of string, works for me, and might prevent issues in non-git VCS.

Harbormaster completed remote builds in B658: Diff 1107.Jul 7 2023, 21:01

Probably fine. Most of these probably should be Strings anyway, but some can be integers in SVN or some other strange cases. Maybe we'll get xphast to tell us more about types, like the common Python tools.

This revision is now accepted and ready to land.Jul 17 2023, 05:56

Closed by commit rP849e18ccbbea: Fix PhabricatorRepository generateURI PHP 8.1 strlen(null) errors. · Explain WhyJul 17 2023, 10:07

This revision was automatically updated to reflect the committed changes.

Sten added a commit: rP849e18ccbbea: Fix PhabricatorRepository generateURI PHP 8.1 strlen(null) errors.