Page MenuHomePhorge
Differential D25489

Fix PHP 8.1 "strlen(null)" exception rendering dashboard panel with latest tasks when custom int field configured
ClosedPublic
Actions

Authored by aklapper on Dec 5 2023, 03:23.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 10, 22:19
Unknown Object (File)
Sat, May 4, 14:23
Unknown Object (File)
Sat, May 4, 14:23
Unknown Object (File)
Sat, May 4, 14:23
Unknown Object (File)
Sat, May 4, 14:23
Unknown Object (File)
Sat, May 4, 13:24
Unknown Object (File)
Sat, May 4, 12:30
Unknown Object (File)
Thu, May 2, 02:25
View All 71 Files
Subscribers
Cigaryno
Matthew
tobiaswiese
valerio.bozzolan

Details

Reviewers
speck
valerio.bozzolan
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15685: Fix PHP 8.1 "strlen(null)" exception rendering dashboard panel with latest tasks when custom int field configured
Commits
rP052b5f41c705: Fix PHP 8.1 "strlen(null)" exception rendering dashboard panel with latest…
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.

ERROR 8192: strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [/var/www/html/phorge/phorge/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php:56]

Closes T15685

Test Plan

After configuring a custom int field and a dashboard panel to query and listed the latest created tasks, access the panel and check the PHP error log.

Diff Detail

Repository
rP Phorge
Branch
arcpatch-D25489
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1037
Build 1037: arc lint + arc unit

Event Timeline

aklapper created this revision.Dec 5 2023, 03:23
Owners added a reviewer: O1: Blessed Committers.Dec 5 2023, 03:23
Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptDec 5 2023, 03:23
aklapper requested review of this revision.Dec 5 2023, 03:23
Harbormaster completed remote builds in B970: Diff 1562.Dec 5 2023, 03:23
speck accepted this revision.Dec 5 2023, 12:04
This revision is now accepted and ready to land.Dec 5 2023, 12:04
valerio.bozzolan added a comment.Dec 5 2023, 12:07

I think that the value may be an int, so, causing a nuclear implosion. Probably nice to check for a concrete populated example, and eventually do line 27 or similar ones.

speck requested changes to this revision.Dec 5 2023, 12:18

Good catch. I don’t think it would cause an implosion but might result in not applying constraints when it should

This revision now requires changes to proceed.Dec 5 2023, 12:18
aklapper added a comment.Dec 6 2023, 18:49

Urgh, indeed not the brightest proposed patch (though it worked locally).
Also need to rework/expand patch because there are several more places in this very file to fix.
More stacktraces triggered by going to http://phorge.localhost/maniphest/ :

[2023-12-06 18:43:19] ERROR 8192: strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [/var/www/html/phorge/phorge/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php:14]
arcanist(head=master, ref.master=e46025f7a914), phorge(head=master, ref.master=5ddca7da55e3)
  #0 PhabricatorStandardCustomFieldInt::buildFieldIndexes() called at [<phorge>/src/infrastructure/customfield/field/PhabricatorCustomField.php:664]
  #1 PhabricatorCustomField::buildFieldIndexes() called at [<phorge>/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php:268]
  #2 PhabricatorCustomFieldList::rebuildIndexes(ManiphestTask) called at [<phorge>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:1539]
  #3 PhabricatorApplicationTransactionEditor::applyTransactions(ManiphestTask, array) called at [<phorge>/src/applications/transactions/editengine/PhabricatorEditEngine.php:1149]
  #4 PhabricatorEditEngine::buildEditResponse(ManiphestTask) called at [<phorge>/src/applications/transactions/editengine/PhabricatorEditEngine.php:1000]
  #5 PhabricatorEditEngine::buildResponse() called at [<phorge>/src/applications/maniphest/controller/ManiphestTaskEditController.php:12]
  #6 ManiphestTaskEditController::handleRequest(AphrontRequest) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:284]
  #7 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:203]
  #8 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phorge>/webroot/index.php:35]

[2023-12-06 18:43:19] ERROR 8192: strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [/var/www/html/phorge/phorge/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php:87]
arcanist(head=master, ref.master=e46025f7a914), phorge(head=master, ref.master=5ddca7da55e3)
  #0 PhabricatorStandardCustomFieldInt::validateApplicationTransactions(ManiphestTransactionEditor, string, array) called at [<phorge>/src/infrastructure/customfield/field/PhabricatorCustomField.php:1010]
  #1 PhabricatorCustomField::validateApplicationTransactions(ManiphestTransactionEditor, string, array) called at [<phorge>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2923]
  #2 PhabricatorApplicationTransactionEditor::validateTransaction(ManiphestTask, string, array) called at [<phorge>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:1234]
  #3 PhabricatorApplicationTransactionEditor::applyTransactions(ManiphestTask, array) called at [<phorge>/src/applications/transactions/editengine/PhabricatorEditEngine.php:1149]
  #4 PhabricatorEditEngine::buildEditResponse(ManiphestTask) called at [<phorge>/src/applications/transactions/editengine/PhabricatorEditEngine.php:1000]
  #5 PhabricatorEditEngine::buildResponse() called at [<phorge>/src/applications/maniphest/controller/ManiphestTaskEditController.php:12]
  #6 ManiphestTaskEditController::handleRequest(AphrontRequest) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:284]
  #7 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:203]
  #8 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phorge>/webroot/index.php:35]

