Page MenuHomePhorge

Fix PHP 8.1 exceptions which block adding an embedded File preview as a Comment
ClosedPublic

Authored by aklapper on May 12 2023, 17:22.
Tags
None
Referenced Files
F2159276: D25221.id726.diff
Thu, Apr 25, 07:11
F2159275: D25221.id787.diff
Thu, Apr 25, 07:11
F2159274: D25221.id777.diff
Thu, Apr 25, 07:11
F2159226: D25221.id.diff
Thu, Apr 25, 06:22
F2159213: D25221.diff
Thu, Apr 25, 06:07
Unknown Object (File)
Mon, Apr 22, 11:34
Unknown Object (File)
Mon, Apr 22, 09:43
Unknown Object (File)
Sun, Apr 21, 20:40

Details

Summary

strlen() was used in Phabricator to check if a generic value is a non-empty string.
Receiving null 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.

Closes T15389

Test Plan

Applied these three changes; afterwards writing {F1234} in the Task Comment text field finally rendered the image preview in the preview area, plus I could successfully add the Comment.

Additional cute tests:

Test alt:

{F1234,alt=0}
{F1234,alt=null}
{F1234,alt=123}
{F1234,alt=0.1}
{F1234,alt=[]}

Test width:

{F1234,width=0}
{F1234,width=10}
{F1234,width=10.1}
{F1234,width=10%}
{F1234,width=49.99%}
{F1234,width=100%}

Test height:

{F1234,height=0}
{F1234,height=10}
{F1234,height=10.1}
{F1234,height=10%}
{F1234,height=49.99%}
{F1234,height=100%}

Test mix width/height:

{F1234,height=0,width=0}
{F1234,height=10,width=15}
{F1234,height=10.1,width=15}
{F1234,height=10%,width=15%}
{F1234,height=49.99%,width=15%}
{F1234,height=100%,width=15%}

If you have no nuclear implosion and if dimensions are somehow respected and the alt text works, well done! Test passed.

Diff Detail

Repository
rP Phorge
Branch
refFile (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 403
Build 403: arc lint + arc unit

Event Timeline

src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php
349
351

For the records apparently this regex was designed to capture stuff like:

  • 0
  • 1
  • 0.1
  • 0.1%
353

We are inside the PHP pitfall if($string) that excludes the string "0".

Interestingly:

preg_match('/^(?:\d*\\.)?\d+%?$/', null)→ 0
preg_match('/^(?:\d*\\.)?\d+%?$/', "0")1

So it seems zero is an useful value to keep included at this level.

valerio.bozzolan retitled this revision from Fix PHP 8.1 exceptions which block adding an embedded file preview as a comment to Fix PHP 8.1 exceptions which block adding an embedded File preview as a Comment.
valerio.bozzolan edited the summary of this revision. (Show Details)

Thanks again for your original troubleshooting and this patch. Probably we hammered this stuff without causing a cute unexpected nuclear implosion, but who knows.

sgtm

src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php
200

✅ The function phutil_nonempty_string() will explode for any non-string and non-null types, and that is OK here.

This revision is now accepted and ready to land.May 21 2023, 09:38