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

arcpatch-D25869

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 1731
Build 1731: arc lint + arc unit

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.