Page MenuHomePhorge

Do not pass a null string to mb_convert_case() for PHP 8.1 compatibility
ClosedPublic

Authored by aklapper on May 3 2023, 14:08.

Details

Summary

Passing null to the $string parameter of mb_convert_case() is deprecated in PHP 8.1.
This is one of the exceptions which block rendering the "Browse Projects" overlay dialog.

Closes part of T15335

Test Plan

Applied this change in Arcanist (plus the four changes in D25179 in Phorge) and the Browse Projects overlay dialog finally rendered in web browser and listed existing projects.

Diff Detail

Repository
rARC Arcanist
Branch
workbColAutoCompl (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 346
Build 346: arc lint + arc unit

Event Timeline

aklapper requested review of this revision.May 3 2023, 14:08

I'm afraid this change should also be applied to a bunch of similar functions in that file?

Thanks for this patch

Since this is a low-level function probably it would be nice an if($str === null) { return ''; } short circuit instead

If you do not have an exception for other functions, I would say to skip this at the moment

Use simpler and more low-level $str === null comparison instead

Fix indentation (where is my linter?)

Thanks just to clarify: does it still fix your problem?

(I think yes but there is honestly a possibility that an object is passed, then casted internally to string by mb_convert_case(), but returning a null. I really do not want to test this corner-case. I just want to say that if this still crashes, it's probably because of that.)

Premising that it would be nice to discover who is calling phutil_utf8_strtolower(null) ignoring the PHPDoc, and premising that if some people has more time to further improve the situation any follow-up patch is welcome,

I tested this locally without any nuclear implosion,

And, this is the very minimal change to short-circuit out this problem introduced by PHP 8.1 deprecation

So, thanks for this!

lgtm

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

Thanks just to clarify: does it still fix your problem?

Yes, it does still fix T15335, when also applying D25179 in addition to D25180 here.