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', + )); } }