Improved static analysis for proxied workflows and activities

[mtorromeo]

What was changed

Added multiple type parameters to stub/proxy classes, with templated ReturnType for workflow methods and UpdateHandle, and used @mixin instead of lying about the return type of proxy instances.

Why?

This fixes multiple type inference issues that I encountered when using this library. I am using PHPStan instead of psalm but every annotation involved is supported by both analyzers.

Checklist

1. How was this tested:

I have been using a patched version with these changes for a few months in a project that is in production without any issues. This does not touch any actual php code anyway, just doc comments for the static analyzers.

2. Any docs updates needed?

Nothing needs to be updated.

[vercel]

@mtorromeo is attempting to deploy a commit to the Temporal Team on Vercel.

A member of the Team first needs to authorize it.

[xepozz]

PR:

Master:

Do we really want to expose internal methods?

[mtorromeo]

Do we really want to expose internal methods?

This is not really about exposing internal methods in my opinion. Not declaring the correct class leads to a weird experience for the final user.

For example, let's say that $child, from your example, could also be assigned a proxy for a different workflow, so static analysis would infer it to be TestWorkflow | OtherWorkflow. Then if I did this I would assume to be able to distinguish between the 2 classes while in fact $child is neither of those:

if ($child instanceof TestWorkflow) {
   // ...
} elseif ($child instanceof OtherWorkflow) {
   // ...
}
// $child is neither

I don't think lying about a returned type is a good idea.

I would also appreciate if my IDE helped me distinguish between a proxied method of my workflow and an internal method. Something that a proper declaration would help with.

[xepozz]

I would assume to be able to distinguish between the 2 classes

Are there real examples?

[mtorromeo]

The real example is a bit long and I cannot share it, but I can try to describe it.

The app has an endpoint that is used to introspect the status of a workflow by its id.

I retrieve the execution with something like this:

$executions = $temporal->listWorkflowExecutions(sprintf('WorkflowId = %s', json_encode($id)));

Then I have a way to map $execution->type->name to its class/interface, let's say I do something like this (simplified):

$class = "MyWorkflows\\{$execution->type->name}";

Some workflows also implement have some #[QueryMethod] that I can call to retrieve additional information. Let's say that I was going to check if I can call some method like this:

$stub = $temporal->newRunningWorkflowStub($class, $id);
if ($stub instanceof WorkflowWithInfo) {
  $info = $stub->getInfo();
}

The condition here is never true because newRunningWorkflowStub returns a Proxy class but this is not apparent from its declaration. With the proposed patch PHPStan (and I guess also Psalm) immediately reports that the condition is always false.

There are also other situations where some issues would not be reported.

Let's say that I try to organize my code with some utility methods such as this (again simplified):

function getWorkflowInfo(WorkflowWithInfo $wf) {
  return $wf->getInfo();
}

// somewhere else
$info = getWorkflowInfo($stub);

I would get no errors from PHPStan, and the phpdoc documentation suggests that I am writing correct code, but it results in runtime errors because I am passing a Proxy class to a function that expects "WorkflowWithInfo".

Basically static analysis is completely broken and it also pushes the developer in writing incorrect code.

If I am being honest, I do not understand why this would require an illustration of a specific use case because I think the current declaration is simply wrong and needs to be corrected.

[xepozz]

I agree that runtime type mismatch is far from the most pleasant thing.

[wolfy-j]

why it's final now? or why it wasn't final before?

[xepozz]

It's internal class, so it's safe to make it final
I'm not sure about the past, but it seems to me there's no extension points, that's why I made it final.

[wolfy-j]

looks good to me