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

Branch

diffusion-svn-commit-exception

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 769
Build 769: arc lint + arc unit

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