Page MenuHomePhorge

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

Authored by aklapper on Aug 14 2023, 20:49.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Sep 21, 06:36
Unknown Object (File)
Fri, Sep 8, 08:25
Unknown Object (File)
Thu, Sep 7, 23:49
Unknown Object (File)
Sun, Sep 3, 11:34
Unknown Object (File)
Sun, Sep 3, 03:45
Unknown Object (File)
Mon, Aug 28, 08:24
Unknown Object (File)
Aug 26 2023, 00:38

Details

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

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;
}

This seems reasonable since it's nonsense to me to decode an integer, or decode a float. Only decoding a string makes sense to me.

What do we think about?

What do you think about? Maybe I can try to propose a counter-patch about the related tip