Skip to content

fix: incorrect updateWithStart signature#594

Merged
roxblnfk merged 5 commits into
temporalio:masterfrom
DanielsStugis:fix-client-calls-interceptor
May 5, 2025
Merged

fix: incorrect updateWithStart signature#594
roxblnfk merged 5 commits into
temporalio:masterfrom
DanielsStugis:fix-client-calls-interceptor

Conversation

@DanielsStugis

@DanielsStugis DanielsStugis commented Apr 11, 2025

Copy link
Copy Markdown
Contributor

What was changed

  • Fixed WorkflowClientCallsInterceptor::updateWithStart() signature to match value returned from WorkflowStarter::updateWithStart()
  • Added test case to cover fixed area.

Why?

Because whenever WorkflowClientCallsInterceptor implementation is provided and updateWithStart() is called exception happens.
Example output, from the provided test case, if signature change is reverted

TypeError: Temporal\Tests\Interceptor\InterceptorCallsCounter::updateWithStart(): Return value must be of type Temporal\Client\Update\UpdateHandle, array returned

Checklist

  1. Closes nothing (did not find any related issue)

  2. How was this tested:
    See the added test case

  3. Any docs updates needed?
    No

@vercel

vercel Bot commented Apr 11, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
php ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 1, 2025 2:44pm

@CLAassistant

CLAassistant commented Apr 11, 2025

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@roxblnfk roxblnfk added the Bug Something isn't working label Apr 11, 2025

@roxblnfk roxblnfk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't notice that my review was in pending status.

* @see WorkflowClientCallsInterceptor::updateWithStart()
*/
public function updateWithStart(UpdateWithStartInput $input, callable $next): UpdateHandle
public function updateWithStart(UpdateWithStartInput $input, callable $next): array

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you replace the array with UpdateWithStartOutput DTO?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, hopefully managed to cover all the relevant places for it.
Also that exception returning makes things messy, considered making separate properties for $handle and $exception, but then we would have nullable values and ugly checks for those

@roxblnfk roxblnfk merged commit 8ede13d into temporalio:master May 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants