Page MenuHomePhorge

Fix a PHP 8.1 deprecated use of strlen with a NULL argument
ClosedPublic

Authored by bob on Aug 16 2023, 16:05.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 10, 00:14
Unknown Object (File)
Fri, May 10, 00:14
Unknown Object (File)
Fri, May 10, 00:14
Unknown Object (File)
Wed, May 8, 07:59
Unknown Object (File)
Wed, May 8, 07:32
Unknown Object (File)
Fri, May 3, 22:24
Unknown Object (File)
Fri, May 3, 22:24
Unknown Object (File)
Fri, May 3, 13:05

Details

Summary

With PHP 8.1+ it was not possible to view a commit from subversion repositories.
Indeed, if the commit user and/or email is not properly defined, strlen(null) is called, causing a deprecation warning, elevated to exception.
Using strlen() to check string validity 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.

Fix T15609

Test Plan
  • Apply D25397 and D25398
  • Sign in
  • Open a diffusion SVN repository
  • Open a commit without user name and or email
  • You should not see the same Runtime Exception (unfortunately, there is another one as described in T15610)

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25399
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 770
Build 770: arc lint + arc unit

Event Timeline

bob requested review of this revision.Aug 16 2023, 16:05

Thanks for this change! Just a small thing

src/applications/diffusion/data/DiffusionCommitRef.php
133–135

Probably we should not make these values as optional. Just introducing coalesce is very OK

valerio.bozzolan retitled this revision from Fix a PHP 8.1/8.2 deprecated use of strlen with a NULL argument to Fix a PHP 8.1 deprecated use of strlen with a NULL argument.Aug 17 2023, 07:47
valerio.bozzolan edited the summary of this revision. (Show Details)

Updating D25399: Reverting formatUser method prototype

bob marked an inline comment as done.Aug 17 2023, 08:28

For some reasons I'm not able to trigger this problem, but that seems perfect to me, thaaaaanks

lgtm

Several ideas/leads about the fact you cannot reproduce :

  • On my instance, subversion repositories are only observed by Phorge
  • These repositories are pretty old and, I'm not sure that oldest commit data are properly filled
  • Any other mysterious/good reason...

Yeah anyway this looks great

whatcouldgowrong

This revision is now accepted and ready to land.Aug 18 2023, 10:35

Great, let's prepare for landing ! BTW, I like your images, espacially this one...