Skip to content

Let the client send a prepared Payload instead of only an Event - #16

Open
loevgaard wants to merge 1 commit into
masterfrom
send-prepared-payload
Open

loevgaard wants to merge 1 commit into
masterfrom
send-prepared-payload

Conversation

@loevgaard

Copy link
Copy Markdown
Member

Summary

Closes #15.

A consumer that hashes the personal data at capture time and sends later (e.g. through a queue) could neither queue the EventUser holds the raw PII until getPayload() runs — nor hand a finished payload back to the client, which only accepted an Event. The bundle works around that today with a PreparedEvent subclass that smuggles a payload through an object designed to produce one (MetaConversionsApiBundle#49).

  • Event::toPayload(): Payload — the wire-ready form of the event: eventName, eventId, the normalized + hashed data, the pixels and the testEventCode. Scalars, arrays and Pixel objects only, so it round-trips through serialize() (tested) and the Symfony serializer without tricks. eventName/eventId are duplicated from data on purpose so log sites don't narrow mixed.
  • Client::sendPayload(Payload $payload) does the sending; sendEvent() is now a one-line delegation to it, so both paths produce byte-identical requests (tested).
  • README section "Sending events later, e.g. through a queue", including the token-less-pixels-then-fill-in pattern (Pixel::$accessToken is already nullable).

Interface decision

sendPayload() lives on a new PayloadClientInterface that Client also implements, rather than being added to ClientInterface. The issue allows either; I went with the second interface because .github/workflows/backwards-compatibility-check.yaml runs Roave with no baseline on every PR, and adding a method to ClientInterface would turn that job permanently red for this PR (and CLAUDE.md asks to avoid public BC breaks). Verified locally: Roave reports no BC breaks from origin/master to this branch. If you'd rather take the documented break and keep a single interface, it's a three-line flip — say so.

One behavioural nuance

sendEvent() on an event without pixels still logs the same error and sends nothing (the guard moved into sendPayload(), so the payload path behaves identically). The only difference: the payload is now built before the guard, so an event with an invalid action_source and no pixels throws from toPayload() instead of being silently skipped — arguably the more correct outcome.

Test plan

  • EventTest::it_converts_to_a_payload — hashed data, pixels, test event code; eventName/eventId match data; the raw email never appears in the serialized payload
  • ClientTest::it_sends_payload — posts data once per pixel with the pixel's token and test_event_code
  • ClientTest::it_sends_the_same_request_for_an_event_and_its_payload
  • ClientTest::it_does_not_send_payload_when_it_has_no_pixels
  • PayloadTestserialize()/unserialize() round trip incl. a token-less pixel
  • 104 tests, 100 % line coverage; PHPStan (level max), ECS, dependency analyser green; Infection MSI 94 % / covered 94 %, no new escaped mutants
  • Roave BC check locally: no breaks

After this is released

The bundle deletes PreparedEvent; its SendEvent message carries a Payload built with token-less pixels, and the handler resolves the tokens and calls sendPayload() on PayloadClientInterface.

A consumer that hashes the personal data at capture time and sends later,
e.g. through a queue, could neither queue the Event (User holds the raw
PII until getPayload() runs) nor hand the finished payload back to the
client, which only accepted an Event.

Event::toPayload() now returns a Payload: the wire-ready form of the
event (name, id, the normalized and hashed data, the pixels and the test
event code), made of scalars, arrays and Pixel objects only so it
serializes without any tricks. Client::sendPayload() sends it, and
sendEvent() is a one-line delegation to it.

sendPayload() lives on a new PayloadClientInterface that Client also
implements, rather than on ClientInterface, so existing implementors of
ClientInterface keep working and the backwards compatibility check stays
green.

Closes #15
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.

Let the client send a prepared payload instead of only an Event

1 participant