Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15390: PHP 8.1 "strlen(null)" exception setting story points on a task

Commits

rP538cccc63e0e: Fix PHP 8.1 "strlen(null)" exception setting Story Points on a Task

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. Here we adopt phutil_string_cast() to
reply to the question "is this an empty string?".

Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_string_cast() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.

Closes T15390

Test Plan

Enable maniphest.points in settings, for example with:

./bin/config set maniphest.points --stdin <<< '{"enabled":true}'

Then try to create a Task: it does not explode anymore in PHP 8.1+.

Also try to set various possible values from the Conduit API method "maniphest.edit".

Diff Detail

Repository

rP Phorge

Branch

arcpatch-D25222_2

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 428
Build 428: arc lint + arc unit

Event Timeline

aklapper created this revision.May 12 2023, 21:21

Owners added a reviewer: O1: Blessed Committers.May 12 2023, 21:21

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 12 2023, 21:21

aklapper requested review of this revision.May 12 2023, 21:21

Harbormaster completed remote builds in B405: Diff 729.May 12 2023, 21:21

lol, maybe makes more sense just to remove this three lines instead of blindly replacing a function call? :P

valerio.bozzolan mentioned this in Wikimedia Hackaton 2023 Athens Code Sprint.May 18 2023, 10:16

valerio.bozzolan edited the test plan for this revision. (Show Details)May 20 2023, 15:41

I was a bit suspicious about Conduit API possibilities (https://we.phorge.it/conduit/method/maniphest.edit/) and I was able to cause a crash with a numerical point like 2 via JSON:

echo '{
  "transactions": [
    {
      "type": "points",
      "value": 2
    }
  ],
  "objectIdentifier": "T1"
}' | arc call-conduit --conduit-uri http://phorge.localhost/ --conduit-token "<yourtoken>" -- maniphest.edit

Hoping to be useful I will amend with a proposal fix

This revision now requires changes to proceed.May 20 2023, 15:47

applied a proposed amend after discussing with the kind @aklapper

Harbormaster completed remote builds in B427: Diff 766.May 20 2023, 15:58

whops, remove my newline

Harbormaster completed remote builds in B428: Diff 767.May 20 2023, 15:59

valerio.bozzolan retitled this revision from Fix PHP 8.1 "strlen(null)" exception setting story points on a task to Fix PHP 8.1 "strlen(null)" exception setting Story Points on a Task.May 20 2023, 16:01

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

valerio.bozzolan edited the test plan for this revision. (Show Details)

Thanks for this patch and for our live ping-pong

I tested like in the meme "A software tester walks into a bar" through the API conduit, ordering an integer, a lizard, etc. No nuclear implosions. But probably a real customer will ask to go to the toilet, causing an implosion at the subatomic level.

sgtm