Add missing ArcanistWorkflow::runWorkflow method
Changes PlannedPublic
Actions

Authored by amybones on Fri, May 30, 23:37.

Details

Reviewers

aklapper

Group Reviewers

O1: Blessed Committers

Summary

No definition of runWorkflow existed in ArcanistWorkflow, now it does and
now phpStan is happier. This also needed to correct the visibility of the phage
workflow.

Test Plan

See no more type errors.

Diff Detail

Repository

rARC Arcanist

Branch

arc_workflow_fixes (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 2030
Build 2030: arc lint + arc unit

Event Timeline

amybones created this revision.Fri, May 30, 23:37

Owners added a reviewer: O1: Blessed Committers.Fri, May 30, 23:37

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptFri, May 30, 23:37

amybones requested review of this revision.Fri, May 30, 23:37

Harbormaster completed remote builds in B2029: Diff 3120.Fri, May 30, 23:37

Get rid of coverage errors :)

Harbormaster completed remote builds in B2030: Diff 3121.Fri, May 30, 23:40

amybones added a child revision: D26041: Fix ArcanistWorkflow::shouldRequireCleanUntrackedFiles.Fri, May 30, 23:49

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

Running phpstan analyse, before this patch I get Call to an undefined method ArcanistWorkflow::runWorkflow(), after this patch I get Method ArcanistWorkflow::runWorkflow() invoked with 1 parameter, 0 required, both in src/workflow/ArcanistWorkflow.php:227. That looks like an improvement indeed.

In D26040#27814, @valerio.bozzolan wrote:

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

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

This revision is now accepted and ready to land.Sat, May 31, 11:56

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.

In D26040#27816, @aklapper wrote:

Running phpstan analyse, [...] Method ArcanistWorkflow::runWorkflow() invoked with 1 parameter, 0 required, both in src/workflow/ArcanistWorkflow.php:227. That looks like an improvement indeed.

Oh, I completely missed that error when I ran phpStan myself. I think probably it makes sense to update this diff to add that argument everywhere. Since that's the case, I retract my assessment about converting things to protected and will do that as well 🙂