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.

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

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