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

Authored by bob on Aug 28 2023, 12:47.

Details

Reviewers

Sten
valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15628: PHP 8.1 "strlen(null)" exception when displaying a 'recent' subversion commit

Commits

rPacfb44d6679e: Fix a PHP 8.1 deprecated use of strlen with a NULL argument on commit page

Summary

With PHP 8.1+ it is not possible to view a commit if the author field is not properly defined
Indeed, if the commit author 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.

Fix T15628

Test Plan

Push a new commit on a subversion repository (since T15629 is not yet addressed)
Visualize the commit
You should not get a RuntimeException

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bob created this revision.Aug 28 2023, 12:47

Owners added a reviewer: O1: Blessed Committers.Aug 28 2023, 12:47

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptAug 28 2023, 12:47

bob requested review of this revision.Aug 28 2023, 12:47

Harbormaster completed remote builds in B812: Diff 1339.Aug 28 2023, 12:47

bob edited the summary of this revision. (Show Details)Aug 28 2023, 12:47

bob added a task: T15628: PHP 8.1 "strlen(null)" exception when displaying a 'recent' subversion commit.

bob edited the summary of this revision. (Show Details)Aug 28 2023, 13:01

bob edited the summary of this revision. (Show Details)

Looks good

valerio.bozzolan retitled this revision from Fix a PHP 8.1 deprecated use of strlen with a NULL argument to Fix a PHP 8.1 deprecated use of strlen with a NULL argument on commit page.Aug 28 2023, 19:58

I'm 5% scared by this since in 8 months we may discover that $author could be an object and so an user may report our well-known exception in some cases. But, the method name is getAuthorString() so I'm quite sure that Evan Priestley already choosen that name for a reason, and so, probably the getAuthor() is always a string or null and nothing else, so it's nice to report alien things with an exception, as proposed here.

Things we can do in a parallel universe:

better inspect and fix that 5% of doubt (impossible to me)
or, adopt phutil_nonempty_stringlike() (but some people don't like this and I somehow agree)
or, cast $author to string and check that (maybe the best option)

In any case, I tested, without any problem, so thanks bob. Sorry for the delay.

IMPORTANT: If you are affected by a nuclear implosion after this change, please contact Phorge and we will dig more as usual. Join T15628 and share your stack trace.