Deprecate "PhutilExecPassthru->execute()" in favor of "resolve()"
ClosedPublic
Actions

Authored by epriestley on Jul 21 2021, 5:11 PM.

Details

Reviewers

cspeckmim

Maniphest Tasks

T13660: Deprecate "PhutilExecPassthru->execute()" in favor of "PhutilExecPassthru->resolve()"

Commits

rARC8bb7d5889050: Deprecate "PhutilExecPassthru->execute()" in favor of "resolve()"

Summary

Fixes T13660. See also D21703. The most desirable modern API here is "resolve()", so deprecate the similar "execute()".

Test Plan

Grepped for callsites.
Ran arc patch --trace in a Git working copy and saw the updated "git apply" in the trace output.
Used this test script (changing the method and the command invoked) to confirm that success and error behavior is identical in "resolve()" and "execute()", except that "execute()" now emits a deprecation warning:

<?php

require_once 'support/init/init-script.php';


$err = id(new PhutilExecPassthru('lsx'))->execute();
var_dump($err);

Diff Detail

Repository

rARC Arcanist

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Jul 21 2021, 5:11 PM

Harbormaster completed remote builds in B25494: Diff 51691.Jul 21 2021, 5:12 PM

epriestley requested review of this revision.Jul 21 2021, 5:12 PM

epriestley edited the test plan for this revision. (Show Details)Jul 21 2021, 5:12 PM

cspeckmim accepted this revision.Jul 21 2021, 5:17 PM

cspeckmim added inline comments.

src/future/exec/PhutilExecPassthru.php
13–14	lol
34	If this is called from Phabricator then this would go into server logs, but where do logs go on Arcanist? The console?

This revision is now accepted and ready to land.Jul 21 2021, 5:17 PM

epriestley added inline comments.Jul 21 2021, 5:20 PM

src/future/exec/PhutilExecPassthru.php

Yeah, here's test.php running ls with execute():

$ php -f test.php                         
[2021-07-21 10:01:26] PHLOG: 'The "execute()" method of "PhutilExecPassthru" is deprecated and calls should be replaced with "resolve()". See T13660.' at [/Users/epriestley/dev/core/lib/arcanist/src/future/exec/PhutilExecPassthru.php:36]
LICENSE		NOTICE		README.md	bin		externals	resources	scripts		src		support		test.php
int(0)

It seems pretty unlikely that anyone is calling execute() and I'm usually not too dainty about breaking almost-certainly-only-internal APIs, but it was in the docblock.

Closed by commit rARC8bb7d5889050: Deprecate "PhutilExecPassthru->execute()" in favor of "resolve()" (authored by epriestley). · Explain WhyJul 21 2021, 5:21 PM

This revision was automatically updated to reflect the committed changes.

epriestley added a commit: rARC8bb7d5889050: Deprecate "PhutilExecPassthru->execute()" in favor of "resolve()".