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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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