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.
Tags
None
Referenced Files
F2054359: D25180.diff
Wed, Mar 27, 17:32
Unknown Object (File)
Sun, Mar 24, 08:03
Unknown Object (File)
Sun, Mar 24, 02:47
Unknown Object (File)
Tue, Mar 19, 10:08
Unknown Object (File)
Feb 17 2024, 07:59
Unknown Object (File)
Feb 16 2024, 22:58
Unknown Object (File)
Feb 15 2024, 11:35
Unknown Object (File)
Feb 15 2024, 10:17

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

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.