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)
Sat, Apr 27, 16:47
Unknown Object (File)
Sat, Apr 27, 16:47
Unknown Object (File)
Sat, Apr 27, 16:47
Unknown Object (File)
Sat, Apr 27, 16:47
Unknown Object (File)
Sat, Apr 27, 16:47
Unknown Object (File)
Thu, Apr 25, 22:13
Unknown Object (File)
Wed, Apr 17, 18:17
Unknown Object (File)
Wed, Apr 17, 06:39
View All 63 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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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 1695

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