Page MenuHomePhabricator

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

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

Details

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

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.Wed, Jul 21, 5:17 PM
src/future/exec/PhutilExecPassthru.php
34

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.