Skip to content

feat: add hooks support#240

Open
keelerm84 wants to merge 4 commits intomainfrom
mk/sdk-1255/hooks-support
Open

feat: add hooks support#240
keelerm84 wants to merge 4 commits intomainfrom
mk/sdk-1255/hooks-support

Conversation

@keelerm84
Copy link
Copy Markdown
Member

@keelerm84 keelerm84 commented Apr 21, 2026

Implements the Hooks specification (evaluation series + track series) to
give users and LaunchDarkly integration packages an extension point for
observing flag evaluations and track calls.

  • Public API under LaunchDarkly\Hooks: Hook (abstract base class),
    Metadata, EvaluationSeriesContext, TrackSeriesContext.
  • LDClient accepts a hooks option at construction and exposes
    addHook() for runtime registration.
  • beforeEvaluation / afterEvaluation fire around every variation,
    variationDetail, and migrationVariation call, with afterEvaluation
    running in reverse registration order per spec 1.3.4.
  • afterTrack fires after a custom event is enqueued. It does not fire
    for trackMigrationOperation (not a custom event) or for track calls
    rejected due to an invalid context.
  • Hook exceptions are caught and logged at the error level; when a stage
    errors, subsequent stages receive the previous successful stage's data
    (spec 1.3.7.1).
  • environmentId is intentionally omitted from the series contexts until
    the SDK can populate it reliably; the spec lists it as optional.
  • Identify series (1.4) and configuration handlers (1.5) are skipped
    because they are client-side only.
  • Contract test service declares evaluation-hooks and track-hooks
    capabilities and wires harness-provided hook configs through a new
    PostingHook implementation.

Note

Medium Risk
Touches core LDClient evaluation/track paths by adding hook dispatch and refactoring evaluation into evaluateInternal, which could affect ordering/performance and error handling if hooks misbehave. Failures are isolated and logged, but this introduces new execution around every flag evaluation and custom event.

Overview
Adds a new public hooks API (LaunchDarkly\Hooks\Hook, Metadata, EvaluationSeriesContext, TrackSeriesContext) and an internal HookRunner to run hook stages with defined ordering and exception isolation.

LDClient now accepts a hooks constructor option and exposes addHook(). It wraps variation, variationDetail, and migrationVariation with beforeEvaluation/afterEvaluation (after runs in reverse registration order) and invokes afterTrack after successful custom track() enqueues, while intentionally not firing hooks for invalid-track calls or trackMigrationOperation.

Updates the contract test service to advertise hook capabilities and to construct a PostingHook from harness config; adds unit tests covering ordering, data propagation, and logging behavior. Updates psalm-baseline.xml accordingly.

Reviewed by Cursor Bugbot for commit b68661a. Bugbot is set up for automated code reviews on this repo. Configure here.

Implements the Hooks specification (evaluation series + track series) to
give users and LaunchDarkly integration packages an extension point for
observing flag evaluations and track calls.

- Public API under `LaunchDarkly\Hooks`: `Hook` (abstract base class),
  `Metadata`, `EvaluationSeriesContext`, `TrackSeriesContext`.
- `LDClient` accepts a `hooks` option at construction and exposes
  `addHook()` for runtime registration.
- `beforeEvaluation` / `afterEvaluation` fire around every `variation`,
  `variationDetail`, and `migrationVariation` call, with `afterEvaluation`
  running in reverse registration order per spec 1.3.4.
- `afterTrack` fires after a custom event is enqueued. It does not fire
  for `trackMigrationOperation` (not a custom event) or for `track` calls
  rejected due to an invalid context.
- Hook exceptions are caught and logged at the error level; when a stage
  errors, subsequent stages receive the previous successful stage's data
  (spec 1.3.7.1).
- `environmentId` is intentionally omitted from the series contexts until
  the SDK can populate it reliably; the spec lists it as optional.
- Identify series (1.4) and configuration handlers (1.5) are skipped
  because they are client-side only.
- Contract test service declares `evaluation-hooks` and `track-hooks`
  capabilities and wires harness-provided hook configs through a new
  `PostingHook` implementation.
@keelerm84 keelerm84 requested a review from a team as a code owner April 21, 2026 19:55
@keelerm84 keelerm84 requested a review from kinyoklion April 21, 2026 19:55
CI flagged an UnusedBaselineEntry for NullEventProcessor's UnusedClass
error. The entry was added when I regenerated the baseline locally, but
CI's psalm doesn't produce that error — so the baseline entry is stale
from CI's perspective. Remove it to match what CI sees.

The root cause of the local/CI divergence appears to be environmental
(same psalm 6.16.1, --no-cache, same composer.lock) but isn't worth
chasing for this PR.
Comment thread test-service/PostingHook.php Outdated
Co-authored-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 8f7b5e0. Configure here.

$runner->afterEvaluation($this->ctx(), $before, $this->detail());

// Hook A's afterEvaluation receives [] because its beforeEvaluation failed.
$this->assertSame([], $a->calls[0]['data']);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Test asserts wrong index, trivially passes always

Medium Severity

The assertion $a->calls[0]['data'] checks the beforeEvaluation record (index 0), whose data field is always [] because HookRunner::beforeEvaluation unconditionally passes an empty array to every hook. The comment says "Hook A's afterEvaluation receives [] because its beforeEvaluation failed," so the assertion needs to use $a->calls[1]['data'] (the afterEvaluation record) to actually verify the error recovery behavior described by spec 1.3.7.1. As written, this test passes regardless of whether the fallback-on-error logic works correctly.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 8f7b5e0. Configure here.

RecordingHook took its shared log as a by-reference constructor-promoted
parameter (`private ?array &$sharedLog`). PHP CS Fixer 3.80.0 — the
version CI ends up running under the prefer-lowest matrix entry — has a
bug in its visibility_required fixer that mutates this into invalid
syntax (`private ?array &public $sharedLog`), breaking `make lint`.

Swap the reference for a small CallLog value object that holds a
mutable calls array. Cross-hook ordering tests read `$shared->calls`
instead of relying on PHP array-by-reference semantics.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants