Paths

Table of Contentst

Fix PHP 8.2 "strlen(null)" exceptions block rendering Differential Revision page (T15432 - 1/2)
Needs RevisionPublic
Actions

Authored by mturdus on Jun 1 2023, 19:51.

Details

Reviewers

speck
valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15432: PHP 8.1 "strlen(null)" exceptions block rendering Differential Revision page

Summary

phutil_json_decode() does not accept null as parameter
strlen() does not accept null as parameter
preg_match() does not accept null as parameter 2 and 3
strcasecmp() does not accept null as parameter 1
Closes T15432 partially: Arcanist also needs some fixes

Test Plan

See Task description T15432

Diff Detail

Repository

rP Phorge

Branch

merula

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 522
Build 522: arc lint + arc unit

Event Timeline

mturdus created this revision.Jun 1 2023, 19:51

Owners added a reviewer: O1: Blessed Committers.Jun 1 2023, 19:51

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptJun 1 2023, 19:51

mturdus requested review of this revision.Jun 1 2023, 19:51

Harbormaster completed remote builds in B496: Diff 864.Jun 1 2023, 19:51

speck added inline comments.Jun 1 2023, 20:27

src/applications/differential/customfield/DifferentialBranchField.php
38–46	Instead I think you can update like this
src/applications/differential/editor/DifferentialTransactionEditor.php
235	`$match` is always null here, see the immediate line above. It's probably a bit weird but `preg_match` uses it as an "out" parameter, so after calling that it will be populated with a value.
src/infrastructure/editor/PhabricatorEditorURIEngine.php
20–21	This could be simplified
src/infrastructure/javelin/markup.php
77 ↗	(On Diff #864)	This doesn't seem right. All HTTP requests must have a method. Do you know how this was hit?

speck added inline comments.Jun 1 2023, 20:31

src/infrastructure/javelin/markup.php
77 ↗	(On Diff #864)	You may have run into this https://secure.phabricator.com/D21864#inline-61142

mturdus added inline comments.Jun 1 2023, 20:49

src/infrastructure/javelin/markup.php

77 ↗

(On Diff #864)

I'm not sure. Like the task described, it was a whole tree of exceptions: when one exception was fixed, another appeared.
This was the stacktrace for this exception:

[Thu Jun 01 20:52:06.380370 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732] [2023-06-01 18:52:06] EXCEPTION: (RuntimeException) strcasecmp(): Passing null to parameter #1 ($string1) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
[Thu Jun 01 20:52:06.380558 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732] arcanist(head=master, ref.master=18554ea76ceb), phorge(head=merula, ref.master=e11c5486c92b, ref.merula=6c62f01668a6, custom=1)
[Thu Jun 01 20:52:06.380563 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<arcanist>/src/error/PhutilErrorHandler.php:261]
[Thu Jun 01 20:52:06.380566 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #1 <#2> strcasecmp(NULL, string) called at [<phorge>/src/infrastructure/javelin/markup.php:77]
[Thu Jun 01 20:52:06.380574 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #2 <#2> phabricator_form(PhabricatorUser, array, array) called at [<phorge>/src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php:251]
[Thu Jun 01 20:52:06.380576 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #3 <#2> javelin_tag(string, array, PHUITabView) called at [<phorge>/src/view/phui/PHUITabGroupView.php:119]
[Thu Jun 01 20:52:06.380578 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #4 <#2> phutil_tag(string, array, array) called at [<phorge>/src/infrastructure/javelin/markup.php:70]
[Thu Jun 01 20:52:06.380579 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #5 <#2> javelin_tag(string, array, array) called at [<phorge>/src/view/AphrontTagView.php:158]
[Thu Jun 01 20:52:06.380581 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #6 <#2> phutil_tag(string, array, array) called at [<phorge>/src/view/phui/PHUITwoColumnView.php:236]
[Thu Jun 01 20:52:06.380582 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #7 <#2> PHUITwoColumnView::buildFooter() called at [<phorge>/src/view/phui/PHUITwoColumnView.php:123]
[Thu Jun 01 20:52:06.380584 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #8 <#2> PHUITwoColumnView::getTagContent() called at [<phorge>/src/view/AphrontTagView.php:161]
[Thu Jun 01 20:52:06.380585 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #9 <#2> phutil_tag(string, array, array) called at [<phorge>/src/view/formation/PHUIFormationContentView.php:13]
[Thu Jun 01 20:52:06.380587 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #10 <#2> phutil_escape_html(PHUIFormationContentView) called at [<phorge>/src/infrastructure/markup/render.php:139]
[Thu Jun 01 20:52:06.380588 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #11 <#2> phutil_escape_html(array) called at [<phorge>/src/infrastructure/markup/render.php:97]
[Thu Jun 01 20:52:06.380590 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #12 <#2> phutil_tag(string, array, array) called at [<phorge>/src/view/formation/PHUIFormationView.php:58]
[Thu Jun 01 20:52:06.380592 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #13 <#2> PHUIFormationView::render() called at [<phorge>/src/view/AphrontView.php:222]
[Thu Jun 01 20:52:06.380593 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #14 <#2> AphrontView::producePhutilSafeHTML() called at [<phorge>/src/infrastructure/markup/render.php:115]
[Thu Jun 01 20:52:06.380595 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #15 <#2> phutil_escape_html(PHUIFormationView) called at [<phorge>/src/infrastructure/markup/render.php:171]
[Thu Jun 01 20:52:06.380596 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #16 <#2> phutil_implode_html(string, array) called at [<phorge>/src/view/page/PhabricatorBarePageView.php:58]
[Thu Jun 01 20:52:06.380598 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #17 <#2> PhabricatorBarePageView::willRenderPage() called at [<phorge>/src/view/page/PhabricatorStandardPageView.php:216]
[Thu Jun 01 20:52:06.380599 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #18 <#2> PhabricatorStandardPageView::willRenderPage() called at [<phorge>/src/view/page/AphrontPageView.php:46]
[Thu Jun 01 20:52:06.380601 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #19 <#2> AphrontPageView::render() called at [<phorge>/src/view/page/PhabricatorStandardPageView.php:904]
[Thu Jun 01 20:52:06.380602 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #20 <#2> PhabricatorStandardPageView::produceAphrontResponse() called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:722]
[Thu Jun 01 20:52:06.380604 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #21 <#2> AphrontApplicationConfiguration::produceResponse(AphrontRequest, PhabricatorStandardPageView) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:299]
[Thu Jun 01 20:52:06.380609 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #22 phlog(RuntimeException) called at [<phorge>/src/aphront/handler/PhabricatorDefaultRequestExceptionHandler.php:41]
[Thu Jun 01 20:52:06.380611 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #23 PhabricatorDefaultRequestExceptionHandler::handleRequestThrowable(AphrontRequest, RuntimeException) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:751]
[Thu Jun 01 20:52:06.380613 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #24 AphrontApplicationConfiguration::handleThrowable(RuntimeException) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:337]
[Thu Jun 01 20:52:06.380615 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #25 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:203]
[Thu Jun 01 20:52:06.380616 2023] [php:notice] [pid 4414:tid 140512489100992] [client ::1:48732]   #26 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phorge>/webroot/index.php:35]

speck added inline comments.Jun 1 2023, 21:10

src/infrastructure/javelin/markup.php
77 ↗	(On Diff #864)	Yeah that looks like the same issue. The preferred update is in `DifferentialRevisionUpdateHistoryView` to ensure that a method is specified for the request.

speck added inline comments.Jun 6 2023, 00:33

src/applications/differential/customfield/DifferentialAuditorsField.php
24 ↗	(On Diff #864)	Which of the errors does this one fix? From the stack traces on the task none contain `setValueFromStorage()`, and based on the context here I think anything passing null to `phutil_json_decode` should be resolved by D25250.

avivey subscribed.Jun 6 2023, 08:37

avivey added inline comments.

src/applications/differential/customfield/DifferentialAuditorsField.php
24 ↗	(On Diff #864)	With this change, calling `setValueFromStorage(null)` will have no effect (whereas before it would actually set the value to either `null` or `array()`. So this is not a safe change.
src/applications/differential/editor/DifferentialTransactionEditor.php
236	I think it might be clearer as if ($branch != null && preg_match(....)) { Also, I think this feature (attaching revision to task based on user's branch) is not documented?

speck requested changes to this revision.Jun 8 2023, 00:32

speck added inline comments.

src/applications/differential/editor/DifferentialTransactionEditor.php
235–236	Please make this edit
src/infrastructure/editor/PhabricatorEditorURIEngine.php
20–21	Please avoid having nested if statements with simple conditions, see suggested change.
src/infrastructure/javelin/markup.php
77 ↗	(On Diff #864)	Please revert this change and update `DifferentialRevisionUpdateHistoryView` instead

This revision now requires changes to proceed.Jun 8 2023, 00:32

speck mentioned this in D25263: Fix PHP 8.1 "preg_match(null)" exception on "Create Revision" page in Differential when not entering data.Jun 8 2023, 00:36

Updated according to the comments

Harbormaster completed remote builds in B522: Diff 908.Jun 8 2023, 17:53

Thanks!

This revision is now accepted and ready to land.Jun 8 2023, 18:25

valerio.bozzolan added inline comments.Jun 10 2023, 10:17

src/applications/conduit/controller/PhabricatorConduitAPIController.php
685	Another potential minimal change without using phutil: $params_json = $request->getStr('params', '');
src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php
255	Can this be clarified?
src/infrastructure/editor/PhabricatorEditorURIEngine.php
20–21	Another safe approach thanks to the return type of trim: if ($pattern === null \|\| trim($pattern) === '') {

mturdus added inline comments.Jun 10 2023, 17:39

src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php
255	The `phabricator_form` method contains the following lines: $http_method = idx($attributes, 'method'); $is_post = (strcasecmp($http_method, 'POST') === 0); if the `method` argument is missing, $http_method becomes null. The strcasecmp statement will crash in PHP 8.2

speck added inline comments.Jun 10 2023, 19:32

src/applications/differential/view/DifferentialRevisionUpdateHistoryView.php
255	For other context, this is the only location using phabricator_form but doesn’t specify which http method to use.

avivey added inline comments.Jun 10 2023, 19:43

src/applications/conduit/controller/PhabricatorConduitAPIController.php
685	Maybe `''` should just be the default return value from `getStr()`, so it's never null?

speck added inline comments.Jun 10 2023, 19:51

src/applications/conduit/controller/PhabricatorConduitAPIController.php
685	Possibly but it’s a potentially larger change to also verify things that are expecting only null (and not empty string) as a return value if it’s unspecified.