Page MenuHomePhorge
Differential D25244

Fix PHP 8.1 "strlen(null)" exceptions which block rendering Browse Repositories overlay dialog
ClosedPublic
Actions

Authored by aklapper on May 24 2023, 15:42.
Tags
None
Referenced Files
F4117998: D25244.1747009715.diff
Sun, May 11, 00:28
F4117742: D25244.1747001841.diff
Sat, May 10, 22:17
F4034115: D25244.1746717150.diff
Wed, May 7, 15:12
F4034114: D25244.1746717149.diff
Wed, May 7, 15:12
F4034113: D25244.1746717148.diff
Wed, May 7, 15:12
F3945908: D25244.1746445768.diff
Sun, May 4, 11:49
F3878094: D25244.1746215569.diff
Thu, May 1, 19:52
F3863349: D25244.1746180226.diff
Thu, May 1, 10:03
View All 60 Files
Subscribers
Cigaryno
Matthew
speck
tobiaswiese
valerio.bozzolan

Details

Reviewers
valerio.bozzolan
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15420: PHP 8.1 "strlen(null)" exceptions block rendering Browse Repositories overlay dialog
Commits
rP6d57904330bc: Fix PHP 8.1 "strlen(null)" exceptions which block rendering Browse Repositories…
Summary

strlen() was used in Phabricator to check if a generic value is a non-empty string.
This behavior 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.

Closes T15420

Test Plan

Applied these two changes; afterwards "Browse Repositories" overlay dialog gets rendered in web browser and lists existing repositories.

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

aklapper created this revision.May 24 2023, 15:42
Owners added a reviewer: O1: Blessed Committers.May 24 2023, 15:42
Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 24 2023, 15:42
aklapper requested review of this revision.May 24 2023, 15:42
Harbormaster completed remote builds in B452: Diff 800.May 24 2023, 15:42
valerio.bozzolan accepted this revision.May 24 2023, 15:50

Thanks (again)

Indeed the Slug and the Callsign should be null or strings and nothing else.

The function phutil_nonempty_string() will report any alien type, and that is OK here.

I tested this with some fuzzy tests in Diffusion Callsign, without any nuclear implosion.

lgtm

src/applications/diffusion/typeahead/DiffusionRepositoryDatasource.php
57

↑ This assumes all the parts as strings.

This revision is now accepted and ready to land.May 24 2023, 15:50
Closed by commit rP6d57904330bc: Fix PHP 8.1 "strlen(null)" exceptions which block rendering Browse Repositories…. · Explain WhyMay 24 2023, 16:10
This revision was automatically updated to reflect the committed changes.
aklapper added a commit: rP6d57904330bc: Fix PHP 8.1 "strlen(null)" exceptions which block rendering Browse Repositories….

Revision Contents
Changeset List

PathSizePackages
src/
applications/
diffusion/
typeahead/
2 linesO1: Blessed Committers
repository/
storage/
2 linesO1: Blessed Committers

Diff 802

View Options

src/applications/diffusion/typeahead/DiffusionRepositoryDatasource.php

Show First 20 Lines • Show All 2,443 Lines • ▼ Show 20 Lines
/* -( Ngrams )------------------------------------------------------------- */ /* -( Ngrams )------------------------------------------------------------- */
protected function withNgramsConstraint( protected function withNgramsConstraint(
PhabricatorSearchNgrams $index, PhabricatorSearchNgrams $index,
$value) { $value) {
if (strlen($value)) { if (phutil_nonempty_string($value)) {
valerio.bozzolanUnsubmitted
Not Done
Inline Actions

✅ I verified the above line

The $value contains the search string, or null.

The function phutil_nonempty_string() is designed to report alien types like objects, and that is good here.

valerio.bozzolan: ✅ I verified the above line The `$value` contains the search string, or null. The function…
$this->ngrams[] = array( $this->ngrams[] = array(
'index' => $index, 'index' => $index,
'value' => $value, 'value' => $value,
'length' => count(phutil_utf8v($value)), 'length' => count(phutil_utf8v($value)),
); );
} }
return $this; return $this;
▲ Show 20 Lines • Show All 823 Lines • Show Last 20 Lines
View Options

src/applications/repository/storage/PhabricatorRepository.php

Show First 20 Lines • Show All 2,443 Lines • ▼ Show 20 Lines
/* -( Ngrams )------------------------------------------------------------- */ /* -( Ngrams )------------------------------------------------------------- */
protected function withNgramsConstraint( protected function withNgramsConstraint(
PhabricatorSearchNgrams $index, PhabricatorSearchNgrams $index,
$value) { $value) {
if (strlen($value)) { if (phutil_nonempty_string($value)) {
valerio.bozzolanUnsubmitted
Not Done
Inline Actions

✅ I verified the above line

The $value contains the search string, or null.

The function phutil_nonempty_string() is designed to report alien types like objects, and that is good here.

valerio.bozzolan: ✅ I verified the above line The `$value` contains the search string, or null. The function…
$this->ngrams[] = array( $this->ngrams[] = array(
'index' => $index, 'index' => $index,
'value' => $value, 'value' => $value,
'length' => count(phutil_utf8v($value)), 'length' => count(phutil_utf8v($value)),
); );
} }
return $this; return $this;
▲ Show 20 Lines • Show All 823 Lines • Show Last 20 Lines
Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0