[2023-12-06 18:43:19] ERROR 8192: strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [/var/www/html/phorge/phorge/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php:107]
arcanist(head=master, ref.master=e46025f7a914), phorge(head=master, ref.master=5ddca7da55e3)
  #0 PhabricatorStandardCustomFieldInt::getApplicationTransactionHasEffect(ManiphestTransaction) called at [<phorge>/src/infrastructure/customfield/field/PhabricatorCustomField.php:917]
  #1 PhabricatorCustomField::getApplicationTransactionHasEffect(ManiphestTransaction) called at [<phorge>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:679]
  #2 PhabricatorApplicationTransactionEditor::transactionHasEffect(ManiphestTask, ManiphestTransaction) called at [<phorge>/src/applications/maniphest/editor/ManiphestTransactionEditor.php:69]
  #3 ManiphestTransactionEditor::transactionHasEffect(ManiphestTask, ManiphestTransaction) called at [<phorge>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2788]
  #4 PhabricatorApplicationTransactionEditor::filterTransactions(ManiphestTask, array) called at [<phorge>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:1283]
  #5 PhabricatorApplicationTransactionEditor::applyTransactions(ManiphestTask, array) called at [<phorge>/src/applications/transactions/editengine/PhabricatorEditEngine.php:1149]
  #6 PhabricatorEditEngine::buildEditResponse(ManiphestTask) called at [<phorge>/src/applications/transactions/editengine/PhabricatorEditEngine.php:1000]
  #7 PhabricatorEditEngine::buildResponse() called at [<phorge>/src/applications/maniphest/controller/ManiphestTaskEditController.php:12]
  #8 ManiphestTaskEditController::handleRequest(AphrontRequest) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:284]
  #9 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:203]
  #10 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phorge>/webroot/index.php:35]

[2023-12-06 18:43:19] ERROR 8192: strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [/var/www/html/phorge/phorge/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php:109]
arcanist(head=master, ref.master=e46025f7a914), phorge(head=master, ref.master=5ddca7da55e3)
  #0 PhabricatorStandardCustomFieldInt::getApplicationTransactionHasEffect(ManiphestTransaction) called at [<phorge>/src/infrastructure/customfield/field/PhabricatorCustomField.php:917]
  #1 PhabricatorCustomField::getApplicationTransactionHasEffect(ManiphestTransaction) called at [<phorge>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:679]
  #2 PhabricatorApplicationTransactionEditor::transactionHasEffect(ManiphestTask, ManiphestTransaction) called at [<phorge>/src/applications/maniphest/editor/ManiphestTransactionEditor.php:69]
  #3 ManiphestTransactionEditor::transactionHasEffect(ManiphestTask, ManiphestTransaction) called at [<phorge>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2788]
  #4 PhabricatorApplicationTransactionEditor::filterTransactions(ManiphestTask, array) called at [<phorge>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:1283]
  #5 PhabricatorApplicationTransactionEditor::applyTransactions(ManiphestTask, array) called at [<phorge>/src/applications/transactions/editengine/PhabricatorEditEngine.php:1149]
  #6 PhabricatorEditEngine::buildEditResponse(ManiphestTask) called at [<phorge>/src/applications/transactions/editengine/PhabricatorEditEngine.php:1000]
  #7 PhabricatorEditEngine::buildResponse() called at [<phorge>/src/applications/maniphest/controller/ManiphestTaskEditController.php:12]
  #8 ManiphestTaskEditController::handleRequest(AphrontRequest) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:284]
  #9 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:203]
  #10 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phorge>/webroot/index.php:35]
aklapper updated this revision to Diff 1576.Dec 6 2023, 22:07

Still WIP, was just checking that nothing explodes locally (yet)

Harbormaster completed remote builds in B977: Diff 1576.Dec 6 2023, 22:07
valerio.bozzolan added a comment.EditedJan 16 2024, 14:20

Useful riepilogue for wild integers

$v(bool)strlen($v)phutil_nonempty_scalar($v)is_int($v) && $v !== null(int) $v(int) phutil_string_cast($v)
nullfalsefalsefalse00
""falsefalsefalse00
1truetruetrue11
"lol"truetruefalse00
"1"truetruefalse11
1.1truetruefalse11
"1.1"truetruefalse11
{"1"}truetruefalseexception1
{""}falsefalsefalseexception0
NOTE: With {"1"} I mean an object with a __toString() that returns "1" - like class One { function __toString() { return "1"; } }
NOTE: With {""} I mean an object with a __toString() that returns "" - like class None { function __toString() { return ""; } }

In short, for wild integers, the is_int() is not OK because of it excludes "1". Maybe is_numeric() but I have not tested. Maybe just:

FromTo
if (strlen($v))if (phutil_nonempty_scalar($v))
if (!strlen($v))if (!phutil_nonempty_scalar($v))

Edited: note that is_int($v) && $v !== null is exactly the same as is_int($v)

aklapper updated this revision to Diff 1657.Jan 22 2024, 18:43

Play it safe by using phutil_nonempty_scalar() and do not assume that integers are actually integers (because "1" and such)

Harbormaster completed remote builds in B1037: Diff 1657.Jan 22 2024, 18:43
valerio.bozzolan accepted this revision.Feb 12 2024, 10:01

Whops, forgot to approve this. Sorry! And thanks.

I was able to test this with custom int fields, successfully.

valerio.bozzolan added a comment.Feb 12 2024, 10:03

(@speck red light)

speck accepted this revision.Feb 12 2024, 12:23

whoops

This revision is now accepted and ready to land.Feb 12 2024, 12:23
Closed by commit rP052b5f41c705: Fix PHP 8.1 "strlen(null)" exception rendering dashboard panel with latest…. · Explain WhyFeb 12 2024, 12:26
This revision was automatically updated to reflect the committed changes.
aklapper added a commit: rP052b5f41c705: Fix PHP 8.1 "strlen(null)" exception rendering dashboard panel with latest….

Revision Contents
Changeset List

PathSizePackages
src/
infrastructure/
customfield/
standard/
10 linesO1: Blessed Committers

Diff 1657

View Options

src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldInt.php

Loading...
Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0