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

Authored by bob on Aug 16 2023, 10:47.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15605: PHP 8.1 "strlen(null)" exception when deploying a new instance of Phorge

Commits

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

Summary

This strlen() call was preventing a new Phorge instance to be deployed/configured.
Indeed, on a fresh instance, configuration's "base-uri" key may not be defined witch lead to a Runtime 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 T15605

Test Plan

Checkout a fresh Phorge local copy from official 'https://we.phorge.it/source/phorge.git'
Install/Configure local webserver/database
Open http://phorge.domain in you browser
Configure Phorge database (as requested by webpage)
Create Phorge database (as requested by webpage)
You should be able to reach administrator account page instead of getting a RuntimeException

Diff Detail

Repository

rP Phorge

Branch

arcpatch-D25394

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 774
Build 774: arc lint + arc unit

Event Timeline

bob created this revision.Aug 16 2023, 10:47

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

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

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

Harbormaster completed remote builds in B761: Diff 1264.Aug 16 2023, 10:47

I removed another commit that Arcanist sent in this diff

Thaanks bob

Interestingly the string "0" is not supposed to be a valid URI, so we can just rely on this fast and readable stuff:

if(!$base_uri) {

}

Do you like this?

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, 09:30

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

IMO, this depends on the objective of this check, I mean, does this check aim to verify if this parameter is defined or does it aim to verify if this parameter looks valid...
Maybe I'm wrong but I guess that the first option was in mind here else a regex would probable be a better approach since the "1" would be considered as a valid URI whereas "0" would not...

I just mean that if($something) is usually avoided in Phorge "only and mainly" because if("0") is counter-intuitively false (so "0" is falsy even if it's a non-empty string). But here, we have not that problem, since "0" is a don't care value.

Feel free to use if(!$base_uri) if you like faster and shorter things.

Also this is an excuse to run arc diff again, since tests and lints were skippeds :^)