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
Unknown Object (File)
Mon, Mar 25, 07:35
Unknown Object (File)
Mon, Mar 25, 07:33
Unknown Object (File)
Mon, Mar 25, 07:33
Unknown Object (File)
Mon, Mar 25, 06:47
Unknown Object (File)
Mon, Mar 25, 06:16
Unknown Object (File)
Sun, Mar 24, 06:48
Unknown Object (File)
Wed, Mar 20, 10:49
Unknown Object (File)
Sun, Mar 10, 21:26

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
T15408 (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 447
Build 447: 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

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

✅ 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
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)

Not sure how to proceed here, this abandoning for now