Page MenuHomePhorge

Fix PHP 8.1 "strlen(null)" exceptions editing a form when custom field of type Date exists
ClosedPublic

Authored by aklapper on Aug 14 2023, 20:01.
Tags
None
Referenced Files
F1409412: D25389.id1250.diff
Sat, Feb 24, 06:16
F1409402: D25389.id1359.diff
Sat, Feb 24, 06:16
F1409272: D25389.id.diff
Sat, Feb 24, 05:47
F1409113: D25389.id1249.diff
Sat, Feb 24, 05:02
F1408978: D25389.id1251.diff
Sat, Feb 24, 04:31
Unknown Object (File)
Tue, Feb 20, 16:04
Unknown Object (File)
Tue, Feb 20, 13:49
Unknown Object (File)
Sat, Feb 17, 18:38

Details

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_scalar() as a replacement when both string and integers could be passed as a value like here.

Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_scalar() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.

Also fix similar warning for ctype_digit(): Argument of type null will be interpreted as string in the future by checking for null first.

EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=customOAuthUrlencodeNull, ref.master=788098096e11, ref.customOAuthUrlencodeNull=4f0f2043b7e9), phorge(head=master, ref.master=bcfcd9acfc12)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldDate.php:27]
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) 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=bcfcd9acfc12)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldDate.php:35]
EXCEPTION: (RuntimeException) ctype_digit(): Argument of type null will be interpreted as string in the future at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=customOAuthUrlencodeNull, ref.master=788098096e11, ref.customOAuthUrlencodeNull=4f0f2043b7e9), phorge(head=customFieldDate, ref.master=bcfcd9acfc12, ref.customFieldDate=bcfcd9acfc12)
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<arcanist>/src/error/PhutilErrorHandler.php:261]
  #1 <#2> ctype_digit(NULL) called at [<phorge>/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldDate.php:77]

Closes T15601

Test Plan

After applying these three changes and creating a custom field with "type": "date" 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
customFieldDate (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 750
Build 750: arc lint + arc unit

Event Timeline

...and wrong. Needs to check for a scalar instead because with a custom Date field I get: Call to phutil_nonempty_string() expected null or a string, got: int.

Check for scalar instead of string as the passed value can also be an integer

and of course after this round I got an additional RuntimeException: ctype_digit(): Argument of type int will be interpreted as string in the future to also tackle

I love this change, I've looked inside the logic and it seems very correct to me,

There is still a possibility that some weird extensions could use objects with magic __toString() methods to return an int-cast-able integer, and this could break renderEditControl() causing an iper-mega-nuclear-implosion. But this corner case is actually nonsense at the moment to be proactively supported without any proof about the existence of these stakeholders.

So everything works, and we like nuclear implosions for alien values, and aliens will contact us to fix these nuclear implosions, and we like it.

Thanks

sgtm

This revision is now accepted and ready to land.Aug 15 2023, 03:54