Page MenuHomePhorge

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

Authored by aklapper on Aug 14 2023, 20:49.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jul 13, 11:52
Unknown Object (File)
Tue, Jul 9, 02:36
Unknown Object (File)
Sat, Jul 6, 18:55
Unknown Object (File)
Sat, Jul 6, 18:55
Unknown Object (File)
Sat, Jul 6, 18:55
Unknown Object (File)
Sat, Jul 6, 18:52
Unknown Object (File)
Sat, Jul 6, 18:51
Unknown Object (File)
Sat, Jul 6, 03:22

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

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

Only call json_decode() in PhabricatorStandardCustomFieldPHIDs.php on string values, instead of not calling setValueFromApplicationTransactions() in PhabricatorCustomFieldEditField.php as proposed by Valerio

if (!is_array($value)) {  // ←←←←←←←←← May be this needs to be converted to is_string($value) or similar thing

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.

Yes this sounds like a cleaner approach indeed so I updated the patch.
However I realize I just patched that very line in rPe610e739cb4294dcab92c3145285a5ffa5c3cf61, so had to adjust a bit more.

Testing both T15602 and T15683 by having both a select and users type custom field configured in /config/edit/maniphest.custom-field-definitions/ I get no exceptions or warnings anymore.

Indeed this fixes the problem. Premising there is still some mystery here that may deserve additional eyes.

src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php
46

Premising that the source code is a bit nonsense. This variable in line 46 is never used 🤔 maybe this case never worked

Comes from rPed7a5078f9546db41d920939883ee93db3dbc6c6

This revision is now accepted and ready to land.Jan 12 2024, 21:54
speck added inline comments.
src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php
46

Is phutil nonempty string function appropriate here?

src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldPHIDs.php
46

Since the purpose is to run json_decode, it seems totally legit to me

I'm still puzzled by the unused $result variable in line 46.

The code setting $result does get reached when: having a custom field which takes more than a single value defined like in T15602 && a Maniphest form configured which uses that custom field, e.g. going to http://phorge.localhost/maniphest/task/edit/form/4/?parent=1&status=open&custom.train.backup=PHID-USER-c33pgqltkiv6v6yl6sz2,PHID-USER-4wojqm5n3xc3qmpzso4z&title=fooo (note the custom.train.backup) && then creating that task. Maybe someone wants to experiment more with these hints, or has an idea what the original intention of an additional call to array_values() here could have been:

if (is_string($value) && phutil_nonempty_string($value)) {
  phlog(pht($value));                   // example output:  ["PHID-USER-c33pgqltkiv6v6yl6sz2","PHID-USER-4wojqm5n3xc3qmpzso4z"]
  $value = json_decode($value, true);
  if (is_array($value)) {               // this is true for our $value because of the json_decode(...,true) called in the line before
    $result = array_values($value);
    phlog(pht(implode(",", $result)));  // example output:  PHID-USER-c33pgqltkiv6v6yl6sz2,PHID-USER-4wojqm5n3xc3qmpzso4z
  }
}
$this->setFieldValue($value);
return $this;

I propose to remove that unused $result array. It's been unused before and looks we were not aware of any issues created by that fact.

Also remove unused $result array

This revision is now accepted and ready to land.Jan 28 2024, 16:08