From 4391acd575c62cac1c7af6f040c473f61a707fc5 Mon Sep 17 00:00:00 2001 From: Dmitriy Derepko Date: Tue, 28 Jul 2026 10:37:40 +0400 Subject: [PATCH 1/2] fix: merge method-level #[MethodRetry] into ActivityOptions retry options The mergeWith() guard on ActivityOptions, LocalActivityOptions, ChildWorkflowOptions and WorkflowOptions was inverted: it only ran the merge when retryOptions was still at its default (null), where null?->mergeWith() collapsed to null, and skipped the merge whenever the user had set retry options. As a result #[MethodRetry] was ignored when RetryOptions were present and lost (or fatal on WorkflowOptions) when they were absent. Delegate precedence to RetryOptions::mergeWith(), which already keeps user-set fields and fills defaults from the attribute, and fall back to a fresh RetryOptions when none is set. Closes #777 --- src/Activity/ActivityOptions.php | 4 +- src/Activity/LocalActivityOptions.php | 4 +- src/Client/WorkflowOptions.php | 4 +- src/Workflow/ChildWorkflowOptions.php | 4 +- tests/Unit/DTO/ActivityOptionsTestCase.php | 44 +++++++++++++++ .../Unit/DTO/ChildWorkflowOptionsTestCase.php | 30 ++++++++++ .../Unit/DTO/LocalActivityOptionsTestCase.php | 55 +++++++++++++++++++ tests/Unit/DTO/WorkflowOptionsTestCase.php | 29 ++++++++++ 8 files changed, 166 insertions(+), 8 deletions(-) create mode 100644 tests/Unit/DTO/LocalActivityOptionsTestCase.php diff --git a/src/Activity/ActivityOptions.php b/src/Activity/ActivityOptions.php index 3274ae2af..906174d3f 100644 --- a/src/Activity/ActivityOptions.php +++ b/src/Activity/ActivityOptions.php @@ -153,8 +153,8 @@ public function mergeWith(?MethodRetry $retry = null): self { $self = clone $this; - if ($retry !== null && $this->diff->isPresent($self, 'retryOptions')) { - $self->retryOptions = $this->retryOptions?->mergeWith($retry); + if ($retry !== null) { + $self->retryOptions = ($self->retryOptions ?? RetryOptions::new())->mergeWith($retry); } return $self; diff --git a/src/Activity/LocalActivityOptions.php b/src/Activity/LocalActivityOptions.php index dd722db39..dd609f998 100644 --- a/src/Activity/LocalActivityOptions.php +++ b/src/Activity/LocalActivityOptions.php @@ -93,8 +93,8 @@ public function mergeWith(?MethodRetry $retry = null): self { $self = clone $this; - if ($retry !== null && $this->diff->isPresent($self, 'retryOptions')) { - $self->retryOptions = $this->retryOptions?->mergeWith($retry); + if ($retry !== null) { + $self->retryOptions = ($self->retryOptions ?? RetryOptions::new())->mergeWith($retry); } return $self; diff --git a/src/Client/WorkflowOptions.php b/src/Client/WorkflowOptions.php index 027f2c522..efad3a864 100644 --- a/src/Client/WorkflowOptions.php +++ b/src/Client/WorkflowOptions.php @@ -215,8 +215,8 @@ public function mergeWith(?MethodRetry $retry = null, ?CronSchedule $cron = null { $self = clone $this; - if ($retry !== null && $self->diff->isPresent($self, 'retryOptions')) { - $self->retryOptions = $self->retryOptions->mergeWith($retry); + if ($retry !== null) { + $self->retryOptions = ($self->retryOptions ?? RetryOptions::new())->mergeWith($retry); } if ($cron !== null && $self->diff->isPresent($self, 'cronSchedule')) { diff --git a/src/Workflow/ChildWorkflowOptions.php b/src/Workflow/ChildWorkflowOptions.php index d30c21d0b..3e7c0c099 100644 --- a/src/Workflow/ChildWorkflowOptions.php +++ b/src/Workflow/ChildWorkflowOptions.php @@ -210,8 +210,8 @@ public function mergeWith(?MethodRetry $retry = null, ?CronSchedule $cron = null { $self = clone $this; - if ($retry !== null && $self->diff->isPresent($self, 'retryOptions')) { - $self->retryOptions = $self->retryOptions?->mergeWith($retry); + if ($retry !== null) { + $self->retryOptions = ($self->retryOptions ?? RetryOptions::new())->mergeWith($retry); } if ($cron !== null && $self->diff->isPresent($self, 'cronSchedule')) { diff --git a/tests/Unit/DTO/ActivityOptionsTestCase.php b/tests/Unit/DTO/ActivityOptionsTestCase.php index 5bd3bbf15..c38b3125b 100644 --- a/tests/Unit/DTO/ActivityOptionsTestCase.php +++ b/tests/Unit/DTO/ActivityOptionsTestCase.php @@ -14,6 +14,7 @@ use Carbon\CarbonInterval; use Temporal\Activity\ActivityCancellationType; use Temporal\Activity\ActivityOptions; +use Temporal\Common\MethodRetry; use Temporal\Common\RetryOptions; use Temporal\Common\Uuid; @@ -124,4 +125,47 @@ public function testRetryOptionsChangesNotMutateState(): void RetryOptions::new() )); } + + public function testMergeWithMethodRetryFillsDefaultRetryOptions(): void + { + $dto = ActivityOptions::new() + ->withRetryOptions(RetryOptions::new()) + ->mergeWith(new MethodRetry(maximumAttempts: 5)); + + $this->assertSame(5, $dto->retryOptions->maximumAttempts); + } + + public function testMergeWithMethodRetryCreatesRetryOptionsWhenNull(): void + { + $dto = ActivityOptions::new()->mergeWith(new MethodRetry(maximumAttempts: 5)); + + $this->assertNotNull($dto->retryOptions); + $this->assertSame(5, $dto->retryOptions->maximumAttempts); + } + + public function testMergeWithMethodRetryKeepsUserDefinedFields(): void + { + $methodRetry = new MethodRetry(maximumAttempts: 5, maximumInterval: 30); + $dto = ActivityOptions::new() + ->withRetryOptions(RetryOptions::new()->withMaximumAttempts(1)) + ->mergeWith($methodRetry); + + $this->assertSame(1, $dto->retryOptions->maximumAttempts); + $this->assertSame($methodRetry->maximumInterval, $dto->retryOptions->maximumInterval); + } + + public function testMergeWithNullRetryDoesNotChangeRetryOptions(): void + { + $retry = RetryOptions::new()->withMaximumAttempts(7); + $dto = ActivityOptions::new()->withRetryOptions($retry)->mergeWith(null); + + $this->assertSame(7, $dto->retryOptions->maximumAttempts); + } + + public function testMergeWithDoesNotMutateState(): void + { + $dto = new ActivityOptions(); + + $this->assertNotSame($dto, $dto->mergeWith(new MethodRetry(maximumAttempts: 5))); + } } diff --git a/tests/Unit/DTO/ChildWorkflowOptionsTestCase.php b/tests/Unit/DTO/ChildWorkflowOptionsTestCase.php index f22d1315b..37a709fd6 100644 --- a/tests/Unit/DTO/ChildWorkflowOptionsTestCase.php +++ b/tests/Unit/DTO/ChildWorkflowOptionsTestCase.php @@ -12,6 +12,8 @@ namespace Temporal\Tests\Unit\DTO; use Temporal\Common\IdReusePolicy; +use Temporal\Common\MethodRetry; +use Temporal\Common\RetryOptions; use Temporal\Workflow\ChildWorkflowCancellationType; use Temporal\Workflow\ChildWorkflowOptions; use Temporal\Workflow\ParentClosePolicy; @@ -107,4 +109,32 @@ public function testChildWorkflowCancellationTypeChangesNotMutateStateUsingEnum( )); $this->assertSame(ChildWorkflowCancellationType::TryCancel->value, $dto->cancellationType); } + + public function testMergeWithMethodRetryFillsDefaultRetryOptions(): void + { + $dto = ChildWorkflowOptions::new() + ->withRetryOptions(RetryOptions::new()) + ->mergeWith(new MethodRetry(maximumAttempts: 5)); + + $this->assertSame(5, $dto->retryOptions->maximumAttempts); + } + + public function testMergeWithMethodRetryCreatesRetryOptionsWhenNull(): void + { + $dto = ChildWorkflowOptions::new()->mergeWith(new MethodRetry(maximumAttempts: 5)); + + $this->assertNotNull($dto->retryOptions); + $this->assertSame(5, $dto->retryOptions->maximumAttempts); + } + + public function testMergeWithMethodRetryKeepsUserDefinedFields(): void + { + $methodRetry = new MethodRetry(maximumAttempts: 5, maximumInterval: 30); + $dto = ChildWorkflowOptions::new() + ->withRetryOptions(RetryOptions::new()->withMaximumAttempts(1)) + ->mergeWith($methodRetry); + + $this->assertSame(1, $dto->retryOptions->maximumAttempts); + $this->assertSame($methodRetry->maximumInterval, $dto->retryOptions->maximumInterval); + } } diff --git a/tests/Unit/DTO/LocalActivityOptionsTestCase.php b/tests/Unit/DTO/LocalActivityOptionsTestCase.php new file mode 100644 index 000000000..26f80a19c --- /dev/null +++ b/tests/Unit/DTO/LocalActivityOptionsTestCase.php @@ -0,0 +1,55 @@ +withRetryOptions(RetryOptions::new()) + ->mergeWith(new MethodRetry(maximumAttempts: 5)); + + $this->assertSame(5, $dto->retryOptions->maximumAttempts); + } + + public function testMergeWithMethodRetryCreatesRetryOptionsWhenNull(): void + { + $dto = LocalActivityOptions::new()->mergeWith(new MethodRetry(maximumAttempts: 5)); + + $this->assertNotNull($dto->retryOptions); + $this->assertSame(5, $dto->retryOptions->maximumAttempts); + } + + public function testMergeWithMethodRetryKeepsUserDefinedFields(): void + { + $methodRetry = new MethodRetry(maximumAttempts: 5, maximumInterval: 30); + $dto = LocalActivityOptions::new() + ->withRetryOptions(RetryOptions::new()->withMaximumAttempts(1)) + ->mergeWith($methodRetry); + + $this->assertSame(1, $dto->retryOptions->maximumAttempts); + $this->assertSame($methodRetry->maximumInterval, $dto->retryOptions->maximumInterval); + } + + public function testMergeWithNullRetryDoesNotChangeRetryOptions(): void + { + $retry = RetryOptions::new()->withMaximumAttempts(7); + $dto = LocalActivityOptions::new()->withRetryOptions($retry)->mergeWith(null); + + $this->assertSame(7, $dto->retryOptions->maximumAttempts); + } +} diff --git a/tests/Unit/DTO/WorkflowOptionsTestCase.php b/tests/Unit/DTO/WorkflowOptionsTestCase.php index 2d6bde9b2..86057164e 100644 --- a/tests/Unit/DTO/WorkflowOptionsTestCase.php +++ b/tests/Unit/DTO/WorkflowOptionsTestCase.php @@ -15,6 +15,7 @@ use Temporal\Api\Common\V1\SearchAttributes; use Temporal\Client\WorkflowOptions; use Temporal\Common\IdReusePolicy; +use Temporal\Common\MethodRetry; use Temporal\Common\RetryOptions; use Temporal\Common\TypedSearchAttributes; use Temporal\Common\Uuid; @@ -242,4 +243,32 @@ public function testSetTypedSearchAttributesCasting(): void $this->assertInstanceOf(SearchAttributes::class, $result); $this->assertCount(1, $result->getIndexedFields()); } + + public function testMergeWithMethodRetryFillsDefaultRetryOptions(): void + { + $dto = WorkflowOptions::new() + ->withRetryOptions(RetryOptions::new()) + ->mergeWith(new MethodRetry(maximumAttempts: 5)); + + $this->assertSame(5, $dto->retryOptions->maximumAttempts); + } + + public function testMergeWithMethodRetryCreatesRetryOptionsWhenNull(): void + { + $dto = WorkflowOptions::new()->mergeWith(new MethodRetry(maximumAttempts: 5)); + + $this->assertNotNull($dto->retryOptions); + $this->assertSame(5, $dto->retryOptions->maximumAttempts); + } + + public function testMergeWithMethodRetryKeepsUserDefinedFields(): void + { + $methodRetry = new MethodRetry(maximumAttempts: 5, maximumInterval: 30); + $dto = WorkflowOptions::new() + ->withRetryOptions(RetryOptions::new()->withMaximumAttempts(1)) + ->mergeWith($methodRetry); + + $this->assertSame(1, $dto->retryOptions->maximumAttempts); + $this->assertSame($methodRetry->maximumInterval, $dto->retryOptions->maximumInterval); + } } From 1247a4d39e4115e94776cc4e0a21a71835764fac Mon Sep 17 00:00:00 2001 From: Dmitriy Derepko Date: Tue, 28 Jul 2026 11:07:25 +0400 Subject: [PATCH 2/2] chore: drop obsolete psalm baseline entry for WorkflowOptions::mergeWith The PossiblyNullReference on WorkflowOptions::mergeWith is no longer raised now that the null-safe fallback (?? RetryOptions::new()) replaced the unguarded $self->retryOptions->mergeWith() call, so the baseline entry became an UnusedBaselineEntry error on the 8.3 static-analysis job. --- psalm-baseline.xml | 3 --- 1 file changed, 3 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index c40a8896c..511fbe21a 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -160,9 +160,6 @@ - - -