Page MenuHomePhorge

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

Authored by aklapper on May 23 2023, 19:10.
Tags
None
Referenced Files
F2162463: D25239.id795.diff
Thu, Apr 25, 10:49
F2162377: D25239.id1198.diff
Thu, Apr 25, 10:48
F2162376: D25239.id1197.diff
Thu, Apr 25, 10:48
F2157641: D25239.id.diff
Wed, Apr 24, 22:24
F2157527: D25239.diff
Wed, Apr 24, 21:17
Unknown Object (File)
Mon, Apr 22, 10:59
Unknown Object (File)
Sun, Apr 21, 23:16
Unknown Object (File)
Sat, Apr 20, 08:41

Details

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
arcpatch-D25239
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 721
Build 721: arc lint + arc unit

Event Timeline

speck requested changes to this revision.Jun 8 2023, 00:59
speck added inline comments.
src/infrastructure/customfield/field/PhabricatorCustomField.php
892–898 ↗(On Diff #795)

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

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...

Remove unrelated Credentials change now covered in T15580 instead

src/applications/passphrase/view/PassphraseCredentialControl.php
53–54 ↗(On Diff #795)

✅ The PHID indeed must be a pure string, or be null.

src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php
46

🔴 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
src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php
43–44

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

Not sure how to proceed here, this abandoning for now