From 6c3f79564762005863ef10b789b16dcd84a0d557 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joachim=20L=C3=B8vgaard?= Date: Mon, 7 Sep 2026 13:49:49 +0200 Subject: [PATCH] Keep access tokens and raw PII out of the Messenger transport SendEvent carried the Event object, so routing it to a transport wrote the access token and every raw email, phone number and name into that transport's storage, and into the failure transport on failure. Hashing only happened later, inside Client::sendEvent(). Require setono/meta-conversions-api-php-sdk ^2.0.0-alpha.3, which has a first-class answer to this: Event::prepare() returns a PreparedEvent whose payload is already normalized and hashed, and ClientInterface::sendPreparedEvent() sends it. SendEvent now carries that PreparedEvent, stripped with withoutAccessTokens() in its constructor so no caller can put a token on the transport. The handler adds the tokens back with withAccessTokens(), taking them from the PixelProviderInterface. The handler also maps the SDK's exception model onto Messenger: invalid input and a rejection by Meta are unrecoverable, while a transport failure, a 5xx response and an error Meta flags as transient are left for Messenger to retry. Skipping a pixel without an access token is left to the SDK client, which does that as of alpha.3, so the handler no longer filters pixels or logs. An integration test pins that behaviour to the real client. Fixes #17 --- README.md | 17 +- UPGRADE.md | 60 ++++++- composer.json | 2 +- src/DependencyInjection/Configuration.php | 4 + .../DispatchOnCommandBusSubscriber.php | 2 +- src/Message/Command/SendEvent.php | 22 ++- src/Message/Handler/SendEventHandler.php | 72 ++++---- src/Provider/PixelProviderInterface.php | 6 +- .../services/conditional/server_side.xml | 3 +- .../RecordingConversionsApiClientFactory.php | 16 +- .../SetonoMetaConversionsApiExtensionTest.php | 14 ++ tests/Integration/PipelineTest.php | 10 +- .../SetonoMetaConversionsApiBundleTest.php | 46 ++++- .../DispatchOnCommandBusSubscriberTest.php | 3 +- tests/Unit/Message/Command/SendEventTest.php | 87 ++++++++++ .../Message/Handler/SendEventHandlerTest.php | 161 +++++++++++++----- 16 files changed, 416 insertions(+), 109 deletions(-) create mode 100644 tests/Unit/Message/Command/SendEventTest.php diff --git a/README.md b/README.md index 1b12896..99d0d9f 100644 --- a/README.md +++ b/README.md @@ -88,9 +88,10 @@ setono_meta_conversions_api: message_bus: messenger.default_bus # The pixels to send events to (empty by default). Alternatively provide pixels from your own source by - # aliasing Setono\MetaConversionsApiBundle\Provider\PixelProviderInterface to your own service. + # aliasing Setono\MetaConversionsApiBundle\Provider\PixelProviderInterface to your own service. It is also asked + # for the access tokens when an event is sent, which may be in a worker, so it has to work without a request. # The access token is only needed for server side tracking: client side tracking renders fbq() calls, which - # only need the pixel id. A pixel without an access token is skipped server side, with a warning in the log + # only need the pixel id. A pixel without an access token is skipped server side, with an error in the log pixels: - id: '%env(META_PIXEL_ID)%' access_token: '%env(META_ACCESS_TOKEN)%' @@ -136,7 +137,13 @@ framework: Every command the bundle dispatches implements `Setono\MetaConversionsApiBundle\Message\Command\CommandInterface`, so you can route them as a group instead. -With a transport, Messenger also retries a failed send and moves it to the failure transport when it keeps failing. +With a transport, Messenger also retries a send that can still succeed, i.e. a network failure, a server error at Meta +or an error Meta itself flags as transient, and moves it to the failure transport when it keeps failing. A send that +a retry cannot fix, an invalid access token for instance, goes to the failure transport straight away. + +What ends up in the transport is the SDK's `PreparedEvent`: its payload is already normalised and hashed, and the +bundle strips the access tokens from its pixels before dispatching. They are added back when the event is sent, from +your `PixelProviderInterface`, so a provider of your own has to work in a worker too, where there is no request. Either way, a send that fails is logged as an error and never propagates into the response, so an expired access token or an outage at Meta cannot break the page. @@ -292,8 +299,8 @@ and phone numbers are never sent, and they are not written to the Messenger tran ### Why did my event not show up? Every listener that drops an event says so at debug level on the `setono_meta_conversions_api` Monolog channel: the -bot filter, the user agent filters, the no-pixels check, and each of the three consent gates. The send handler logs a -warning when a pixel has no access token. +bot filter, the user agent filters, the no-pixels check, and each of the three consent gates. The SDK client logs an +error on the same channel when it skips a pixel without an access token. ```yaml # config/packages/monolog.yaml diff --git a/UPGRADE.md b/UPGRADE.md index 79208e1..ee177cf 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -3,7 +3,7 @@ ## Requirements - PHP 8.1+ (was 7.4) and Symfony 6.4 or 7.4 (Symfony 5.4, 6.0–6.3 and 7.0–7.3 are no longer supported). -- `setono/meta-conversions-api-php-sdk` `^1.0` (was `^0.2.1`). Consequences: +- `setono/meta-conversions-api-php-sdk` `^2.0` (was `^0.2.1`). Consequences: - Events are posted to Graph API **v25.0 / v26.0** (whichever `facebook/php-business-sdk` is installed) instead of v14.0. The payloads are unchanged. - The SDK needs a [PSR-18](https://www.php-fig.org/psr/psr-18/) HTTP client and [PSR-17](https://www.php-fig.org/psr/psr-17/) @@ -13,6 +13,16 @@ - `php-http/discovery` ships a Composer plugin. Add `"php-http/discovery": false` (or `true`) to `config.allow-plugins` in your `composer.json` to avoid the interactive prompt. - `Client::setResponseFactory()` was removed from the SDK. Drop the call if you configured the client manually. + - `ClientException` no longer exists. Everything the SDK throws implements + `Setono\MetaConversionsApi\Exception\ExceptionInterface`; catch that, or `InvalidArgumentException`, + `TransportException` and `ResponseException` specifically. The bundle's handler uses them to tell Messenger what + is worth retrying: a transport failure, a 5xx response or an error Meta flags as transient is retried, anything + else goes straight to the failure transport. + - `ClientInterface` gained `sendPreparedEvent()`, and the bundle now sends server side events through it rather + than through `sendEvent()`. If you decorate or replace `Setono\MetaConversionsApi\Client\ClientInterface`, + implement it: a decorator that only wraps `sendEvent()` is no longer called for the events the bundle sends. + - Until the SDK's 2.0 is stable you have to allow its pre-release in your own `composer.json`, because a stability + flag on a dependency's requirement is not inherited: `composer require setono/meta-conversions-api-php-sdk:^2.0@alpha`. - `setono/consent-contracts` is now a required dependency. The consent *bundle* (`setono/consent-bundle`) remains optional. - `symfony/monolog-bundle` is no longer required by the bundle. The SDK client is wired to the `logger` service when it exists. @@ -46,6 +56,10 @@ services: Setono\MetaConversionsApiBundle\Provider\PixelProviderInterface: '@App\Provider\MyPixelProvider' ``` +Your provider is now also asked for the access tokens when an event is sent, which may be in a worker long after the +request is gone, see [The SendEvent command changed shape](#the-sendevent-command-changed-shape). It therefore has to +return the pixels, with their tokens, when there is no request. + ## Messenger bus The bundle no longer registers a `setono_meta_conversions_api.command_bus` Messenger bus, and it no longer prepends @@ -81,6 +95,42 @@ Enabling client side tracking without the tag bag bundle now throws `\LogicExcep `\InvalidArgumentException`, which is what Symfony uses for "this bundle needs that bundle". Adjust your test if you asserted on the old type. +## The SendEvent command changed shape + +`SendEvent` no longer carries the `Setono\MetaConversionsApi\Event\Event` object. It carries the SDK's +`Setono\MetaConversionsApi\Event\PreparedEvent` instead: the payload already normalized and hashed, the pixels and the +test event code. + +```php +new SendEvent(PreparedEvent $preparedEvent); +``` + +Build one from an event with `SendEvent::fromEvent($event)`. The constructor strips the access tokens from the pixels, +whatever it is given, so there is no way to put one on the transport. + +**Why:** when the command is routed to a transport it is written to that transport's storage, and to the failure +transport when it fails. Previously that storage received the Conversions API access token and every raw email +address, phone number and name the application had attached, because hashing only happened later inside +`Client::sendEvent()`. Failure transports are often kept indefinitely, which made that a retention problem too. + +The access tokens are added back when the event is sent, from the `PixelProviderInterface`. That call may happen in a +worker, so if you provide your own pixels, your provider has to return them, with their tokens, when there is no +request. `SendEventHandler::__construct()` changed accordingly, from `(ClientInterface $client, ?LoggerInterface $logger)` +to `(ClientInterface $client, PixelProviderInterface $pixelProvider)`. Update the service definition if you decorated +or redefined it. + +When none of an event's pixels has an access token, the SDK refuses to send and the handler tells Messenger not to +retry, so the message goes to the failure transport, if you have one, where it can be retried once the token is +configured. Handled synchronously, it is logged as an error instead. + +If you wrote your own handler or middleware for `SendEvent`, read `$message->preparedEvent` (its `payload`, `pixels` +and `testEventCode`) instead of `$message->event`. + +**Deploying:** a `SendEvent` that 0.1.x wrote to a transport still has the old shape and cannot be handled by this +release. It fails, is retried, and ends up in the failure transport, its body still holding the access token and the +raw personal data. Stop the workers and let the transport drain on the old release before deploying, and remove +whatever is left in the failure transport afterwards with `messenger:failed:remove`. + ## Removed container parameters `setono_meta_conversions_api.client_side.enabled` and `setono_meta_conversions_api.server_side.enabled` are gone. No @@ -93,7 +143,7 @@ which is what enrichment listeners need, but the properties can no longer be swa ## Failures no longer propagate `DispatchOnCommandBusSubscriber` catches and logs anything thrown while dispatching, at error level on the -`setono_meta_conversions_api` channel. Previously a synchronously handled command let a `ClientException` from the SDK +`setono_meta_conversions_api` channel. Previously a synchronously handled command let an exception from the SDK propagate out of `EventDispatcher::dispatch()` into the controller, so an expired access token or an outage at Meta returned a 500 to the visitor. @@ -115,9 +165,9 @@ registered a listener between the old and the new filter positions expecting it ## Pixel access token `pixels[].access_token` is no longer required. Client side tracking only needs the pixel id, so a client-side-only -setup no longer has to configure a dummy token. Server side, a pixel without an access token is skipped and logged as -a warning instead of being posted to Meta, rejected with a 400 and retried by Messenger until it lands in the failure -transport. +setup no longer has to configure a dummy token. Server side, a pixel without an access token is skipped, and the SDK +logs an error naming it, instead of it being posted to Meta, rejected with a 400 and retried by Messenger until it +lands in the failure transport. ## Test event code diff --git a/composer.json b/composer.json index 3b3ad26..81143ac 100644 --- a/composer.json +++ b/composer.json @@ -15,7 +15,7 @@ "psr/log": "^1.1 || ^2.0 || ^3.0", "setono/bot-detection-bundle": "^1.7", "setono/consent-contracts": "^1.1", - "setono/meta-conversions-api-php-sdk": "^1.2", + "setono/meta-conversions-api-php-sdk": "^2.0.0-alpha.3", "symfony/config": "^6.4 || ^7.4", "symfony/dependency-injection": "^6.4 || ^7.4", "symfony/event-dispatcher": "^6.4 || ^7.4", diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index d0e4401..be385c1 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -56,6 +56,10 @@ public function getConfigTreeBuilder(): TreeBuilder ->end() ->end() ->arrayNode('pixels') + ->validate() + ->ifTrue(static fn (array $pixels): bool => count(array_unique(array_column($pixels, 'id'), \SORT_REGULAR)) !== count($pixels)) + ->thenInvalid('Each pixel id can only be configured once, because the access token is looked up by pixel id when an event is sent') + ->end() ->arrayPrototype() ->children() ->scalarNode('id')->isRequired()->cannotBeEmpty()->end() diff --git a/src/EventSubscriber/DispatchOnCommandBusSubscriber.php b/src/EventSubscriber/DispatchOnCommandBusSubscriber.php index 5b56332..90c4db5 100644 --- a/src/EventSubscriber/DispatchOnCommandBusSubscriber.php +++ b/src/EventSubscriber/DispatchOnCommandBusSubscriber.php @@ -43,7 +43,7 @@ public function dispatch(ConversionsApiEventRaised $event): void } try { - $this->commandBus->dispatch(new SendEvent($event->event)); + $this->commandBus->dispatch(SendEvent::fromEvent($event->event)); } catch (\Throwable $e) { // Tracking must never take the page down. Two things can throw here: // diff --git a/src/Message/Command/SendEvent.php b/src/Message/Command/SendEvent.php index 70c4d07..d4de2cc 100644 --- a/src/Message/Command/SendEvent.php +++ b/src/Message/Command/SendEvent.php @@ -5,13 +5,33 @@ namespace Setono\MetaConversionsApiBundle\Message\Command; use Setono\MetaConversionsApi\Event\Event; +use Setono\MetaConversionsApi\Event\PreparedEvent; /** * Send a conversions api event to Meta/Facebook + * + * This deliberately carries the SDK's PreparedEvent rather than the Event object. When the command is routed to a + * transport it is written to that transport's storage, and to the failure transport when it fails, so it must not + * carry anything that does not belong there: + * + * - The payload of a prepared event is already normalized and hashed, so no raw email addresses or phone numbers are + * stored. + * - The access tokens are stripped in the constructor and added back when the event is sent, from the + * PixelProviderInterface. */ final class SendEvent implements CommandInterface { - public function __construct(public Event $event) + public readonly PreparedEvent $preparedEvent; + + public function __construct(PreparedEvent $preparedEvent) + { + // Event::prepare() keeps the access tokens on the pixels. Stripping them here, rather than trusting every + // caller to have done it, means there is no way to put an access token on the transport + $this->preparedEvent = $preparedEvent->withoutAccessTokens(); + } + + public static function fromEvent(Event $event): self { + return new self($event->prepare()); } } diff --git a/src/Message/Handler/SendEventHandler.php b/src/Message/Handler/SendEventHandler.php index 00c21dd..abdfd2e 100644 --- a/src/Message/Handler/SendEventHandler.php +++ b/src/Message/Handler/SendEventHandler.php @@ -4,59 +4,59 @@ namespace Setono\MetaConversionsApiBundle\Message\Handler; -use Psr\Log\LoggerInterface; -use Psr\Log\NullLogger; use Setono\MetaConversionsApi\Client\ClientInterface; -use Setono\MetaConversionsApi\Event\Event; -use Setono\MetaConversionsApi\Pixel\Pixel; +use Setono\MetaConversionsApi\Exception\InvalidArgumentException; +use Setono\MetaConversionsApi\Exception\ResponseException; use Setono\MetaConversionsApiBundle\Message\Command\SendEvent; +use Setono\MetaConversionsApiBundle\Provider\PixelProviderInterface; +use Symfony\Component\Messenger\Exception\UnrecoverableMessageHandlingException; final class SendEventHandler { - private readonly LoggerInterface $logger; - public function __construct( private readonly ClientInterface $client, - ?LoggerInterface $logger = null, + private readonly PixelProviderInterface $pixelProvider, ) { - $this->logger = $logger ?? new NullLogger(); } public function __invoke(SendEvent $message): void { - $event = $message->event; - - // A pixel without an access token cannot be used server side: Meta answers 400, the SDK throws, and - // Messenger retries the message until it ends up in the failure transport. One warning is more useful. - // Client side tracking is unaffected, because rendering fbq() calls only needs the pixel id - $pixels = array_values(array_filter( - $event->pixels, - fn (Pixel $pixel): bool => $this->hasAccessToken($pixel, $event), - )); - - if ([] === $pixels) { - return; + // The access tokens never travel with the message, so they are added back here. A pixel that still has none + // afterwards, one that is only used client side for instance, is skipped by the SDK client, which logs it + $preparedEvent = $message->preparedEvent->withAccessTokens($this->accessTokens()); + + // The SDK throws one exception per thing that can be done about a failure, which maps onto Messenger directly. + // A TransportException is deliberately not caught: the request never got a response, so a retry may succeed + try { + $this->client->sendPreparedEvent($preparedEvent); + } catch (InvalidArgumentException $e) { + // The input is wrong, e.g. none of the pixels has an access token or the payload cannot be encoded as JSON, + // and it will be just as wrong on the next attempt + throw new UnrecoverableMessageHandlingException($e->getMessage(), 0, $e); + } catch (ResponseException $e) { + // A server error, or an error Meta itself flags as transient, may go away. Anything else, an invalid access + // token for instance, is rejected again however many times Messenger retries + if ($e->statusCode >= 500 || true === $e->errorResponse?->transient) { + throw $e; + } + + throw new UnrecoverableMessageHandlingException($e->getMessage(), 0, $e); } - - // Cloned so the event the application still holds is not mutated when the command is handled synchronously - $event = clone $event; - $event->pixels = $pixels; - - $this->client->sendEvent($event); } - private function hasAccessToken(Pixel $pixel, Event $event): bool + /** + * @return array the access tokens indexed by pixel id + */ + private function accessTokens(): array { - if (null !== $pixel->accessToken) { - return true; - } + $accessTokens = []; - $this->logger->warning('The pixel {pixel} has no access token, so the event {event_name} ({event_id}) was not sent to it', [ - 'pixel' => $pixel->id, - 'event_name' => $event->eventName, - 'event_id' => $event->eventId, - ]); + foreach ($this->pixelProvider->getPixels() as $pixel) { + if (null !== $pixel->accessToken) { + $accessTokens[$pixel->id] = $pixel->accessToken; + } + } - return false; + return $accessTokens; } } diff --git a/src/Provider/PixelProviderInterface.php b/src/Provider/PixelProviderInterface.php index b3a2a6c..894a758 100644 --- a/src/Provider/PixelProviderInterface.php +++ b/src/Provider/PixelProviderInterface.php @@ -9,7 +9,11 @@ interface PixelProviderInterface { /** - * Returns the applicable pixel(s) for the current request + * Returns the applicable pixel(s) + * + * This is called while a request is handled, to decide which pixels an event goes to, and again when the event is + * sent, to get the access tokens, which never travel with the queued event. The second call may happen in a + * worker, where there is no request, so it has to return the pixels and their access tokens then too * * @return list */ diff --git a/src/Resources/config/services/conditional/server_side.xml b/src/Resources/config/services/conditional/server_side.xml index dabb149..e9928a6 100644 --- a/src/Resources/config/services/conditional/server_side.xml +++ b/src/Resources/config/services/conditional/server_side.xml @@ -14,10 +14,9 @@ - + - diff --git a/tests/Double/RecordingConversionsApiClientFactory.php b/tests/Double/RecordingConversionsApiClientFactory.php index e84d579..4e88536 100644 --- a/tests/Double/RecordingConversionsApiClientFactory.php +++ b/tests/Double/RecordingConversionsApiClientFactory.php @@ -6,20 +6,21 @@ use Setono\MetaConversionsApi\Client\ClientInterface; use Setono\MetaConversionsApi\Event\Event; +use Setono\MetaConversionsApi\Event\PreparedEvent; /** - * Records every event handed to the SDK client, so an end to end test can inspect what would have been sent + * Records every prepared event handed to the SDK client, so an end to end test can inspect what would have been sent * * Built through a factory because a container definition cannot hold a live object */ final class RecordingConversionsApiClientFactory { - /** @var list */ - public static array $events = []; + /** @var list */ + public static array $preparedEvents = []; public static function reset(): void { - self::$events = []; + self::$preparedEvents = []; } public static function create(): ClientInterface @@ -27,7 +28,12 @@ public static function create(): ClientInterface return new class() implements ClientInterface { public function sendEvent(Event $event): void { - RecordingConversionsApiClientFactory::$events[] = $event; + $this->sendPreparedEvent($event->prepare()); + } + + public function sendPreparedEvent(PreparedEvent $preparedEvent): void + { + RecordingConversionsApiClientFactory::$preparedEvents[] = $preparedEvent; } }; } diff --git a/tests/Integration/DependencyInjection/SetonoMetaConversionsApiExtensionTest.php b/tests/Integration/DependencyInjection/SetonoMetaConversionsApiExtensionTest.php index 9e1b059..6cfd8f6 100644 --- a/tests/Integration/DependencyInjection/SetonoMetaConversionsApiExtensionTest.php +++ b/tests/Integration/DependencyInjection/SetonoMetaConversionsApiExtensionTest.php @@ -91,6 +91,20 @@ public function it_does_not_load_client_side_event_subscribers_when_client_side_ $this->assertContainerBuilderNotHasService(AddLibraryToTagBagSubscriber::class); } + #[Test] + public function it_rejects_the_same_pixel_id_twice(): void + { + // The access token is resolved by pixel id when the event is sent, so a second entry would silently win + $this->expectException(InvalidConfigurationException::class); + + $this->load([ + 'pixels' => [ + ['id' => '1234', 'access_token' => 'first'], + ['id' => '1234', 'access_token' => 'second'], + ], + ]); + } + #[Test] public function it_accepts_a_pixel_without_an_access_token(): void { diff --git a/tests/Integration/PipelineTest.php b/tests/Integration/PipelineTest.php index ecbebc5..60d8e51 100644 --- a/tests/Integration/PipelineTest.php +++ b/tests/Integration/PipelineTest.php @@ -9,6 +9,7 @@ use Setono\BotDetectionBundle\SetonoBotDetectionBundle; use Setono\MetaConversionsApi\Client\ClientInterface; use Setono\MetaConversionsApi\Event\Event; +use Setono\MetaConversionsApi\Pixel\Pixel; use Setono\MetaConversionsApiBundle\Event\ConversionsApiEventRaised; use Setono\MetaConversionsApiBundle\SetonoMetaConversionsApiBundle; use Setono\MetaConversionsApiBundle\Tests\Double\RecordingConversionsApiClientFactory; @@ -80,8 +81,11 @@ static function (ConversionsApiEventRaised $event): void { $dispatcher->dispatch(new ConversionsApiEventRaised($metaEvent), ConversionsApiEventRaised::class); // Server side: the command was dispatched, handled, and reached the client - self::assertCount(1, RecordingConversionsApiClientFactory::$events); - $sent = RecordingConversionsApiClientFactory::$events[0]->getPayload(); + self::assertCount(1, RecordingConversionsApiClientFactory::$preparedEvents); + $sent = RecordingConversionsApiClientFactory::$preparedEvents[0]->payload; + + // The access token is stripped before the command is dispatched and resolved again by the handler + self::assertEquals([new Pixel('1234', 's3cr3t')], RecordingConversionsApiClientFactory::$preparedEvents[0]->pixels); self::assertSame('ViewContent', $sent['event_name']); self::assertSame('https://example.com/jeans', $sent['event_source_url']); @@ -129,7 +133,7 @@ static function () use (&$enriched): void { $dispatcher->dispatch(new ConversionsApiEventRaised(new Event(Event::EVENT_VIEW_CONTENT)), ConversionsApiEventRaised::class); - self::assertSame([], RecordingConversionsApiClientFactory::$events); + self::assertSame([], RecordingConversionsApiClientFactory::$preparedEvents); // ... and the application never spent anything enriching it self::assertFalse($enriched); diff --git a/tests/Integration/SetonoMetaConversionsApiBundleTest.php b/tests/Integration/SetonoMetaConversionsApiBundleTest.php index 82cdfed..2e956d1 100644 --- a/tests/Integration/SetonoMetaConversionsApiBundleTest.php +++ b/tests/Integration/SetonoMetaConversionsApiBundleTest.php @@ -229,6 +229,42 @@ public function it_dispatches_on_the_configured_message_bus(): void #[Test] public function it_sends_events_through_the_applications_http_client(): void + { + $event = new Event(Event::EVENT_VIEW_CONTENT); + $event->pixels = [new Pixel('1234', 's3cr3t')]; + + self::recordingClient()->sendEvent($event); + + // The Graph API version follows whichever facebook/php-business-sdk is installed + self::assertSame( + [['POST', sprintf('https://graph.facebook.com/v%s/1234/events', ApiConfig::APIVersion)]], + RecordingHttpClientFactory::$requests, + ); + } + + /** + * The README promises that a pixel without an access token, one that is only used client side for instance, is + * skipped server side while the event still reaches the others. The bundle no longer does that itself, the SDK + * client does, so this pins the promise to the real client rather than to a double + */ + #[Test] + public function it_skips_a_pixel_without_an_access_token_and_still_sends_to_the_others(): void + { + $event = new Event(Event::EVENT_VIEW_CONTENT); + $event->pixels = [new Pixel('1111'), new Pixel('1234', 's3cr3t')]; + + self::recordingClient()->sendEvent($event); + + self::assertSame( + [['POST', sprintf('https://graph.facebook.com/v%s/1234/events', ApiConfig::APIVersion)]], + RecordingHttpClientFactory::$requests, + ); + } + + /** + * Boots a kernel whose PSR-18 client records requests instead of sending them, and returns the SDK client + */ + private static function recordingClient(): ClientInterface { RecordingHttpClientFactory::reset(); @@ -255,18 +291,10 @@ public function it_sends_events_through_the_applications_http_client(): void }); }]); - $event = new Event(Event::EVENT_VIEW_CONTENT); - $event->pixels = [new Pixel('1234', 's3cr3t')]; - $client = self::getContainer()->get('test.conversions_api_client'); self::assertInstanceOf(ClientInterface::class, $client); - $client->sendEvent($event); - // The Graph API version follows whichever facebook/php-business-sdk is installed - self::assertSame( - [['POST', sprintf('https://graph.facebook.com/v%s/1234/events', ApiConfig::APIVersion)]], - RecordingHttpClientFactory::$requests, - ); + return $client; } #[Test] diff --git a/tests/Unit/EventSubscriber/DispatchOnCommandBusSubscriberTest.php b/tests/Unit/EventSubscriber/DispatchOnCommandBusSubscriberTest.php index dc9803c..4c78cc6 100644 --- a/tests/Unit/EventSubscriber/DispatchOnCommandBusSubscriberTest.php +++ b/tests/Unit/EventSubscriber/DispatchOnCommandBusSubscriberTest.php @@ -34,7 +34,8 @@ public function it_dispatches_the_command(): void self::assertCount(1, $dispatched); self::assertInstanceOf(SendEvent::class, $dispatched[0]); - self::assertSame($metaEvent, $dispatched[0]->event); + self::assertSame($metaEvent->eventName, $dispatched[0]->preparedEvent->eventName); + self::assertSame($metaEvent->eventId, $dispatched[0]->preparedEvent->eventId); } #[Test] diff --git a/tests/Unit/Message/Command/SendEventTest.php b/tests/Unit/Message/Command/SendEventTest.php new file mode 100644 index 0000000..b8ac1f0 --- /dev/null +++ b/tests/Unit/Message/Command/SendEventTest.php @@ -0,0 +1,87 @@ +preparedEvent; + + self::assertSame('Purchase', $preparedEvent->eventName); + self::assertSame('TEST1234', $preparedEvent->testEventCode); + self::assertArrayHasKey('user_data', $preparedEvent->payload); + self::assertEquals([new Pixel('1234')], $preparedEvent->pixels); + } + + /** + * The message is written to the transport's storage, and to the failure transport when it fails, so neither + * the access token nor any raw personal data may travel in it + */ + #[Test] + public function it_does_not_carry_the_access_token_or_raw_personal_data(): void + { + $serialized = serialize(SendEvent::fromEvent(self::event())); + + self::assertStringNotContainsString('s3cr3t', $serialized); + self::assertStringNotContainsString('customer@example.com', $serialized); + self::assertStringNotContainsString('+4512345678', $serialized); + self::assertStringNotContainsString('Joachim', $serialized); + } + + /** + * Event::prepare() keeps the tokens on the pixels, and a caller may well forget withoutAccessTokens(). The + * constructor does not leave that to chance + */ + #[Test] + public function it_strips_the_access_tokens_whatever_it_is_given(): void + { + $prepared = self::event()->prepare(); + self::assertSame('s3cr3t', $prepared->pixels[0]->accessToken); + + $message = new SendEvent($prepared); + + self::assertNull($message->preparedEvent->pixels[0]->accessToken); + self::assertStringNotContainsString('s3cr3t', serialize($message)); + } + + #[Test] + public function it_carries_the_hashed_personal_data(): void + { + $userData = SendEvent::fromEvent(self::event())->preparedEvent->payload['user_data']; + + self::assertIsArray($userData); + self::assertSame([hash('sha256', 'customer@example.com')], $userData['em']); + } + + #[Test] + public function it_survives_the_transport(): void + { + $message = SendEvent::fromEvent(self::event()); + + self::assertEquals($message, unserialize(serialize($message))); + } + + private static function event(): Event + { + $event = new Event(Event::EVENT_PURCHASE); + $event->pixels = [new Pixel('1234', 's3cr3t')]; + $event->testEventCode = 'TEST1234'; + $event->userData->email[] = 'customer@example.com'; + $event->userData->phoneNumber[] = '+4512345678'; + $event->userData->firstName[] = 'Joachim'; + + return $event; + } +} diff --git a/tests/Unit/Message/Handler/SendEventHandlerTest.php b/tests/Unit/Message/Handler/SendEventHandlerTest.php index 6e38834..a193464 100644 --- a/tests/Unit/Message/Handler/SendEventHandlerTest.php +++ b/tests/Unit/Message/Handler/SendEventHandlerTest.php @@ -5,82 +5,165 @@ namespace Setono\MetaConversionsApiBundle\Tests\Unit\Message\Handler; use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; +use Psr\Http\Client\ClientExceptionInterface; use Setono\MetaConversionsApi\Client\ClientInterface; -use Setono\MetaConversionsApi\Event\Event; +use Setono\MetaConversionsApi\Client\ErrorResponse; +use Setono\MetaConversionsApi\Event\PreparedEvent; +use Setono\MetaConversionsApi\Exception\InvalidArgumentException; +use Setono\MetaConversionsApi\Exception\ResponseException; +use Setono\MetaConversionsApi\Exception\TransportException; use Setono\MetaConversionsApi\Pixel\Pixel; use Setono\MetaConversionsApiBundle\Message\Command\SendEvent; use Setono\MetaConversionsApiBundle\Message\Handler\SendEventHandler; +use Setono\MetaConversionsApiBundle\Tests\Double\Doubles; +use Symfony\Component\Messenger\Exception\UnrecoverableMessageHandlingException; #[CoversClass(SendEventHandler::class)] final class SendEventHandlerTest extends TestCase { #[Test] - public function it_sends_the_event(): void + public function it_sends_the_prepared_event_with_its_access_token_back(): void { - $event = new Event(Event::EVENT_VIEW_CONTENT); - $event->pixels = [new Pixel('1234', 's3cr3t')]; - + $sent = null; $client = $this->createMock(ClientInterface::class); - $client->expects(self::once())->method('sendEvent')->with($event); + $client->expects(self::never())->method('sendEvent'); + $client->expects(self::once())->method('sendPreparedEvent')->willReturnCallback( + static function (PreparedEvent $preparedEvent) use (&$sent): void { + $sent = $preparedEvent; + }, + ); + + (new SendEventHandler($client, Doubles::pixelProvider([new Pixel('1234', 's3cr3t')])))(self::message(['1234'])); - (new SendEventHandler($client))(new SendEvent($event)); + self::assertInstanceOf(PreparedEvent::class, $sent); + // The payload travelled through the transport ready to post, so it is handed to the client untouched + self::assertSame(['event_name' => 'ViewContent'], $sent->payload); + self::assertSame('an-event-id', $sent->eventId); + self::assertSame('TEST1234', $sent->testEventCode); + // ... while the access token, which never travelled, is back + self::assertEquals([new Pixel('1234', 's3cr3t')], $sent->pixels); } + /** + * Skipping a pixel without an access token is the SDK client's job, so such a pixel has to reach it as it is: one + * the provider does not know about, and one it knows but has no token for, which is what an unset env var gives + */ #[Test] - public function it_skips_pixels_without_an_access_token(): void + public function it_leaves_pixels_without_an_access_token_to_the_sdk(): void { - $event = new Event(Event::EVENT_VIEW_CONTENT); - $event->pixels = [new Pixel('no-token'), new Pixel('1234', 's3cr3t')]; - $sent = null; $client = $this->createMock(ClientInterface::class); - $client->expects(self::once())->method('sendEvent')->willReturnCallback( - static function (Event $event) use (&$sent): void { - $sent = $event; + $client->expects(self::once())->method('sendPreparedEvent')->willReturnCallback( + static function (PreparedEvent $preparedEvent) use (&$sent): void { + $sent = $preparedEvent; }, ); - (new SendEventHandler($client))(new SendEvent($event)); + $provider = Doubles::pixelProvider([new Pixel('5678'), new Pixel('1234', 's3cr3t')]); - self::assertInstanceOf(Event::class, $sent); - self::assertEquals([new Pixel('1234', 's3cr3t')], $sent->pixels); + (new SendEventHandler($client, $provider))(self::message(['unknown', '5678', '1234'])); + + self::assertInstanceOf(PreparedEvent::class, $sent); + self::assertEquals([new Pixel('unknown'), new Pixel('5678'), new Pixel('1234', 's3cr3t')], $sent->pixels); } + /** + * Messenger retries whatever a handler throws, three times by default, unless it is told not to. These are the + * failures a retry cannot fix + */ #[Test] - public function it_does_not_send_when_no_pixel_has_an_access_token(): void + #[DataProvider('failuresARetryCannotFix')] + public function it_tells_messenger_not_to_retry(\Throwable $failure): void { - $event = new Event(Event::EVENT_VIEW_CONTENT); - $event->pixels = [new Pixel('no-token')]; - $client = $this->createMock(ClientInterface::class); - $client->expects(self::never())->method('sendEvent'); + $client->method('sendPreparedEvent')->willThrowException($failure); + + try { + (new SendEventHandler($client, Doubles::pixelProvider([new Pixel('1234', 's3cr3t')])))(self::message(['1234'])); + self::fail('Expected an UnrecoverableMessageHandlingException'); + } catch (UnrecoverableMessageHandlingException $e) { + // The reason must not be lost on the way to the failure transport + self::assertSame($failure, $e->getPrevious()); + self::assertSame($failure->getMessage(), $e->getMessage()); + } + } + + /** + * @return iterable + */ + public static function failuresARetryCannotFix(): iterable + { + yield 'invalid input' => [new InvalidArgumentException('The payload cannot be encoded as JSON')]; + + // What the SDK client throws when server side tracking is on but no access token is configured at all + yield 'none of the pixels has an access token' => [new InvalidArgumentException('The event was not sent to Meta/Facebook because none of its pixels has an access token: 1234')]; + + yield 'meta rejects the request' => [new ResponseException( + 400, + '{}', + ErrorResponse::fromJson('{"error":{"message":"Invalid OAuth access token","type":"OAuthException","code":190,"fbtrace_id":"x"}}'), + )]; + + yield 'meta says explicitly that it is not transient' => [new ResponseException( + 400, + '{}', + ErrorResponse::fromJson('{"error":{"message":"Invalid parameter","type":"OAuthException","code":100,"is_transient":false,"fbtrace_id":"x"}}'), + )]; - (new SendEventHandler($client))(new SendEvent($event)); + yield 'a client error from a proxy' => [new ResponseException(403, 'Forbidden', null)]; } + /** + * ... and these are the ones it may well fix, so they reach Messenger untouched + */ #[Test] - public function it_does_not_mutate_the_event_it_was_given(): void + #[DataProvider('failuresARetryMayFix')] + public function it_lets_messenger_retry(\Throwable $failure): void { - $event = new Event(Event::EVENT_VIEW_CONTENT); - $event->pixels = [new Pixel('no-token'), new Pixel('1234', 's3cr3t')]; - - $sent = null; $client = $this->createMock(ClientInterface::class); - $client->method('sendEvent')->willReturnCallback( - static function (Event $event) use (&$sent): void { - $sent = $event; + $client->method('sendPreparedEvent')->willThrowException($failure); + + try { + (new SendEventHandler($client, Doubles::pixelProvider([new Pixel('1234', 's3cr3t')])))(self::message(['1234'])); + self::fail('Expected the failure to propagate'); + } catch (\Throwable $e) { + self::assertSame($failure, $e); + } + } + + /** + * @return iterable + */ + public static function failuresARetryMayFix(): iterable + { + yield 'the request never got a response' => [new TransportException( + new class('Connection timed out') extends \RuntimeException implements ClientExceptionInterface { }, - ); + )]; - (new SendEventHandler($client))(new SendEvent($event)); + yield 'a server error at meta' => [new ResponseException(503, 'Service Unavailable', null)]; - // The application may still hold the event when the command is handled synchronously - self::assertNotSame($event, $sent); - self::assertSame( - ['no-token', '1234'], - array_map(static fn (Pixel $pixel): string => $pixel->id, $event->pixels), - ); + yield 'an error meta flags as transient' => [new ResponseException( + 400, + '{}', + ErrorResponse::fromJson('{"error":{"message":"Application request limit reached","type":"OAuthException","code":4,"is_transient":true,"fbtrace_id":"x"}}'), + )]; + } + + /** + * @param list $pixelIds + */ + private static function message(array $pixelIds): SendEvent + { + return new SendEvent(new PreparedEvent( + 'ViewContent', + 'an-event-id', + ['event_name' => 'ViewContent'], + array_map(static fn (string $pixelId): Pixel => new Pixel($pixelId), $pixelIds), + 'TEST1234', + )); } }