Fix PHP 8.1 "json_decode(null)" exception editing a form when custom field of type Users exists
Needs ReviewPublic
Actions

Authored by aklapper on Aug 14 2023, 20:49.

Details

Reviewers

None

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15602: PHP 8.1 "json_decode(null)" exception editing a form when custom field of type Users exists

Summary

When $value is null, do not pass $value down the stack in buildControl() to ultimately end up with json_decode complaining. Instead, just skip this call.

EXCEPTION: (RuntimeException) json_decode(): Passing null to parameter #1 ($json) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=customOAuthUrlencodeNull, ref.master=788098096e11, ref.customOAuthUrlencodeNull=4f0f2043b7e9), phorge(head=customFieldDate, ref.master=bcfcd9acfc12, ref.customFieldDate=ae8cbe84252d)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<arcanist>/src/error/PhutilErrorHandler.php:261]
  #1 <#2> json_decode(NULL, boolean) called at [<phorge>/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php:44]
  #2 <#2> PhabricatorStandardCustomFieldPHIDs::setValueFromStorage(NULL) called at [<phorge>/src/infrastructure/customfield/field/PhabricatorCustomField.php:895]
  #3 <#2> PhabricatorCustomField::setValueFromApplicationTransactions(NULL) called at [<phorge>/src/infrastructure/customfield/editor/PhabricatorCustomFieldEditField.php:70]
  #4 <#2> PhabricatorCustomFieldEditField::buildControl() called at [<phorge>/src/applications/transactions/editfield/PhabricatorEditField.php:385]

Closes T15602

Test Plan

After applying these three changes and creating a custom field with "type": "users" under /config/edit/maniphest.custom-field-definitions/, the website /transactions/editengine/maniphest.task/view/5/ renders correctly in the browser, showing "This is a preview of the current form configuration."

Diff Detail

Repository

rP Phorge

Branch

T15602 (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 752
Build 752: arc lint + arc unit

Event Timeline

aklapper created this revision.Aug 14 2023, 20:49

Owners added a reviewer: O1: Blessed Committers.Aug 14 2023, 20:49

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptAug 14 2023, 20:49

aklapper requested review of this revision.Aug 14 2023, 20:49

Harbormaster completed remote builds in B752: Diff 1252.Aug 14 2023, 20:49

Thanks my friend

I have some doubts about the root problem here. Maybe it's OK that we set NULL from application transactions.

Looking at the stack trace, this also seems interestingly wrong, and maybe it's the only thing that we should handle:

public function setValueFromStorage($value) {
  // NOTE: We're accepting either a JSON string (a real storage value) or
  // an array (from HTTP parameter prefilling). This is a little hacky, but
  // should hold until this can get cleaned up more thoroughly.
  // TODO: Clean this up.

  $result = array();
  if (!is_array($value)) {  // ←←←←←←←←← May be this needs to be converted to is_string($value) or similar thing
    $value = json_decode($value, true);
    if (is_array($value)) {
      $result = array_values($value);
    }
  }

  $this->setFieldValue($value);

  return $this;
}