(This seems nice) Small question: Does it still work for you keeping the method as protected instead of public?

In D26040#27816, @aklapper wrote:

Keeping it protected will throw Protected method PhageExecWorkflow::runWorkflow() overriding public method ArcanistWorkflow::runWorkflow() should also be public.

Exactly what I mean: this patch introduced a public method, but what about just introducing a protected method? and rollback src/phage/workflow/PhageExecWorkflow.php

In D26040#27819, @valerio.bozzolan wrote:

In D26040#27816, @aklapper wrote:

Keeping it protected will throw Protected method PhageExecWorkflow::runWorkflow() overriding public method ArcanistWorkflow::runWorkflow() should also be public.

Exactly what I mean: this patch introduced a public method, but what about just introducing a protected method? and rollback src/phage/workflow/PhageExecWorkflow.php

Hmm.. I'm not totally sure that it makes sense to convert things to protected. Because phage is the only one defining the method as protected. An example
is here ArcanistLiberateWorkflow. I'm happy to switch everything over to protected but that's a larger change that will require more validation.

(You are soo fast woow)

Thanks to your last change you highlighted that (apparently) nothing reads that argument runWorkflow(PhutilArgumentParser $args).

Checked from master:

grep -RF -- '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:      $err = $this->runWorkflow($args);
./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() {

Given this situation, and if you kindly confirm since you explored this better than us lol, it seems to me that the root problem could be in the caller of ->runWorkflow($args), in fact see the first and the last highlighted lines:

https://we.phorge.it/source/arcanist/browse/master/src/workflow/ArcanistWorkflow.php;c739de1ef2f5598d34e0f02f046ea040a33133e7$221-227

So, premising that children classes are (probably) just reading these arguments by doing $this->getArgument() - that works thanks to line 221 already highlighted,

So:

• keeping the method as protected
• fixing the public methods, making them protected
• ArcanistWorkflow.php: editing the new definition to be just runWorkflow() without arguments
• ArcanistWorkflow.php: editing the caller to be ->runWorkflow()
• restoring the introduction of the new argument where missing

What do you think about?

Additionally, this suggestion causes less code to be touched

(about my last phrase I just mean we can have everywhere protected function runWorkflow() and we can avoid to introduce the argument - so for example we could rollback the file PhageExecWorkflow.php 🤯 🤯 🤯 lol )

I personally think having the arguments passed into runWorkflow makes a bit more organizational sense (in that I have a horrible functional programming streak that says use arguments for everything lmao), but it does somewhat leak an abstraction by committing more to a particular argument parsing interface, which looks to have been part of the problem with the previous approach (using an array for arguments).

So, I'm happy to rollback adding the argument (and this time I'll wait a little longer before doing it hahahah!) However, I will say that it won't actually touch that much less code, since basically every method except PhageExecWorkflow was a public method rather than protected.

I'm not sure what you mean by:

In D26040#28043, @valerio.bozzolan wrote:

[..snip..]

• restoring the introduction of the new argument where missing

Can you clarify?