Details
- Reviewers
epriestley - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T13588: PHP 8 Compatibility
- Commits
- rARC34b20ec7d88d: Updates for PHP 8.2 compatibility - Arcanist
- Tried to create a diff with invalid diff contents via the web page
- Run arc unit --everything
Diff Detail
- Repository
- rARC Arcanist
- Branch
- cspeck-php8
- Lint
Lint Warnings Severity Location Code Message Warning src/parser/ArcanistDiffParser.php:266 TXT3 Line Too Long - Unit
Test Failures - Build Status
Buildable 25788 Build 35616: arc lint + arc unit
| Time | Test | |
|---|---|---|
| 2,365 ms | ArcanistBundleTestCase::testGitRepository EXCEPTION (RuntimeException): strlen(): Passing null to parameter #1 ($string) of type string is deprecated
#0 /Users/cspeckrun/Source/phacility/arcanist/src/parser/ArcanistBundle.php(798): PhutilErrorHandler::handleError(8192, 'strlen(): Passi...', '/Users/cspeckru...', 798)
#1 /Users/cspeckrun/Source/phacility/arcanist/src/parser/ArcanistBundle.php(409): ArcanistBundle->buildBinaryChange(Object(ArcanistDiffChange), NULL)
| |
| 0 ms | ArcanistBaseCommitParserTestCase::testBasics 2 assertion(s) passed. | |
| 0 ms | ArcanistBaseCommitParserTestCase::testHalt 1 assertion(s) passed. | |
| 0 ms | ArcanistBaseCommitParserTestCase::testJump 1 assertion(s) passed. | |
| 0 ms | ArcanistBaseCommitParserTestCase::testJumpReturn 1 assertion(s) passed. | |
| View Full Test Results (1 Failed · 77 Passed) |
Event Timeline
| src/utils/utils.php | ||
|---|---|---|
| 1310 ↗ | (On Diff #52138) | 2023/05/24 22:58:48 [error] 91#91: *441 FastCGI sent in stderr: "PHP message: [2023-05-24 18:58:48] EXCEPTION: (RuntimeException) substr(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]; PHP message: arcanist(head=cspeck-php8, ref.master=0fc22183e796, ref.cspeck-php8=1b2082e959d5), phabricator(head=cspeck-php8-diffusion, ref.master=58995268dd97, ref.cspeck-php8-diffusion=fbda1ebf4bdf); PHP message: #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<arcanist>/src/error/PhutilErrorHandler.php:261]; PHP message: #1 <#2> substr(NULL, integer, integer) called at [<arcanist>/externals/jsonlint/src/Seld/JsonLint/JsonParser.php:484]; PHP message: #2 <#2> JsonLintJsonParser::failOnBOM(NULL) called at [<arcanist>/externals/jsonlint/src/Seld/JsonLint/JsonParser.php:135]; PHP message: #3 <#2> JsonLintJsonParser::parse(NULL, integer) called at [<arcanist>/src/parser/PhutilJSONParser.php:29] PHP message: #4 <#2> PhutilJSONParser::parse(NULL) called at [<arcanist>/src/utils/utils.php:1314]; PHP message: #5 <#2> phutil_json_decode(NULL) called at [<phabricator>/src/applications/differential/customfield/DifferentialAuditorsField.php:24]; PHP message: #6 <#2> DifferentialAuditorsField::setValueFromStorage(NULL) called at [<phabricator>/src/infrastructure/customfield/query/PhabricatorCustomFieldStorageQuery.php:78]; PHP message: #7 <#2> PhabricatorCustomFieldStorageQuery::loadFieldsFromStorage(DifferentialCustomFieldStorage, array) called at [<phabricator>/src/infrastructure/customfield/query/PhabricatorCustomFieldStorageQuery.php:47]; PHP message: #8 <#2> PhabricatorCustomFieldStorageQuery::execute() called at [<phabricator>/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php:60]; PHP message: #9 <#2> PhabricatorCustomFieldList::readFieldsFromStorage(DifferentialRevision) called at [<phabricator>/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php:152 |
Fix a null issue that came up while using user/pass for http authentication to git/hg
I think the JSON change is suspicious, everything else looks good.
| src/future/oauth/PhutilOAuth1Future.php | ||
|---|---|---|
| 232–233 ↗ | (On Diff #52146) | Or coalesce($consumer_secret, ''), like SQL COALESCE(...). |
| src/utils/utils.php | ||
| 1310 ↗ | (On Diff #52138) | This modification possibly changes the behavior of the function, although I'm not totally sure what PhutilJSONParser did before. Before, I suspect phutil_json_decode(null) likely threw an exception? And I think this is the desired behavior: null isn't valid JSON. After the change, the $string !== null check will cause us to return false now. I'd say this isn't good (that is, phutil_json_decode(null) shouldn't be false) so I think a better change here is probably to just do something like this explicitly at the top of the function and make sure null produces an exception: if ($string === null) {
throw new PhutilJSONParserException(pht('Value "null" is not a valid JSON encoded object.'));
}Alternatively, you could try to fix the substr() call inside JsonParser so we reach a natural exception somewhere in those internals. |
| src/utils/utils.php | ||
|---|---|---|
| 1310 ↗ | (On Diff #52138) | Would throwing a PhutilJSONParserException be handled differently from the error thrown passing null into substr? I did make an assumption that any exception would result in the same issue. I debated looking into the substr issue at the call site but decided against modifying library code, in the event maybe the library is updated or something, though I suppose if it was it'd probably be updated for PHP8 and not require merging and/or break a bunch of call-sites in the process. |
PhutilJSONParserException is sometimes handled specifically:
$ git grep -i PhutilJSONParserException | grep catch src/aphront/AphrontRequest.php: } catch (PhutilJSONParserException $ex) { src/applications/auth/adapter/PhutilAmazonAuthAdapter.php: } catch (PhutilJSONParserException $ex) { src/applications/auth/adapter/PhutilDisqusAuthAdapter.php: } catch (PhutilJSONParserException $ex) { src/applications/auth/adapter/PhutilFacebookAuthAdapter.php: } catch (PhutilJSONParserException $ex) { src/applications/auth/adapter/PhutilGitHubAuthAdapter.php: } catch (PhutilJSONParserException $ex) { ...
At least some of these convert the message into a nice human-readable error UI, so it's slightly desirable to get the function throwing PhutilJSONParserException instead of RuntimeException. Whatever exception JSONParser is supposed to raise probably also has a more helpful message than "substring error" too, although maybe not.
I think modifying the library is totally fine if that's easiest, it has been modified a handful of times anyway (D9627, etc) and, yeah, this probably doesn't make any update we might perform in the future meaningfully more challenging.
Switch to using coalesce(), revert json parsing behavior change and throw exception instead, update .editorconfig
I tried creating, updating, and landing a diff revision with the change to throw an exception and none of the workflows were interrupted/error'd.