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
F2182769: D25399.id.diff
Wed, May 8, 07:59
F2182670: D25399.diff
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
Unknown Object (File)
Fri, May 3, 13:05
Unknown Object (File)
Thu, May 2, 04:36
Unknown Object (File)
Apr 6 2024, 04:58

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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