Page MenuHomePhorge

Fix PHP 8.1 "strlen(null)" exception which blocks rendering the "Manage Menu" page
ClosedPublic

Authored by aklapper on May 1 2023, 14:34.
Tags
None
Referenced Files
F3252965: D25166.1742418537.diff
Tue, Mar 18, 21:08
F3220691: D25166.1741813027.diff
Tue, Mar 11, 20:57
F2996433: D25166.1740376314.diff
Sun, Feb 23, 05:51
F2994528: D25166.1740313962.diff
Sat, Feb 22, 12:32
F2994523: D25166.1740313662.diff
Sat, Feb 22, 12:27
F2994521: D25166.1740313644.diff
Sat, Feb 22, 12:27
F2994510: D25166.1740312679.diff
Sat, Feb 22, 12:11
F2983850: D25166.1740001940.diff
Feb 18 2025, 21:52

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.
However, the parameter $item_id is a numeric value when populated.
Thus replace strlen() with phutil_nonempty_scalar() as phutil_nonempty_string() could break a Conduit API consumer sending a numeric value.

Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_scalar() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.

Closes T15317

Test Plan

Applied this change and /home/menu/configure/ rendered in web browser.

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

aklapper requested review of this revision.May 1 2023, 14:34
src/applications/search/engine/PhabricatorProfileMenuEngine.php
138

It seems $item_id is a numeric value when populated, like "36". For example when editing a specific menu item.

The risk of phutil_nonempty_string() in this case is to break a Conduit API consumer that sends a clean numerical $item_id ) 36.

To do not introduce unexpected regressions, I think it's better to use phutil_nonempty_scalar() that is a bit more permissive on primitive types like integers.

@aklapper feel free to say "yeah whatever, feel free to apply the proposed fix" - no problem; or, feel free to update

This revision now requires changes to proceed.May 1 2023, 20:24

Fix PHP 8.1 "strlen(null)" exception which blocks rendering the "Manage Menu" page

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.
However, the parameter $item_id is a numeric value when populated.
Thus replace strlen() with phutil_nonempty_scalar() as phutil_nonempty_string() could break a Conduit API consumer sending a numeric value.

Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_scalar() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.

Closes T15317

Test Plan:
Applied this change and /home/menu/configure/ rendered in web browser.

(When using arc diff --update it's still unclear to me how to update the commit message. It seems a separate comment gets added ( https://we.phorge.it/D25166#5298 ) instead.

Thanks again

Tested this change locally and it worked perfectly. I was able to edit a menu entry without implosions. The approach seems just totally backward compatible and without much possibility of introducing any regressions.

yesyes

This revision is now accepted and ready to land.May 2 2023, 20:36