From a95e07530d6f6c837422c926e570a32a362af733 Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Fri, 7 Aug 2026 13:35:03 +0530 Subject: [PATCH 1/2] refactor(vcs): report the events a delivery describes A delivery was read as one event, which holds for every provider here but not for all of them: a Bitbucket push reports each ref it touched in a single delivery, and only the first would have been seen. getEvents() replaces getEvent() and hands back the events a delivery describes, empty when it describes none. Providers that carry one event per delivery return the one they parsed, so what callers read is unchanged beyond the list around it. --- src/VCS/Adapter.php | 9 ++++++--- src/VCS/Adapter/Git/GitHub.php | 16 ++++++++-------- src/VCS/Adapter/Git/GitLab.php | 10 +++++----- src/VCS/Adapter/Git/Gitea.php | 10 +++++----- tests/VCS/Adapter/GitHubTest.php | 2 +- tests/VCS/Adapter/GitLabTest.php | 4 ++-- tests/VCS/Base.php | 22 +++++++++++----------- 7 files changed, 38 insertions(+), 35 deletions(-) diff --git a/src/VCS/Adapter.php b/src/VCS/Adapter.php index 367c068d..7a0f0e32 100644 --- a/src/VCS/Adapter.php +++ b/src/VCS/Adapter.php @@ -216,13 +216,16 @@ public function validateWebhookEvent(string $payload, string $signature, string } /** - * Parses webhook event payload + * Parses a webhook delivery into the events it describes. + * + * A delivery usually describes one event, but some providers batch + * several into one -- Bitbucket reports every ref a push touched. * * @param string $event Type of event: push, pull_request etc * @param string $payload The webhook payload received from Git provider - * @return array Parsed payload as a json object + * @return array> Parsed payloads as json objects */ - abstract public function getEvent(string $event, string $payload): array; + abstract public function getEvents(string $event, string $payload): array; /** * HTTP header name carrying the webhook event type (e.g. 'x-github-event'). diff --git a/src/VCS/Adapter/Git/GitHub.php b/src/VCS/Adapter/Git/GitHub.php index 6f052061..3d9cc2f0 100644 --- a/src/VCS/Adapter/Git/GitHub.php +++ b/src/VCS/Adapter/Git/GitHub.php @@ -1291,9 +1291,9 @@ public function getFileUrl(string $owner, string $repositoryName, string $refere * * @param string $event Type of event: push, pull_request etc * @param string $payload The webhook payload received from GitHub - * @return array Parsed payload as a json object + * @return array> Parsed payloads as json objects */ - public function getEvent(string $event, string $payload): array + public function getEvents(string $event, string $payload): array { $payload = json_decode($payload, true); @@ -1344,7 +1344,7 @@ public function getEvent(string $event, string $payload): array } } - return [ + return [[ 'branchCreated' => $branchCreated, 'branchDeleted' => $branchDeleted, 'branch' => $branch, @@ -1365,7 +1365,7 @@ public function getEvent(string $event, string $payload): array 'pullRequestNumber' => '', 'action' => '', 'affectedFiles' => \array_keys($affectedFiles), - ]; + ]]; case 'pull_request': $payloadRepository = $payload['repository'] ?? []; $payloadRepositoryOwner = $payloadRepository['owner'] ?? []; @@ -1393,7 +1393,7 @@ public function getEvent(string $event, string $payload): array $baseLogin = $payloadPullRequestBaseUser['login'] ?? ''; $external = $headLogin !== $baseLogin; - return [ + return [[ 'branch' => $branch, 'branchUrl' => $branchUrl, 'repositoryId' => $repositoryId, @@ -1408,7 +1408,7 @@ public function getEvent(string $event, string $payload): array 'external' => $external, 'pullRequestNumber' => $pullRequestNumber, 'action' => $action, - ]; + ]]; case 'installation': case 'installation_repositories': $payloadInstallation = $payload['installation'] ?? []; @@ -1417,11 +1417,11 @@ public function getEvent(string $event, string $payload): array $action = $payload['action'] ?? ''; $userName = $payloadInstallationAccount['login'] ?? ''; - return [ + return [[ 'action' => $action, 'installationId' => $installationId, 'userName' => $userName, - ]; + ]]; } return []; diff --git a/src/VCS/Adapter/Git/GitLab.php b/src/VCS/Adapter/Git/GitLab.php index 34416911..6d25871b 100644 --- a/src/VCS/Adapter/Git/GitLab.php +++ b/src/VCS/Adapter/Git/GitLab.php @@ -1042,7 +1042,7 @@ public function generateCloneCommand(string $owner, string $repositoryName, stri 'merge' => 'closed', ]; - public function getEvent(string $event, string $payload): array + public function getEvents(string $event, string $payload): array { $payloadArray = json_decode($payload, true); if ($payloadArray === null || !is_array($payloadArray)) { @@ -1082,7 +1082,7 @@ public function getEvent(string $event, string $payload): array $allZeroSha = str_repeat('0', 40); - return [ + return [[ 'branchCreated' => ($payloadArray['before'] ?? '') === $allZeroSha, 'branchDeleted' => ($payloadArray['after'] ?? '') === $allZeroSha, 'branch' => $branch, @@ -1103,7 +1103,7 @@ public function getEvent(string $event, string $payload): array 'pullRequestNumber' => '', 'action' => '', 'affectedFiles' => \array_keys($affectedFiles), - ]; + ]]; case 'Merge Request Hook': $project = $payloadArray['project'] ?? []; @@ -1121,7 +1121,7 @@ public function getEvent(string $event, string $payload): array $external = isset($mr['source_project_id'], $mr['target_project_id']) && $mr['source_project_id'] !== $mr['target_project_id']; - return [ + return [[ 'branch' => $branch, 'branchUrl' => $branchUrl, 'repositoryId' => $repositoryId, @@ -1136,7 +1136,7 @@ public function getEvent(string $event, string $payload): array 'external' => $external, 'pullRequestNumber' => $mr['iid'] ?? '', 'action' => $action, - ]; + ]]; default: return []; diff --git a/src/VCS/Adapter/Git/Gitea.php b/src/VCS/Adapter/Git/Gitea.php index 8d630dad..d5d7ab18 100644 --- a/src/VCS/Adapter/Git/Gitea.php +++ b/src/VCS/Adapter/Git/Gitea.php @@ -1119,7 +1119,7 @@ public function generateCloneCommand(string $owner, string $repositoryName, stri * @param string $payload The webhook payload received from Gitea * @return array Parsed payload as an array */ - public function getEvent(string $event, string $payload): array + public function getEvents(string $event, string $payload): array { $payload = json_decode($payload, true); @@ -1166,7 +1166,7 @@ public function getEvent(string $event, string $payload): array } } - return [ + return [[ 'branchCreated' => $branchCreated, 'branchDeleted' => $branchDeleted, 'branch' => $branch, @@ -1187,7 +1187,7 @@ public function getEvent(string $event, string $payload): array 'pullRequestNumber' => '', 'action' => '', 'affectedFiles' => \array_keys($affectedFiles), - ]; + ]]; case 'pull_request': $payloadRepository = $payload['repository'] ?? []; @@ -1217,7 +1217,7 @@ public function getEvent(string $event, string $payload): array $baseRepoFullName = $payloadRepository['full_name'] ?? ''; $external = !empty($headRepoFullName) && !empty($baseRepoFullName) && $headRepoFullName !== $baseRepoFullName; - return [ + return [[ 'branch' => $branch, 'branchUrl' => $branchUrl, 'repositoryId' => $repositoryId, @@ -1232,7 +1232,7 @@ public function getEvent(string $event, string $payload): array 'external' => $external, 'pullRequestNumber' => $pullRequestNumber, 'action' => $action, - ]; + ]]; } return []; diff --git a/tests/VCS/Adapter/GitHubTest.php b/tests/VCS/Adapter/GitHubTest.php index 17d57374..bd29d117 100644 --- a/tests/VCS/Adapter/GitHubTest.php +++ b/tests/VCS/Adapter/GitHubTest.php @@ -145,7 +145,7 @@ public function testGetEventInstallation(): void $this->fail('Failed to encode JSON payload'); } - $result = $this->vcsAdapter->getEvent('installation', $payload); + $result = $this->vcsAdapter->getEvents('installation', $payload)[0]; $this->assertSame('deleted', $result['action']); $this->assertSame('1234', $result['installationId']); diff --git a/tests/VCS/Adapter/GitLabTest.php b/tests/VCS/Adapter/GitLabTest.php index ec655a36..6f41afe2 100644 --- a/tests/VCS/Adapter/GitLabTest.php +++ b/tests/VCS/Adapter/GitLabTest.php @@ -159,7 +159,7 @@ public function testGetEventPushMatchesCheckoutSha(): void $this->fail('Failed to encode JSON payload'); } - $result = $this->vcsAdapter->getEvent('Push Hook', $payload); + $result = $this->vcsAdapter->getEvents('Push Hook', $payload)[0]; $this->assertIsArray($result); $this->assertSame('def456', $result['commitHash']); @@ -181,7 +181,7 @@ public function testGetEventPullRequestActionMapping(): void $this->fail('Failed to encode JSON payload'); } - $result = $this->vcsAdapter->getEvent('Merge Request Hook', $payload); + $result = $this->vcsAdapter->getEvents('Merge Request Hook', $payload)[0]; $this->assertSame($mapped, $result['action'], "native action '{$native}' should map to '{$mapped}'"); } } diff --git a/tests/VCS/Base.php b/tests/VCS/Base.php index 032585ff..59345b8c 100644 --- a/tests/VCS/Base.php +++ b/tests/VCS/Base.php @@ -1556,7 +1556,7 @@ protected function awaitWebhook(string $eventName, string $secret): array 'Webhook signature did not validate' ); - return $this->vcsAdapter->getEvent($eventName, $payload); + return $this->vcsAdapter->getEvents($eventName, $payload)[0] ?? []; } public function testValidateWebhookEvent(): void @@ -2312,10 +2312,10 @@ public function testGetRepositoryAfterDeleteFails(): void public function testGetEventPush(): void { - $result = $this->vcsAdapter->getEvent( + $result = $this->vcsAdapter->getEvents( static::$pushEventName, $this->pushPayload(static::$defaultBranch, ['file1.txt'], ['file2.txt'], ['file3.txt']) - ); + )[0]; $this->assertSame(static::$defaultBranch, $result['branch']); $this->assertSame(self::EVENT_REPOSITORY_ID, $result['repositoryId']); @@ -2335,10 +2335,10 @@ public function testGetEventPush(): void public function testGetEventPushDetectsBranchCreated(): void { - $result = $this->vcsAdapter->getEvent( + $result = $this->vcsAdapter->getEvents( static::$pushEventName, $this->pushPayload(static::$defaultBranch, created: true) - ); + )[0]; $this->assertTrue($result['branchCreated']); $this->assertFalse($result['branchDeleted']); @@ -2346,10 +2346,10 @@ public function testGetEventPushDetectsBranchCreated(): void public function testGetEventPushDetectsBranchDeleted(): void { - $result = $this->vcsAdapter->getEvent( + $result = $this->vcsAdapter->getEvents( static::$pushEventName, $this->pushPayload(static::$defaultBranch, deleted: true) - ); + )[0]; $this->assertFalse($result['branchCreated']); $this->assertTrue($result['branchDeleted']); @@ -2357,7 +2357,7 @@ public function testGetEventPushDetectsBranchDeleted(): void public function testGetEventPullRequest(): void { - $result = $this->vcsAdapter->getEvent(static::$pullRequestEventName, $this->pullRequestPayload()); + $result = $this->vcsAdapter->getEvents(static::$pullRequestEventName, $this->pullRequestPayload())[0]; $this->assertSame('opened', $result['action']); $this->assertSame(self::EVENT_HEAD_BRANCH, $result['branch']); @@ -2371,7 +2371,7 @@ public function testGetEventPullRequest(): void public function testGetEventPullRequestDetectsExternal(): void { - $result = $this->vcsAdapter->getEvent(static::$pullRequestEventName, $this->pullRequestPayload(external: true)); + $result = $this->vcsAdapter->getEvents(static::$pullRequestEventName, $this->pullRequestPayload(external: true))[0]; $this->assertTrue($result['external']); } @@ -2379,7 +2379,7 @@ public function testGetEventPullRequestDetectsExternal(): void public function testGetEventInvalidPayload(): void { $this->expectException(Exception::class); - $this->vcsAdapter->getEvent('push', 'invalid json'); + $this->vcsAdapter->getEvents('push', 'invalid json'); } public function testGetEventUnsupportedEvent(): void @@ -2390,7 +2390,7 @@ public function testGetEventUnsupportedEvent(): void $this->fail('Failed to encode JSON payload'); } - $result = $this->vcsAdapter->getEvent('unsupported_event', $payload); + $result = $this->vcsAdapter->getEvents('unsupported_event', $payload); $this->assertIsArray($result); $this->assertEmpty($result); From d73bbe9b8306b6e5a303f522eae4ad40cefe8b9d Mon Sep 17 00:00:00 2001 From: harsh mahajan Date: Fri, 7 Aug 2026 13:45:04 +0530 Subject: [PATCH 2/2] test(vcs): assert the delivery describes one event Reading the first event said nothing about how many came back, so a delivery parsed into none or several would have failed on the event's contents rather than on the count that explains it. --- tests/VCS/Adapter/GitHubTest.php | 5 ++++- tests/VCS/Adapter/GitLabTest.php | 10 +++++++-- tests/VCS/Base.php | 37 ++++++++++++++++++++++++-------- 3 files changed, 40 insertions(+), 12 deletions(-) diff --git a/tests/VCS/Adapter/GitHubTest.php b/tests/VCS/Adapter/GitHubTest.php index bd29d117..a31f5851 100644 --- a/tests/VCS/Adapter/GitHubTest.php +++ b/tests/VCS/Adapter/GitHubTest.php @@ -145,7 +145,10 @@ public function testGetEventInstallation(): void $this->fail('Failed to encode JSON payload'); } - $result = $this->vcsAdapter->getEvents('installation', $payload)[0]; + $events = $this->vcsAdapter->getEvents('installation', $payload); + $this->assertIsArray($events); + $this->assertCount(1, $events); + $result = $events[0]; $this->assertSame('deleted', $result['action']); $this->assertSame('1234', $result['installationId']); diff --git a/tests/VCS/Adapter/GitLabTest.php b/tests/VCS/Adapter/GitLabTest.php index 6f41afe2..13c4704b 100644 --- a/tests/VCS/Adapter/GitLabTest.php +++ b/tests/VCS/Adapter/GitLabTest.php @@ -159,7 +159,10 @@ public function testGetEventPushMatchesCheckoutSha(): void $this->fail('Failed to encode JSON payload'); } - $result = $this->vcsAdapter->getEvents('Push Hook', $payload)[0]; + $events = $this->vcsAdapter->getEvents('Push Hook', $payload); + $this->assertIsArray($events); + $this->assertCount(1, $events); + $result = $events[0]; $this->assertIsArray($result); $this->assertSame('def456', $result['commitHash']); @@ -181,7 +184,10 @@ public function testGetEventPullRequestActionMapping(): void $this->fail('Failed to encode JSON payload'); } - $result = $this->vcsAdapter->getEvents('Merge Request Hook', $payload)[0]; + $events = $this->vcsAdapter->getEvents('Merge Request Hook', $payload); + $this->assertIsArray($events); + $this->assertCount(1, $events); + $result = $events[0]; $this->assertSame($mapped, $result['action'], "native action '{$native}' should map to '{$mapped}'"); } } diff --git a/tests/VCS/Base.php b/tests/VCS/Base.php index 59345b8c..d50b4093 100644 --- a/tests/VCS/Base.php +++ b/tests/VCS/Base.php @@ -1556,7 +1556,11 @@ protected function awaitWebhook(string $eventName, string $secret): array 'Webhook signature did not validate' ); - return $this->vcsAdapter->getEvents($eventName, $payload)[0] ?? []; + $events = $this->vcsAdapter->getEvents($eventName, $payload); + $this->assertIsArray($events); + $this->assertCount(1, $events); + + return $events[0]; } public function testValidateWebhookEvent(): void @@ -2312,10 +2316,13 @@ public function testGetRepositoryAfterDeleteFails(): void public function testGetEventPush(): void { - $result = $this->vcsAdapter->getEvents( + $events = $this->vcsAdapter->getEvents( static::$pushEventName, $this->pushPayload(static::$defaultBranch, ['file1.txt'], ['file2.txt'], ['file3.txt']) - )[0]; + ); + $this->assertIsArray($events); + $this->assertCount(1, $events); + $result = $events[0]; $this->assertSame(static::$defaultBranch, $result['branch']); $this->assertSame(self::EVENT_REPOSITORY_ID, $result['repositoryId']); @@ -2335,10 +2342,13 @@ public function testGetEventPush(): void public function testGetEventPushDetectsBranchCreated(): void { - $result = $this->vcsAdapter->getEvents( + $events = $this->vcsAdapter->getEvents( static::$pushEventName, $this->pushPayload(static::$defaultBranch, created: true) - )[0]; + ); + $this->assertIsArray($events); + $this->assertCount(1, $events); + $result = $events[0]; $this->assertTrue($result['branchCreated']); $this->assertFalse($result['branchDeleted']); @@ -2346,10 +2356,13 @@ public function testGetEventPushDetectsBranchCreated(): void public function testGetEventPushDetectsBranchDeleted(): void { - $result = $this->vcsAdapter->getEvents( + $events = $this->vcsAdapter->getEvents( static::$pushEventName, $this->pushPayload(static::$defaultBranch, deleted: true) - )[0]; + ); + $this->assertIsArray($events); + $this->assertCount(1, $events); + $result = $events[0]; $this->assertFalse($result['branchCreated']); $this->assertTrue($result['branchDeleted']); @@ -2357,7 +2370,10 @@ public function testGetEventPushDetectsBranchDeleted(): void public function testGetEventPullRequest(): void { - $result = $this->vcsAdapter->getEvents(static::$pullRequestEventName, $this->pullRequestPayload())[0]; + $events = $this->vcsAdapter->getEvents(static::$pullRequestEventName, $this->pullRequestPayload()); + $this->assertIsArray($events); + $this->assertCount(1, $events); + $result = $events[0]; $this->assertSame('opened', $result['action']); $this->assertSame(self::EVENT_HEAD_BRANCH, $result['branch']); @@ -2371,7 +2387,10 @@ public function testGetEventPullRequest(): void public function testGetEventPullRequestDetectsExternal(): void { - $result = $this->vcsAdapter->getEvents(static::$pullRequestEventName, $this->pullRequestPayload(external: true))[0]; + $events = $this->vcsAdapter->getEvents(static::$pullRequestEventName, $this->pullRequestPayload(external: true)); + $this->assertIsArray($events); + $this->assertCount(1, $events); + $result = $events[0]; $this->assertTrue($result['external']); }