Fix PHP 8.1 "strlen(null)" exceptions creating a Diffusion Identity without entering assignee
ClosedPublic
Actions

Authored by aklapper on Jun 2 2023, 17:58.

Details

Reviewers

speck

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15440: PHP 8.1 strlen(null) and mb_detect_encoding(null) exceptions creating a Diffusion Identity without entering assignee

Commits

rPb09471d55748: Fix PHP 8.1 "strlen(null)" exceptions creating a Diffusion Identity without…

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.

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.

EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=b325304b6e52), phorge(head=diffusionCreateIdentity, ref.master=cbc0e661544a, ref.diffusionCreateIdentity=cbc0e661544a)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/repository/xaction/PhabricatorRepositoryIdentityAssignTransaction.php:55]

EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=b325304b6e52), phorge(head=diffusionCreateIdentity, ref.master=cbc0e661544a, ref.diffusionCreateIdentity=cbc0e661544a)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:298]

EXCEPTION: (RuntimeException) mb_detect_encoding(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=b325304b6e52), phorge(head=diffusionCreateIdentity, ref.master=cbc0e661544a, ref.diffusionCreateIdentity=cbc0e661544a)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<arcanist>/src/error/PhutilErrorHandler.php:261]
  #1 <#2> mb_detect_encoding(NULL, array) called at [<phorge>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:313]

Closes T15440

Test Plan

After applying these three changes, going to /diffusion/identity/edit/form/default/ and selecting the "Create Identity" button will only show a AphrontQueryException unrelated to PHP 8 and covered in T15439 instead.

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.Jun 2 2023, 17:58

Owners added a reviewer: O1: Blessed Committers.Jun 2 2023, 17:58

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptJun 2 2023, 17:58

aklapper requested review of this revision.Jun 2 2023, 17:58

Harbormaster completed remote builds in B501: Diff 872.Jun 2 2023, 17:58

valerio.bozzolan added inline comments.Jun 3 2023, 10:03

src/infrastructure/storage/lisk/PhabricatorLiskDAO.php
313	🔶 It this a test?

It this a test?

No but you found a mistake. :) Calling getUTF8StringFromStorage($string, $encoding) both (!) parameters are null - see the second exception about line 298. Thus I initially replaced the strlen call in line 298 but then realized that mb_detect_encoding accepts the second parameter to be null. Still passed but probably only because I had already changed line 293 so this wasn't reached anymore, sigh. So the correct change would be updating line 298 indeed, uhm.

Fix broken code

Harbormaster completed remote builds in B505: Diff 879.Jun 3 2023, 10:18

aklapper retitled this revision from Fix PHP 8.1 exceptions creating a Diffusion Identity without entering assignee to Fix PHP 8.1 "strlen(null)" exceptions creating a Diffusion Identity without entering assignee.Jun 3 2023, 10:29

aklapper edited the summary of this revision. (Show Details)

I think there's another issue here. I don't think this functionality is fleshed out and not an issue with PHP 8.1/2. On this install and others I get 502 gateway when trying to use the Create Identity form. I tried playing around with updating PhabricatorRepositoryEditEngine::newEditableObject() so that it assigns the authorPHID (the field I initially see being reported as not allowed to be null) so it's the current viewer. After doing that I got another error about the identity hash not being allowed to be null. Because of this I don't think we should update getUTF8StringFromStorage() here and just expect this form to blow up for the time being.

speck requested changes to this revision.Jun 8 2023, 00:29

speck added inline comments.