Fix PHP 8 exit status cannot be null error in PhutilArgumentParser
AcceptedPublic
Actions

Authored by xtex on Tue, Feb 11, 01:58.

Details

Reviewers

Group Reviewers

Blessed Committers
O1: Blessed Committers

Maniphest Tasks

T15990: ERROR 8192: exit(): Passing null to parameter #1 ($status) of type string|int is deprecated at [arcanist/src/parser/argument/PhutilArgumentParser.php:494] on PHP 8.4

Summary

Passing null as the first parameter ($status) to exit has been deprecated
and leads to a PHP error.

Closes T15990

Test Plan

Run any bin scripts with PHP 8.4.
bin/arc version should finish successfully without PHP errors.

Diff Detail

Repository

rARC Arcanist

Branch

master

Lint

Lint Skipped

Unit

Test Failures

Build Status

Buildable 1689
Build 1689: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	2,780 ms	ArcanistBundleTestCase::testGitRepository EXCEPTION (Exception): Expected patch and actual patch for 5dec8bf28557f078d1987c4e8cfb53d08310f522 differ. Wrote actual patch to '/home/xtex/src/aosc/tracking/arcanist/src/parser/__tests__/patches//5dec8bf28557f078d1987c4e8cfb53d08310f522.gitpatch.real'. #0 /home/xtex/src/aosc/tracking/arcanist/src/parser/__tests__/ArcanistBundleTestCase.php(87): ArcanistBundleTestCase->runGitRepositoryTests(Object(PhutilDirectoryFixture)) #1 /home/xtex/src/aosc/tracking/arcanist/src/unit/engine/phutil/PhutilTestCase.php(639): ArcanistBundleTestCase->testGitRepository()
	166 ms	PhutilLibraryTestCase::testLibraryMap EXCEPTION (CommandException): JSON command 'php -f /home/xtex/src/aosc/tracking/arcanist/src/moduleutils/../../support/lib/extract-symbols.php -- --ugly -- /home/xtex/src/aosc/tracking/arcanist/src/browse/query/ArcanistBrowseCommitHardpointQuery.php' emitted text to stderr when none was expected: 0 COMMAND php -f /home/xtex/src/aosc/tracking/arcanist/src/moduleutils/../../support/lib/extract-symbols.php -- --ugly -- /home/xtex/src/aosc/tracking/arcanist/src/browse/query/ArcanistBrowseCommitHardpointQuery.php
	0 ms	ArcanistBaseCommitParserTestCase::testBasics 2 assertion(s) passed.
	0 ms	ArcanistBaseCommitParserTestCase::testHalt 1 assertion(s) passed.
	0 ms	ArcanistBaseCommitParserTestCase::testJump 1 assertion(s) passed.
		View Full Test Results (2 Failed · 98 Passed)

Event Timeline

xtex created this revision.Tue, Feb 11, 01:58

Owners added a reviewer: O1: Blessed Committers.Tue, Feb 11, 01:58

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptTue, Feb 11, 01:58

xtex requested review of this revision.Tue, Feb 11, 01:58

Harbormaster completed remote builds in B1689: Diff 2626.Tue, Feb 11, 01:58

One test is failing for PHP 8 (:
Lints are skipped because the same problem make linting failed for me.

xtex added a project: PHP 8 support.Tue, Feb 11, 02:06

none

Harbormaster completed remote builds in B1690: Diff 2627.Tue, Feb 11, 02:09

xtex edited the test plan for this revision. (Show Details)Tue, Feb 11, 07:08

Thanks! I'm sorry that the execute method has undocumented return.

https://we.phorge.it/source/arcanist/browse/master/src/parser/argument/workflow/PhutilArgumentWorkflow.php;ec68f53ba2d6d209d2930046fe1fedd9ffc3179d$175?as=source

As far as I understand, here ArcanistPhutilWorkflow#execute() runs executeWorkflow and keeping its return:

https://we.phorge.it/source/arcanist/browse/master/src/toolset/ArcanistPhutilWorkflow.php$21

That returns what is returned by runWorkflow() or it throws

https://we.phorge.it/source/arcanist/browse/master/src/workflow/ArcanistWorkflow.php$244

So it seems to me that the root problem is that these methods should always return an int:

$ grep -R 'function runWorkflow'
src/toolset/workflow/ArcanistVersionWorkflow.php:  public function runWorkflow() {
src/toolset/workflow/ArcanistAliasWorkflow.php:  public function runWorkflow() {
src/toolset/workflow/ArcanistPromptsWorkflow.php:  public function runWorkflow() {
src/toolset/workflow/ArcanistShellCompleteWorkflow.php:  public function runWorkflow() {
src/phage/workflow/PhageExecWorkflow.php:  protected function runWorkflow() {
src/workflow/ArcanistLandWorkflow.php:  public function runWorkflow() {
src/workflow/ArcanistDownloadWorkflow.php:  public function runWorkflow() {
src/workflow/ArcanistAnoidWorkflow.php:  public function runWorkflow() {
src/workflow/ArcanistLookWorkflow.php:  public function runWorkflow() {
src/workflow/ArcanistPasteWorkflow.php:  public function runWorkflow() {
src/workflow/ArcanistUpgradeWorkflow.php:  public function runWorkflow() {
src/workflow/ArcanistWorkflow.php:  protected function runWorkflowCleanup() {
src/workflow/ArcanistCallConduitWorkflow.php:  public function runWorkflow() {
src/workflow/ArcanistMarkersWorkflow.php:  public function runWorkflow() {
src/workflow/ArcanistUploadWorkflow.php:  public function runWorkflow() {
src/workflow/ArcanistLiberateWorkflow.php:  public function runWorkflow() {
src/workflow/ArcanistWeldWorkflow.php:  public function runWorkflow() {
src/workflow/ArcanistInspectWorkflow.php:  public function runWorkflow() {
src/workflow/ArcanistAmendWorkflow.php:  public function runWorkflow() {
src/workflow/ArcanistWorkWorkflow.php:  public function runWorkflow() {
src/browse/workflow/ArcanistBrowseWorkflow.php:  public function runWorkflow() {

ArcanistVersionWorkflow: not nice that runWorkflow() does not return 0 on success
ArcanistAliasWorkflow: ✅ it seems it returns 0 or 1 on success or error
ArcanistPromptsWorkflow: ✅ same
ArcanistShellCompleteWorkflow.php: it seems it does not cover all return cases to me
....

So I think your edit is nice to “to be broad about what is accepted” but then these logics should be "strict about what I provide".

Please wait a week to attract more eyeballs and comments before running arc land. Thanks :)

This revision is now accepted and ready to land.Thu, Feb 13, 11:28

valerio.bozzolan retitled this revision from Fix PHP 8 exit status cannot be null error to Fix PHP 8 exit status cannot be null error in PhutilArgumentParser.Tue, Feb 18, 06:47

valerio.bozzolan mentioned this in D25888: PHP 8.4: Do not pass null to exit() in PhutilArgumentParser.

aklapper edited the summary of this revision. (Show Details)Tue, Feb 18, 17:05

aklapper added a task: T15990: ERROR 8192: exit(): Passing null to parameter #1 ($status) of type string|int is deprecated at [arcanist/src/parser/argument/PhutilArgumentParser.php:494] on PHP 8.4.