Page MenuHomePhorge

Fix PhabricatorRepository generateURI PHP 8.1 strlen(null) errors
ClosedPublic

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

Details

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 requested review of this revision.Jul 5 2023, 14:35
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

  1. 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.

  1. 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.

avivey subscribed.

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