Fix PHP 8.1 exceptions which block rendering Drydock's Create Blueprint page
AbandonedPublic
Actions

Authored by aklapper on May 23 2023, 19:10.

Details

Reviewers

speck
valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15408: PHP 8.1 "json_decode(null)" exception blocks creating Almanac Hosts blueprint in Drydock

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.

In a similar fashion, passing null to json_decode() is also deprecated.

Closes T15408

Test Plan

Applied these two changes and the "Create Blueprint" page on /drydock/blueprint/edit/form/default/ finally rendered in web browser.

Diff Detail

Repository

rP Phorge

Branch

T15408 (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 447
Build 447: arc lint + arc unit

Event Timeline

aklapper created this revision.May 23 2023, 19:10

Owners added a reviewer: O1: Blessed Committers.May 23 2023, 19:10

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 23 2023, 19:10

aklapper requested review of this revision.May 23 2023, 19:10

Harbormaster completed remote builds in B447: Diff 795.May 23 2023, 19:10

aklapper mentioned this in T15413: PHP 8.1 "json_decode(null)" exception blocks creating Almanac Hosts blueprint in Drydock.May 23 2023, 19:24

aklapper mentioned this in D25240: Fix PHP 8.1 "strlen(null)" exception which blocks rendering errors on Create Blueprint page.May 23 2023, 19:25

speck requested changes to this revision.Jun 8 2023, 00:59

speck added inline comments.

src/infrastructure/customfield/field/PhabricatorCustomField.php
892–898	I think this would be a large breaking change - I think null is a valid value to be set here. The issue is likely further up the stack.

This revision now requires changes to proceed.Jun 8 2023, 00:59

aklapper mentioned this in rPc553692d452b: Fix PHP 8.1 "strlen(null)" exception which blocks rendering errors on Create….Jun 10 2023, 20:58

Handle empty $value in PhabricatorStandardCustomFieldPHIDs.php instead

I don't see any better spot up the stack so I'm afraid it makes sense to instead go lower in the stack...

Harbormaster completed remote builds in B720: Diff 1197.Aug 1 2023, 17:31

Remove unrelated Credentials change now covered in T15580 instead

Harbormaster completed remote builds in B721: Diff 1198.Aug 1 2023, 17:34

aklapper mentioned this in T15581: PHP 8.1 "strlen(null)" exception creating Blueprint Working Copy.Aug 1 2023, 17:43

valerio.bozzolan added inline comments.Aug 2 2023, 16:37

src/applications/passphrase/view/PassphraseCredentialControl.php
53–54	✅ The PHID indeed must be a pure string, or be null.
src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php
46 ↗	(On Diff #1198)	🔴 Something seems terribly broken in the legacy code. The `$result` is never used.

After this change it's probably no longer possible to persist NULL.

This revision now requires changes to proceed.Aug 4 2023, 15:53

valerio.bozzolan added inline comments.Aug 4 2023, 15:58

src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php
43–44 ↗	(On Diff #1198)	The goal here is to decode a string. So, checking for "not array" is clearly not a good way to allow-list strings. Interestingly, adopting `is_string()` also should fix the PHP 8.1 problem since NULL is not a string :D so it's never passed as `json_decode(null)`.
46 ↗	(On Diff #1198)