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

Authored by bob on Aug 16 2023, 16:17.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15610: PHP 8.2 "strlen(): Passing null to parameter #1 ($string) of type string is deprecated" in svn commit viewer

Commits

rP5bd526646144: Fix a PHP 8.1 deprecated use of strlen with a NULL argument

Summary

This call prevents users to view a commit in subversion repositories
Indeed, if commiter and/or author field is not properly defined strlen is call with a NULL argument.
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 T15610

Test Plan

Sign in (if needed)
Open a diffusion SVN repository
Open a commit without user name and or email
You should be able to view the commit

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bob created this revision.Aug 16 2023, 16:17

Owners added a reviewer: O1: Blessed Committers.Aug 16 2023, 16:17

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptAug 16 2023, 16:17

Harbormaster completed remote builds in B769: Diff 1275.Aug 16 2023, 16:17

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

bob edited the test plan for this revision. (Show Details)Aug 18 2023, 13:51

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 19 2023, 02:52

valerio.bozzolan edited the summary of this revision. (Show Details)

Thanks

Can somebody else test this? I need more time, since I have issues with hosted repositories at the moment :(

Also bob: is "Sign in" really needed in the test plan in your opinion?

Maybe not. Just being able to see the repository is OK (e.g. with a public Phorge and a public repo)

You're right valerio, this sign in is an automatic reflex since my Phorge instance is private ;)

bob edited the test plan for this revision. (Show Details)Aug 28 2023, 09:09

I was trying to test this, but I don't know how to do a Subversion commit without a committer name 🤔

An example repository would be greatly appreciated.

Premising that I 100% blindly trust the changes in PhabricatorRepositoryCommit. I would just to test PhabricatorRepositoryCommitData since it's not about any "raw" string. So I would like to see if getCommitter() could be an object. Because in case, there we need the "like" thing: phutil_nonempty_stringlike().

Honestly, I don't know why we have old commits improperly filled, you can maybe play with svnadmin command on a dummy repo as described here : Change author of SVN commit

Nobody replied for months. I basically like the disclaimer and I basically trust your test in your production. Thanks.

lgtm