Page MenuHomePhabricator

Updates for PHP 8.2 compatibility - Arcanist
ClosedPublic

Authored by cspeckmim on May 20 2023, 4:45 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Feb 25, 6:13 AM
Unknown Object (File)
Thu, Feb 22, 3:03 AM
Unknown Object (File)
Fri, Feb 16, 7:11 PM
Unknown Object (File)
Fri, Feb 16, 1:26 PM
Unknown Object (File)
Fri, Feb 16, 1:26 PM
Unknown Object (File)
Tue, Feb 13, 8:50 PM
Unknown Object (File)
Mon, Feb 12, 12:28 AM
Unknown Object (File)
Thu, Feb 8, 5:40 PM
Subscribers

Details

Summary

While testing and updating Phabricator to address PHP 8.2 incompatibilities this diff covers issues that stem from rARC.

Refs T13588

Test Plan
  1. Tried to create a diff with invalid diff contents via the web page
  2. Run arc unit --everything

Diff Detail

Repository
rARC Arcanist
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

cspeckmim held this revision as a draft.

Fixing lint, fixing error that happened during UT

Run all unit tests why not, suss out more exceptions

Fix that came up from phab trying to parse something from a diff with null value

cspeckmim retitled this revision from Updates for PHP 8.2 compatibility to Updates for PHP 8.2 compatibility - Arcanist.May 24 2023, 11:15 PM
src/utils/utils.php
1314
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

Minor cleanup for the OAuth key building

I think the JSON change is suspicious, everything else looks good.

src/future/oauth/PhutilOAuth1Future.php
232–235

Or coalesce($consumer_secret, ''), like SQL COALESCE(...).

src/utils/utils.php
1314

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.

This revision now requires changes to proceed.May 26 2023, 11:45 PM
cspeckmim added inline comments.
src/utils/utils.php
1314

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.

This revision is now accepted and ready to land.May 28 2023, 11:24 PM