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
Unknown Object (File)
Tue, Mar 26, 06:08
Unknown Object (File)
Sun, Mar 24, 06:38
Unknown Object (File)
Wed, Mar 20, 10:41
Unknown Object (File)
Wed, Mar 13, 05:34
Unknown Object (File)
Wed, Mar 13, 02:04
Unknown Object (File)
Feb 25 2024, 07:38
Unknown Object (File)
Feb 25 2024, 07:38
Unknown Object (File)
Feb 25 2024, 07:38

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

Event Timeline

src/applications/files/markup/PhabricatorEmbedFileRemarkupRule.php
348–349
348–349

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.

351

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

  • 0
  • 1
  • 0.1
  • 0.1%
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