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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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