From 7d43410ca128c778f1b5f277740b3c6d159a728a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joachim=20L=C3=B8vgaard?= Date: Mon, 26 Oct 2026 09:39:03 +0100 Subject: [PATCH 1/6] Defer persisting metadata of new clients until they come back The metadata of a new client (no client id cookie on the request) is stored in a signed, HttpOnly cookie (_pending_metadata) instead of the database. When the client comes back with it, PendingMetadataClientContext hands it to the client and it's persisted at the end of that request. Clients that never come back (crawlers, link checkers, cookie blocking browsers) never produce a row. Metadata that doesn't fit in the cookie (2 KB) or is changed after the cookie is stored is persisted right away like before. StoreCookieSubscriber now runs at priority -64 so metadata set by default priority response listeners is included. Closes #8 --- CLAUDE.md | 15 +- README.md | 47 +++- config/services.xml | 23 ++ src/Client/ChangeAwareMetadata.php | 16 ++ src/Context/PendingMetadataClientContext.php | 47 ++++ src/EventSubscriber/StoreCookieSubscriber.php | 59 +++- .../CookieBasedPendingMetadataStorage.php | 97 +++++++ .../PendingMetadataStorageInterface.php | 34 +++ tests/Functional/NewClientMetadataTest.php | 252 ++++++++++++++++++ tests/Unit/Client/ChangeAwareMetadataTest.php | 15 ++ .../PendingMetadataClientContextTest.php | 114 ++++++++ .../SetonoClientExtensionTest.php | 44 +++ .../StoreCookieSubscriberTest.php | 234 +++++++++++++--- .../CookieBasedPendingMetadataStorageTest.php | 228 ++++++++++++++++ 14 files changed, 1173 insertions(+), 52 deletions(-) create mode 100644 src/Context/PendingMetadataClientContext.php create mode 100644 src/PendingMetadataStorage/CookieBasedPendingMetadataStorage.php create mode 100644 src/PendingMetadataStorage/PendingMetadataStorageInterface.php create mode 100644 tests/Functional/NewClientMetadataTest.php create mode 100644 tests/Unit/Context/PendingMetadataClientContextTest.php create mode 100644 tests/Unit/PendingMetadataStorage/CookieBasedPendingMetadataStorageTest.php diff --git a/CLAUDE.md b/CLAUDE.md index 283cbf0..cd52f11 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -35,8 +35,9 @@ The core abstraction is `ClientContextInterface::getClient(): Client`. Controlle Three concerns are each built as a **Symfony service-decoration chain** in `config/services.xml`. Higher `decoration-priority` is applied first and ends up *innermost*; the public alias resolves to the outermost decorator. Understanding the resolution order is essential before changing any service wiring: -- **Client context** — runtime order outermost→innermost: `CachedClientContext` (prio 32) → `CookieBasedClientContext` (prio 64) → `DefaultClientContext`. +- **Client context** — runtime order outermost→innermost: `CachedClientContext` (prio 32) → `PendingMetadataClientContext` (prio 48) → `CookieBasedClientContext` (prio 64) → `DefaultClientContext`. - `Cached` memoizes the `Client` for the request. It implements `ResetInterface` and is tagged `kernel.reset`, so long-running runtimes (FrankenPHP worker mode, RoadRunner) don't hand one visitor's client to the next request; `tests/Functional/LongRunningProcessTest.php` covers this. Any new service that holds per-request state needs the same, unless it keys the state by the request object like `CachedCookieProvider`. + - `PendingMetadata`: if the main request carries pending metadata signed for the client's id and the client has no metadata yet (`count()` initializes the lazy ghost), returns a new `Client` with that metadata in a `ChangeAwareMetadata` marked dirty, so the persister writes it. - `CookieBased` reads the cookie; if present, builds `Client(cookieId, metadataProvider->getMetadata(...))`; otherwise delegates. - `Default` returns an anonymous `new Client(null, new ChangeAwareMetadata())`. - **Cookie provider** — `CachedCookieProvider` (prio 32, memoizes per `Request` in a `WeakMap` keyed by the request object, so it needs no reset; not `spl_object_hash()`, which PHP hands to a new request once the old one is freed) → `RequestBasedCookieProvider` (parses the cookie off the request). The test kernel keeps its requests alive, so only `CachedCookieProviderTest` can reproduce that handle reuse. @@ -44,13 +45,23 @@ Three concerns are each built as a **Symfony service-decoration chain** in `conf ### Request lifecycle -- `EventSubscriber/StoreCookieSubscriber` (on `kernel.response`): dispatches `Event/PreStoreCookieEvent` (an app can set `$store = false` to veto), then writes/refreshes the cookie with an updated `lastSeenAt`. The cookie is set `HttpOnly(false)` on purpose so client-side JS can read it. +- `EventSubscriber/StoreCookieSubscriber` (on `kernel.response`, priority -64 so metadata set by default-priority response listeners is included, and before the profiler at -100): dispatches `Event/PreStoreCookieEvent` (an app can set `$store = false` to veto), then writes/refreshes the cookie with an updated `lastSeenAt`. The cookie is set `HttpOnly(false)` on purpose so client-side JS can read it. It also handles pending metadata (below). - `EventSubscriber/StoreMetadataSubscriber` (on `kernel.finish_request`): calls the metadata persister for the current client. ### Lazy metadata (why writes are cheap) `Client/ChangeAwareMetadata` extends `setono/client`'s `Metadata` and flips a `dirty` flag on `set`/`remove`. `Client/LazyChangeAwareMetadata` adds Symfony VarExporter's `LazyGhostTrait`, so `DoctrineOrmBasedMetadataProvider` returns a lazy ghost whose DB query only fires on first access. `DoctrineOrmBasedMetadataPersister::persist()` then **short-circuits** in two cases: the metadata is a lazy ghost that was never initialized (never read), or it is not dirty (never mutated). This is what makes "lazy loaded metadata" in the README real — no SELECT and no flush unless the app touched metadata. +### Pending metadata (new clients) + +A *new* client is one whose request has no id cookie. Its metadata is not persisted until it comes back, so clients that never return (crawlers, cookie blockers) never produce a row (issue #8): + +- `StoreCookieSubscriber`, for a new client: dirty `ChangeAwareMetadata` goes to `PendingMetadataStorage/PendingMetadataStorageInterface::store()` and is then `markClean()`ed, so `StoreMetadataSubscriber` skips it. If `store()` returns `false` (too big / not JSON encodable) it stays dirty and is persisted right away; anything set after the subscriber ran makes it dirty again and is persisted right away too. On veto, a new client's metadata is `markClean()`ed (discarded), because the client never receives its id. +- `StoreCookieSubscriber`, when the cookie is stored: always calls `remove()`, which clears the pending cookie if the request carried one (a `store()` in the same response replaces the clearing cookie). +- `PendingMetadataClientContext` adopts the pending metadata on the return request (see above). +- `CookieBasedPendingMetadataStorage` (default, alias `setono_client.pending_metadata_storage.default`): cookie `_pending_metadata`, value `.` signed with `%kernel.secret%` over a context string + client id + JSON, `HttpOnly`, `Secure` on HTTPS, same expiration as the id cookie, max 2048 bytes URL-encoded. JSON flags mirror Doctrine's json type (`JSON_PRESERVE_ZERO_FRACTION`). +- `tests/Functional/NewClientMetadataTest.php` runs the real services through `HttpKernel` with an in-memory Doctrine manager and shows the whole flow. + ### Entity mapping `Entity/Metadata` (table `setono_client__metadata`, string id `clientId`, nullable `json` `metadata`) is mapped in `config/doctrine-mapping/Metadata.orm.xml` as a **mapped-superclass**. `EventListener/Doctrine/ConvertToEntityListener` listens on Doctrine's `loadClassMetadata` and flips `isMappedSuperclass = false` on the bundle's own `Metadata`, so it becomes a concrete entity. Apps can swap in a custom class via the `metadata_class` config option; `SetonoClientExtension::load()` enforces that it implements `Entity/MetadataInterface`, stores its declared name (e.g. without a leading backslash) in the parameter, and the app is responsible for mapping it. When the custom class extends `Entity/Metadata`, the extension removes the listener: the bundle's class stays a mapped superclass and the app's entity inherits its fields (`tests/Functional/CustomMetadataClassTest.php` runs `MetadataTest` that way). A custom class that only implements the interface leaves the bundle's `Metadata` an entity, as before. diff --git a/README.md b/README.md index 7762aab..6737ceb 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,9 @@ Recognize returning visitors in your Symfony application and attach your own met The bundle assigns every visitor a stable **client id**, stored in a first‑party cookie (`setono_client_id` by default) that also remembers when the visitor was *first* and *last* seen. On top of that you can persist arbitrary **metadata** per client — for example the first Google click id (`gclid`) a visitor arrived with, an A/B test variant, -or a referrer. Metadata is **lazy**: if you never read or write it, the bundle never touches the database. +or a referrer. Metadata is **lazy**: if you never read or write it, the bundle never touches the database. And the +metadata of a first‑time visitor is only written to the database once the visitor comes back, so crawlers don't fill +your database. Typical use cases: first‑party analytics, marketing attribution, returning‑visitor personalization. @@ -22,6 +24,7 @@ Typical use cases: first‑party analytics, marketing attribution, returning‑v - [Get the current client](#get-the-current-client) - [Read and write metadata](#read-and-write-metadata) - [Set metadata from a request](#set-metadata-from-a-request) + - [Metadata of new visitors](#metadata-of-new-visitors) - [Access the cookie directly](#access-the-cookie-directly) - [Skip storing the cookie (consent)](#skip-storing-the-cookie-consent) - [Configuration](#configuration) @@ -112,7 +115,8 @@ final class SomeService ### Read and write metadata `$client->metadata` is a key/value store. Reading a key for the first time lazily loads the metadata from the -database; writing marks it dirty so it is persisted at the end of the request. +database; writing marks it dirty so it is persisted at the end of the request (for a first‑time visitor, once it comes +back: see [Metadata of new visitors](#metadata-of-new-visitors)). ```php $metadata = $client->metadata; @@ -164,6 +168,31 @@ final class GoogleClickIdSubscriber implements EventSubscriberInterface } ``` +### Metadata of new visitors + +A visitor without the cookie is a *new* client, and many new clients never come back: crawlers, link checkers, link +preview fetchers and browsers that block cookies get a new client id on every request. So that they don't fill the +`setono_client__metadata` table with rows nobody reads, the metadata of a new client is not written to the database +right away. It travels to the browser in a second cookie, `setono_client_id_pending_metadata` (the cookie name plus +`_pending_metadata`), and is persisted on the visitor's next request, i.e. once the visitor has shown that it keeps +its cookies. A visitor that never comes back never produces a row. + +In the request where you set it, the metadata is available as usual, and TTLs carry over unchanged. + +Things to be aware of: + +- The pending metadata cookie is `HttpOnly` and signed with your `kernel.secret`, so JavaScript can't read it and the + visitor can neither forge nor change it. It is not encrypted, so visitors can see their own pending metadata in the + browser's developer tools until it is persisted. Changing `kernel.secret` discards pending metadata that hasn't been + persisted yet. +- Metadata that doesn't fit in about 2 KB, or can't be JSON encoded, is persisted right away. +- The metadata of a new client is captured when the cookie is stored (on `kernel.response` with priority `-64`). Set it + before that, e.g. on `kernel.request`, in a controller, or in a `kernel.response` listener with the default priority. + Metadata set later is persisted right away. +- If you need the metadata of a new client in the database during its first request, replace the + `setono_client.pending_metadata_storage.default` alias with a `PendingMetadataStorageInterface` implementation whose + `store()` returns `false`. + ### Access the cookie directly The cookie stores the client id plus the first/last seen timestamps. Read it through `CookieProviderInterface`: @@ -222,6 +251,9 @@ final class CookieConsentSubscriber implements EventSubscriberInterface } ``` +If you skip the cookie for a new visitor, its metadata is not persisted either: without the cookie the visitor can't be +recognized again, so nothing could ever read that metadata. + ## Configuration All options are optional; the defaults are shown below: @@ -230,6 +262,7 @@ All options are optional; the defaults are shown below: setono_client: cookie: # Name of the cookie that holds the client id. + # The pending metadata cookie is named after it: _pending_metadata. # NOTE: changing this makes every visitor with the old cookie name look like a new client. name: setono_client_id @@ -251,9 +284,14 @@ setono_client: A new client id is generated only when no valid cookie is present. - **Lazy metadata** — `$client->metadata` is a lazy ghost object. The database is queried only the first time you read a value, and a row is written only at the end of the request, and only if the metadata was actually changed. +- **Pending metadata** — for a new client, `StoreCookieSubscriber` stores changed metadata in the pending metadata + cookie instead of the database. When the client comes back with it, the metadata is handed to the client and + persisted at the end of that request, and the pending metadata cookie is removed. - **Resolution chain** — `ClientContextInterface` is built from a chain of decorating services - (`CachedClientContext` → `CookieBasedClientContext` → `DefaultClientContext`). The cached layer memoizes the client - per request; the cookie‑based layer builds it from the cookie; the default layer creates a fresh, anonymous client. + (`CachedClientContext` → `PendingMetadataClientContext` → `CookieBasedClientContext` → `DefaultClientContext`). The + cached layer memoizes the client per request; the pending metadata layer hands a returning client the metadata it + was given as a new client; the cookie‑based layer builds the client from the cookie; the default layer creates a + fresh, anonymous client. ## Extending the bundle @@ -265,6 +303,7 @@ Every moving part is a service behind an interface, so you can swap or decorate | Reading the cookie | `Setono\ClientBundle\CookieProvider\CookieProviderInterface` | | Loading metadata | `Setono\ClientBundle\MetadataProvider\MetadataProviderInterface` | | Persisting metadata | `Setono\ClientBundle\MetadataPersister\MetadataPersisterInterface` | +| Holding the metadata of new clients | `Setono\ClientBundle\PendingMetadataStorage\PendingMetadataStorageInterface` | | Metadata entity | `Setono\ClientBundle\Entity\MetadataInterface` | To add behaviour, [decorate](https://symfony.com/doc/current/service_container/service_decoration.html) the relevant diff --git a/config/services.xml b/config/services.xml index 78c7ed7..1218c38 100644 --- a/config/services.xml +++ b/config/services.xml @@ -17,6 +17,14 @@ + + + + + + @@ -61,6 +69,7 @@ class="Setono\ClientBundle\EventSubscriber\StoreCookieSubscriber"> + %setono_client.cookie.name% %setono_client.cookie.expiration% @@ -86,6 +95,20 @@ %setono_client.metadata_class% + + + + + + + %setono_client.cookie.name%_pending_metadata + %setono_client.cookie.expiration% + %kernel.secret% + + diff --git a/src/Client/ChangeAwareMetadata.php b/src/Client/ChangeAwareMetadata.php index 84c393a..3f7430f 100644 --- a/src/Client/ChangeAwareMetadata.php +++ b/src/Client/ChangeAwareMetadata.php @@ -15,6 +15,22 @@ public function isDirty(): bool return $this->dirty; } + /** + * Use this when the metadata holds changes that were not made through this object, e.g. pending metadata of a new client + */ + public function markDirty(): void + { + $this->dirty = true; + } + + /** + * Use this when the changes must not be persisted, e.g. because they were stored as pending metadata of a new client + */ + public function markClean(): void + { + $this->dirty = false; + } + public function set(string $key, mixed $value, ?int $ttl = null): void { parent::set($key, $value, $ttl); diff --git a/src/Context/PendingMetadataClientContext.php b/src/Context/PendingMetadataClientContext.php new file mode 100644 index 0000000..4fe3277 --- /dev/null +++ b/src/Context/PendingMetadataClientContext.php @@ -0,0 +1,47 @@ +decorated->getClient(); + + $request = $this->requestStack->getMainRequest(); + if (null === $request) { + return $client; + } + + $pendingMetadata = $this->pendingMetadataStorage->get($request, $client->id); + + // If the client already has metadata (e.g. because a concurrent request persisted the pending metadata), + // that metadata is at least as new as the pending metadata + if (null === $pendingMetadata || count($client->metadata) > 0) { + return $client; + } + + $metadata = new ChangeAwareMetadata($pendingMetadata); + $metadata->markDirty(); // the pending metadata isn't persisted yet + + return new Client($client->id, $metadata); + } +} diff --git a/src/EventSubscriber/StoreCookieSubscriber.php b/src/EventSubscriber/StoreCookieSubscriber.php index 6ac8f61..0e10c34 100644 --- a/src/EventSubscriber/StoreCookieSubscriber.php +++ b/src/EventSubscriber/StoreCookieSubscriber.php @@ -5,12 +5,18 @@ namespace Setono\ClientBundle\EventSubscriber; use Psr\EventDispatcher\EventDispatcherInterface; +use Setono\Client\Client; use Setono\Client\Cookie; +use Setono\Client\Metadata; +use Setono\ClientBundle\Client\ChangeAwareMetadata; use Setono\ClientBundle\Context\ClientContextInterface; use Setono\ClientBundle\CookieProvider\CookieProviderInterface; use Setono\ClientBundle\Event\PreStoreCookieEvent; +use Setono\ClientBundle\PendingMetadataStorage\PendingMetadataStorageInterface; use Symfony\Component\EventDispatcher\EventSubscriberInterface; use Symfony\Component\HttpFoundation\Cookie as HttpCookie; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Event\ResponseEvent; use Symfony\Component\HttpKernel\KernelEvents; @@ -19,6 +25,7 @@ final class StoreCookieSubscriber implements EventSubscriberInterface public function __construct( private readonly ClientContextInterface $clientContext, private readonly CookieProviderInterface $cookieProvider, + private readonly PendingMetadataStorageInterface $pendingMetadataStorage, private readonly EventDispatcherInterface $eventDispatcher, private readonly string $cookieName, private readonly string $cookieExpiration, @@ -28,7 +35,9 @@ public function __construct( public static function getSubscribedEvents(): array { return [ - KernelEvents::RESPONSE => 'store', + // Runs after listeners with the default priority, so the metadata they set on a new client is included + // in its pending metadata, and before the profiler (-100) collects the response + KernelEvents::RESPONSE => ['store', -64], ]; } @@ -38,22 +47,62 @@ public function store(ResponseEvent $event): void return; } - $preStoreEvent = new PreStoreCookieEvent($event->getRequest(), $event->getResponse()); + $request = $event->getRequest(); + $response = $event->getResponse(); + + $preStoreEvent = new PreStoreCookieEvent($request, $response); $this->eventDispatcher->dispatch($preStoreEvent); + $cookie = $this->cookieProvider->getCookie($request); + if (!$preStoreEvent->store) { + // A new client that doesn't get a cookie can't be recognized again, so its metadata would never be read + if (null === $cookie) { + self::markClean($this->clientContext->getClient()->metadata); + } + return; } - $cookie = $this->cookieProvider->getCookie($event->getRequest()); + // Pending metadata sent by the client is handed to the client when it's resolved, at the latest when its metadata + // is persisted on kernel.finish_request (see PendingMetadataClientContext), so the client can forget it + $this->pendingMetadataStorage->remove($request, $response); if (null === $cookie) { - $cookie = new Cookie($this->clientContext->getClient()->id); + $client = $this->clientContext->getClient(); + $cookie = new Cookie($client->id); + + $this->storePendingMetadata($request, $response, $client); } - $event->getResponse()->headers->setCookie( + $response->headers->setCookie( HttpCookie::create($this->cookieName, $cookie->withLastSeenAt(time())->toString(), $this->cookieExpiration) ->withHttpOnly(false), // this allows javascript to read the cookie ); } + + /** + * The metadata of a new client is not persisted until the client comes back with its cookie + */ + private function storePendingMetadata(Request $request, Response $response, Client $client): void + { + $metadata = $client->metadata; + if (!$metadata instanceof ChangeAwareMetadata || !$metadata->isDirty()) { + return; + } + + $data = $metadata->toArray(); + + // If the metadata can't be stored as pending metadata, it stays dirty and is persisted right away + if ([] === $data || $this->pendingMetadataStorage->store($request, $response, $client->id, $data)) { + $metadata->markClean(); + } + } + + private static function markClean(Metadata $metadata): void + { + if ($metadata instanceof ChangeAwareMetadata) { + $metadata->markClean(); + } + } } diff --git a/src/PendingMetadataStorage/CookieBasedPendingMetadataStorage.php b/src/PendingMetadataStorage/CookieBasedPendingMetadataStorage.php new file mode 100644 index 0000000..806b19f --- /dev/null +++ b/src/PendingMetadataStorage/CookieBasedPendingMetadataStorage.php @@ -0,0 +1,97 @@ +cookies->get($this->cookieName); + if (!is_string($value)) { + return null; + } + + $position = strrpos($value, '.'); + if (false === $position) { + return null; + } + + $json = substr($value, 0, $position); + if (!hash_equals($this->sign($clientId, $json), substr($value, $position + 1))) { + return null; + } + + /** @var array $metadata */ + $metadata = json_decode($json, true); + + return $metadata; + } + + public function store(Request $request, Response $response, string $clientId, array $metadata): bool + { + try { + // Like Doctrine's json type, this preserves the zero fraction of floats, so the metadata is the same whether it + // comes from the cookie or the database. Unescaped slashes and unicode keep the URL encoded cookie small + $json = json_encode( + $metadata, + \JSON_THROW_ON_ERROR | \JSON_PRESERVE_ZERO_FRACTION | \JSON_UNESCAPED_SLASHES | \JSON_UNESCAPED_UNICODE, + ); + } catch (\JsonException) { + return false; + } + + $value = $json . '.' . $this->sign($clientId, $json); + + // the cookie value is URL encoded when sent + if (strlen(rawurlencode($value)) > self::MAX_VALUE_LENGTH) { + return false; + } + + $response->headers->setCookie(HttpCookie::create( + $this->cookieName, + $value, + $this->cookieExpiration, + secure: $request->isSecure(), + )); + + return true; + } + + public function remove(Request $request, Response $response): void + { + if ($request->cookies->has($this->cookieName)) { + $response->headers->clearCookie($this->cookieName); + } + } + + private function sign(string $clientId, string $json): string + { + return hash_hmac('sha256', self::SIGNATURE_CONTEXT . '.' . $clientId . '.' . $json, $this->secret); + } +} diff --git a/src/PendingMetadataStorage/PendingMetadataStorageInterface.php b/src/PendingMetadataStorage/PendingMetadataStorageInterface.php new file mode 100644 index 0000000..b7798e4 --- /dev/null +++ b/src/PendingMetadataStorage/PendingMetadataStorageInterface.php @@ -0,0 +1,34 @@ +|null + */ + public function get(Request $request, string $clientId): ?array; + + /** + * Returns false if the metadata cannot be stored (e.g. because it is too big), in which case it must be persisted right away + * + * @param array $metadata the metadata as returned by Metadata::toArray() + */ + public function store(Request $request, Response $response, string $clientId, array $metadata): bool; + + /** + * Removes the pending metadata the request carries, if any + */ + public function remove(Request $request, Response $response): void; +} diff --git a/tests/Functional/NewClientMetadataTest.php b/tests/Functional/NewClientMetadataTest.php new file mode 100644 index 0000000..215b0b3 --- /dev/null +++ b/tests/Functional/NewClientMetadataTest.php @@ -0,0 +1,252 @@ + */ + private \ArrayObject $database; + + /** @var \ArrayObject */ + private \ArrayObject $databaseCalls; + + private ManagerRegistry $managerRegistry; + + /** + * The cookies of the browser used in the tests + * + * @var array + */ + private array $cookieJar = []; + + protected function setUp(): void + { + /** @var \ArrayObject $database */ + $database = new \ArrayObject(); + /** @var \ArrayObject $databaseCalls */ + $databaseCalls = new \ArrayObject(); + $this->database = $database; + $this->databaseCalls = $databaseCalls; + + $manager = $this->prophesize(EntityManagerInterface::class); + $manager->find(MetadataEntity::class, Argument::type('string'))->will( + static function (array $arguments) use ($database, $databaseCalls): ?MetadataEntity { + $databaseCalls->append('find'); + + /** @var string $clientId */ + $clientId = $arguments[1]; + + return $database[$clientId] ?? null; + }, + ); + $manager->persist(Argument::type(MetadataEntity::class))->will( + static function (array $arguments) use ($database, $databaseCalls): void { + $databaseCalls->append('persist'); + + /** @var MetadataEntity $entity */ + $entity = $arguments[0]; + $database[(string) $entity->getClientId()] = $entity; + }, + ); + $manager->flush()->will(static function () use ($databaseCalls): void { + $databaseCalls->append('flush'); + }); + + $managerRegistry = $this->prophesize(ManagerRegistry::class); + $managerRegistry->getManagerForClass(MetadataEntity::class)->willReturn($manager->reveal()); + + $this->managerRegistry = $managerRegistry->reveal(); + } + + /** + * @test + */ + public function it_does_not_touch_the_database_for_a_new_client_that_never_comes_back(): void + { + $response = $this->handle(Request::create('/?gclid=abc')); + + self::assertEqualsCanonicalizing( + [self::COOKIE_NAME, self::PENDING_METADATA_COOKIE_NAME], + array_map(static fn ($cookie) => $cookie->getName(), $response->headers->getCookies()), + ); + self::assertSame([], $this->databaseCalls->getArrayCopy()); + } + + /** + * @test + */ + public function it_persists_the_metadata_when_the_new_client_comes_back(): void + { + $this->browse('/?gclid=abc'); + self::assertSame([], $this->databaseCalls->getArrayCopy()); + + $response = $this->browse('/'); + + $clientId = Cookie::fromString($this->cookieJar[self::COOKIE_NAME])->clientId; + $entity = $this->database[$clientId] ?? null; + self::assertNotNull($entity); + + $metadata = $entity->getMetadata(); + self::assertSame('abc', $metadata['gclid']); + self::assertArrayHasKey('__expires', $metadata); + + $pendingMetadataCookies = array_values(array_filter( + $response->headers->getCookies(), + static fn ($cookie) => $cookie->getName() === self::PENDING_METADATA_COOKIE_NAME, + )); + self::assertCount(1, $pendingMetadataCookies); + self::assertTrue($pendingMetadataCookies[0]->isCleared()); + self::assertArrayNotHasKey(self::PENDING_METADATA_COOKIE_NAME, $this->cookieJar); + + // from now on, the metadata is only loaded when it's used + $databaseCalls = count($this->databaseCalls); + $this->browse('/'); + self::assertCount($databaseCalls, $this->databaseCalls); + } + + /** + * @test + */ + public function it_persists_the_metadata_right_away_when_it_does_not_fit_in_the_pending_metadata_cookie(): void + { + $this->browse('/?gclid=' . str_repeat('a', 2048)); + + self::assertArrayNotHasKey(self::PENDING_METADATA_COOKIE_NAME, $this->cookieJar); + self::assertCount(1, $this->database); + } + + /** + * @test + */ + public function it_persists_metadata_set_after_the_cookie_was_stored_right_away(): void + { + $this->browse('/?gclid=abc', metadataListenerPriority: -128); + + self::assertCount(1, $this->database); + } + + /** + * @test + */ + public function it_does_not_persist_the_metadata_of_a_new_client_that_does_not_get_a_cookie(): void + { + $response = $this->handle(Request::create('/?gclid=abc'), storeCookie: false); + + self::assertCount(0, $response->headers->getCookies()); + self::assertSame([], $this->databaseCalls->getArrayCopy()); + } + + /** + * Sends a request with the cookies of the browser and updates them with the cookies of the response + */ + private function browse(string $uri, int $metadataListenerPriority = 0): Response + { + $response = $this->handle(Request::create($uri, cookies: $this->cookieJar), $metadataListenerPriority); + + foreach ($response->headers->getCookies() as $cookie) { + if ($cookie->isCleared()) { + unset($this->cookieJar[$cookie->getName()]); + } else { + $this->cookieJar[$cookie->getName()] = (string) $cookie->getValue(); + } + } + + return $response; + } + + private function handle(Request $request, int $metadataListenerPriority = 0, bool $storeCookie = true): Response + { + $requestStack = new RequestStack(); + $eventDispatcher = new EventDispatcher(); + + $cookieProvider = new CachedCookieProvider( + new RequestBasedCookieProvider($requestStack, self::COOKIE_NAME), + $requestStack, + ); + $pendingMetadataStorage = new CookieBasedPendingMetadataStorage( + self::PENDING_METADATA_COOKIE_NAME, + '+365 days', + 'secret', + ); + $clientContext = new CachedClientContext(new PendingMetadataClientContext( + new CookieBasedClientContext( + new DefaultClientContext(), + new DoctrineOrmBasedMetadataProvider(new EmptyMetadataProvider(), $this->managerRegistry, MetadataEntity::class), + $cookieProvider, + ), + $pendingMetadataStorage, + $requestStack, + )); + + $eventDispatcher->addSubscriber(new StoreCookieSubscriber( + $clientContext, + $cookieProvider, + $pendingMetadataStorage, + $eventDispatcher, + self::COOKIE_NAME, + '+365 days', + )); + $eventDispatcher->addSubscriber(new StoreMetadataSubscriber( + $clientContext, + new DoctrineOrmBasedMetadataPersister($this->managerRegistry, MetadataEntity::class), + )); + + // Like Setono/SyliusGoogleAdsPlugin, the application stores the Google click id on the response + $eventDispatcher->addListener(KernelEvents::RESPONSE, static function (ResponseEvent $event) use ($clientContext): void { + $gclid = $event->getRequest()->query->get('gclid'); + if (null !== $gclid) { + $clientContext->getClient()->metadata->set('gclid', $gclid, 7_776_000); + } + }, $metadataListenerPriority); + + $eventDispatcher->addListener(PreStoreCookieEvent::class, static function (PreStoreCookieEvent $event) use ($storeCookie): void { + $event->store = $storeCookie; + }); + + $request->attributes->set('_controller', static fn (): Response => new Response()); + + return (new HttpKernel($eventDispatcher, new ControllerResolver(), $requestStack, new ArgumentResolver())) + ->handle($request); + } +} diff --git a/tests/Unit/Client/ChangeAwareMetadataTest.php b/tests/Unit/Client/ChangeAwareMetadataTest.php index a4b1389..ec02117 100644 --- a/tests/Unit/Client/ChangeAwareMetadataTest.php +++ b/tests/Unit/Client/ChangeAwareMetadataTest.php @@ -43,4 +43,19 @@ public function it_becomes_dirty_when_removing_a_value(): void self::assertTrue($metadata->isDirty()); self::assertFalse($metadata->has('foo')); } + + /** + * @test + */ + public function it_can_be_marked_dirty_and_clean(): void + { + $metadata = new ChangeAwareMetadata(['foo' => 'bar']); + + $metadata->markDirty(); + self::assertTrue($metadata->isDirty()); + + $metadata->markClean(); + self::assertFalse($metadata->isDirty()); + self::assertSame('bar', $metadata->get('foo')); + } } diff --git a/tests/Unit/Context/PendingMetadataClientContextTest.php b/tests/Unit/Context/PendingMetadataClientContextTest.php new file mode 100644 index 0000000..18fc8ef --- /dev/null +++ b/tests/Unit/Context/PendingMetadataClientContextTest.php @@ -0,0 +1,114 @@ +prophesize(PendingMetadataStorageInterface::class); + $storage->get(Argument::cetera())->shouldNotBeCalled(); + + $context = $this->createContext($decoratedClient, $storage->reveal(), new RequestStack()); + + self::assertSame($decoratedClient, $context->getClient()); + } + + /** + * @test + */ + public function it_returns_the_decorated_client_when_there_is_no_pending_metadata(): void + { + $decoratedClient = new Client('client-id'); + $request = new Request(); + + $storage = $this->prophesize(PendingMetadataStorageInterface::class); + $storage->get($request, 'client-id')->willReturn(null); + + $context = $this->createContext($decoratedClient, $storage->reveal(), self::createRequestStack($request)); + + self::assertSame($decoratedClient, $context->getClient()); + } + + /** + * @test + */ + public function it_hands_the_pending_metadata_to_a_client_without_metadata(): void + { + $request = new Request(); + $pendingMetadata = ['foo' => 'bar', '__expires' => ['foo' => 1893456000]]; + + $storage = $this->prophesize(PendingMetadataStorageInterface::class); + $storage->get($request, 'client-id')->willReturn($pendingMetadata); + + $context = $this->createContext( + new Client('client-id', new ChangeAwareMetadata()), + $storage->reveal(), + self::createRequestStack($request), + ); + + $client = $context->getClient(); + + self::assertSame('client-id', $client->id); + self::assertInstanceOf(ChangeAwareMetadata::class, $client->metadata); + self::assertTrue($client->metadata->isDirty()); + self::assertSame($pendingMetadata, $client->metadata->toArray()); + } + + /** + * @test + */ + public function it_ignores_the_pending_metadata_when_the_client_has_metadata(): void + { + $decoratedClient = new Client('client-id', new Metadata(['foo' => 'persisted'])); + $request = new Request(); + + $storage = $this->prophesize(PendingMetadataStorageInterface::class); + $storage->get($request, 'client-id')->willReturn(['foo' => 'pending']); + + $context = $this->createContext($decoratedClient, $storage->reveal(), self::createRequestStack($request)); + + self::assertSame($decoratedClient, $context->getClient()); + self::assertSame(['foo' => 'persisted'], $decoratedClient->metadata->toArray()); + } + + private function createContext( + Client $decoratedClient, + PendingMetadataStorageInterface $storage, + RequestStack $requestStack, + ): PendingMetadataClientContext { + $decorated = $this->prophesize(ClientContextInterface::class); + $decorated->getClient()->willReturn($decoratedClient); + + return new PendingMetadataClientContext($decorated->reveal(), $storage, $requestStack); + } + + private static function createRequestStack(Request $request): RequestStack + { + $requestStack = new RequestStack(); + $requestStack->push($request); + + return $requestStack; + } +} diff --git a/tests/Unit/DependencyInjection/SetonoClientExtensionTest.php b/tests/Unit/DependencyInjection/SetonoClientExtensionTest.php index 88e1e6d..14bbf2b 100644 --- a/tests/Unit/DependencyInjection/SetonoClientExtensionTest.php +++ b/tests/Unit/DependencyInjection/SetonoClientExtensionTest.php @@ -7,10 +7,13 @@ use Matthias\SymfonyDependencyInjectionTest\PhpUnit\AbstractExtensionTestCase; use Setono\ClientBundle\Context\ClientContextInterface; use Setono\ClientBundle\Context\DefaultClientContext; +use Setono\ClientBundle\Context\PendingMetadataClientContext; use Setono\ClientBundle\CookieProvider\RequestBasedCookieProvider; use Setono\ClientBundle\DependencyInjection\SetonoClientExtension; use Setono\ClientBundle\Entity\Metadata; use Setono\ClientBundle\EventListener\Doctrine\ConvertToEntityListener; +use Setono\ClientBundle\PendingMetadataStorage\CookieBasedPendingMetadataStorage; +use Setono\ClientBundle\PendingMetadataStorage\PendingMetadataStorageInterface; use Setono\ClientBundle\Tests\Unit\DependencyInjection\Fixture\CustomMetadata; use Setono\ClientBundle\Tests\Unit\DependencyInjection\Fixture\InterfaceOnlyMetadata; @@ -65,6 +68,47 @@ public function after_loading_the_services_are_registered(): void $this->assertContainerBuilderHasAlias(ClientContextInterface::class, 'setono_client.client_context.default'); } + /** + * @test + */ + public function after_loading_the_pending_metadata_services_are_registered(): void + { + $this->load(); + + $this->assertContainerBuilderHasService( + 'setono_client.pending_metadata_storage.cookie_based', + CookieBasedPendingMetadataStorage::class, + ); + $this->assertContainerBuilderHasServiceDefinitionWithArgument( + 'setono_client.pending_metadata_storage.cookie_based', + 0, + '%setono_client.cookie.name%_pending_metadata', + ); + $this->assertContainerBuilderHasServiceDefinitionWithArgument( + 'setono_client.pending_metadata_storage.cookie_based', + 2, + '%kernel.secret%', + ); + $this->assertContainerBuilderHasAlias( + 'setono_client.pending_metadata_storage.default', + 'setono_client.pending_metadata_storage.cookie_based', + ); + $this->assertContainerBuilderHasAlias( + PendingMetadataStorageInterface::class, + 'setono_client.pending_metadata_storage.default', + ); + + $this->assertContainerBuilderHasService( + 'setono_client.client_context.pending_metadata', + PendingMetadataClientContext::class, + ); + // between the cached (32) and the cookie based (64) client context + self::assertSame( + ['setono_client.client_context.default', null, 48], + $this->container->getDefinition('setono_client.client_context.pending_metadata')->getDecoratedService(), + ); + } + /** * @test */ diff --git a/tests/Unit/EventSubscriber/StoreCookieSubscriberTest.php b/tests/Unit/EventSubscriber/StoreCookieSubscriberTest.php index cc9ef5d..c514649 100644 --- a/tests/Unit/EventSubscriber/StoreCookieSubscriberTest.php +++ b/tests/Unit/EventSubscriber/StoreCookieSubscriberTest.php @@ -7,13 +7,17 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; +use Prophecy\Prophecy\ObjectProphecy; use Psr\EventDispatcher\EventDispatcherInterface; use Setono\Client\Client; use Setono\Client\Cookie; +use Setono\Client\Metadata; +use Setono\ClientBundle\Client\ChangeAwareMetadata; use Setono\ClientBundle\Context\ClientContextInterface; use Setono\ClientBundle\CookieProvider\CookieProviderInterface; use Setono\ClientBundle\Event\PreStoreCookieEvent; use Setono\ClientBundle\EventSubscriber\StoreCookieSubscriber; +use Setono\ClientBundle\PendingMetadataStorage\PendingMetadataStorageInterface; use Symfony\Component\EventDispatcher\EventDispatcher; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; @@ -27,12 +31,20 @@ final class StoreCookieSubscriberTest extends TestCase private const COOKIE_NAME = 'setono_client_id'; + /** @var ObjectProphecy */ + private ObjectProphecy $pendingMetadataStorage; + + protected function setUp(): void + { + $this->pendingMetadataStorage = $this->prophesize(PendingMetadataStorageInterface::class); + } + /** * @test */ public function it_subscribes_to_the_response_event(): void { - self::assertSame([KernelEvents::RESPONSE => 'store'], StoreCookieSubscriber::getSubscribedEvents()); + self::assertSame([KernelEvents::RESPONSE => ['store', -64]], StoreCookieSubscriber::getSubscribedEvents()); } /** @@ -41,15 +53,12 @@ public function it_subscribes_to_the_response_event(): void public function it_stores_a_new_cookie_when_none_exists(): void { $clientContext = $this->prophesize(ClientContextInterface::class); - $clientContext->getClient()->willReturn(new Client('client-id')); + $clientContext->getClient()->willReturn(new Client('client-id', new ChangeAwareMetadata(['foo' => 'bar']))); - $cookieProvider = $this->prophesize(CookieProviderInterface::class); - $cookieProvider->getCookie(Argument::type(Request::class))->willReturn(null); + $this->pendingMetadataStorage->remove(Argument::cetera())->shouldBeCalled(); + $this->pendingMetadataStorage->store(Argument::cetera())->shouldNotBeCalled(); - $response = new Response(); - $this->createSubscriber($clientContext->reveal(), $cookieProvider->reveal())->store( - $this->createResponseEvent(new Request(), $response), - ); + $response = $this->store($clientContext, null); $cookies = $response->headers->getCookies(); self::assertCount(1, $cookies); @@ -66,13 +75,7 @@ public function it_reuses_the_client_id_from_an_existing_cookie(): void $clientContext = $this->prophesize(ClientContextInterface::class); $clientContext->getClient()->shouldNotBeCalled(); - $cookieProvider = $this->prophesize(CookieProviderInterface::class); - $cookieProvider->getCookie(Argument::type(Request::class))->willReturn(new Cookie('existing-id')); - - $response = new Response(); - $this->createSubscriber($clientContext->reveal(), $cookieProvider->reveal())->store( - $this->createResponseEvent(new Request(), $response), - ); + $response = $this->store($clientContext, new Cookie('existing-id')); $cookies = $response->headers->getCookies(); self::assertCount(1, $cookies); @@ -84,16 +87,8 @@ public function it_reuses_the_client_id_from_an_existing_cookie(): void */ public function it_refreshes_the_last_seen_timestamp_and_keeps_the_first_seen_timestamp(): void { - $clientContext = $this->prophesize(ClientContextInterface::class); - - $cookieProvider = $this->prophesize(CookieProviderInterface::class); - $cookieProvider->getCookie(Argument::type(Request::class))->willReturn(new Cookie('existing-id', 2, 1_000, 2_000)); - $before = time(); - $response = new Response(); - $this->createSubscriber($clientContext->reveal(), $cookieProvider->reveal())->store( - $this->createResponseEvent(new Request(), $response), - ); + $response = $this->store($this->prophesize(ClientContextInterface::class), new Cookie('existing-id', 2, 1_000, 2_000)); $cookie = Cookie::fromString((string) $response->headers->getCookies()[0]->getValue()); self::assertSame(1_000, $cookie->firstSeenAt); @@ -115,8 +110,8 @@ public function it_uses_the_configured_cookie_name_and_expiration(): void $before = time(); $response = new Response(); $this->createSubscriber( - $clientContext->reveal(), - $cookieProvider->reveal(), + $clientContext, + $cookieProvider, cookieName: 'custom_name', cookieExpiration: '+30 days', )->store($this->createResponseEvent(new Request(), $response)); @@ -131,14 +126,105 @@ public function it_uses_the_configured_cookie_name_and_expiration(): void /** * @test */ - public function it_does_not_store_a_cookie_for_sub_requests(): void + public function it_removes_the_pending_metadata(): void + { + $request = new Request(); + $response = new Response(); + + $this->pendingMetadataStorage->remove($request, $response)->shouldBeCalledOnce(); + + $this->store($this->prophesize(ClientContextInterface::class), new Cookie('existing-id'), $request, $response); + } + + /** + * @test + */ + public function it_stores_the_changed_metadata_of_a_new_client_as_pending_metadata(): void + { + $request = new Request(); + $response = new Response(); + + $metadata = new ChangeAwareMetadata(); + $metadata->set('foo', 'bar'); + + $clientContext = $this->prophesize(ClientContextInterface::class); + $clientContext->getClient()->willReturn(new Client('client-id', $metadata)); + + $this->pendingMetadataStorage->remove($request, $response)->shouldBeCalled(); + $this->pendingMetadataStorage->store($request, $response, 'client-id', ['foo' => 'bar'])->willReturn(true)->shouldBeCalledOnce(); + + $this->store($clientContext, null, $request, $response); + + self::assertFalse($metadata->isDirty()); + } + + /** + * @test + */ + public function it_keeps_the_metadata_dirty_when_it_cannot_be_stored_as_pending_metadata(): void + { + $metadata = new ChangeAwareMetadata(); + $metadata->set('foo', 'bar'); + + $clientContext = $this->prophesize(ClientContextInterface::class); + $clientContext->getClient()->willReturn(new Client('client-id', $metadata)); + + $this->pendingMetadataStorage->remove(Argument::cetera())->shouldBeCalled(); + $this->pendingMetadataStorage->store(Argument::cetera())->willReturn(false)->shouldBeCalledOnce(); + + $this->store($clientContext, null); + + self::assertTrue($metadata->isDirty()); + } + + /** + * @test + */ + public function it_does_not_store_empty_pending_metadata(): void + { + $metadata = new ChangeAwareMetadata(); + $metadata->set('foo', 'bar'); + $metadata->remove('foo'); + + $clientContext = $this->prophesize(ClientContextInterface::class); + $clientContext->getClient()->willReturn(new Client('client-id', $metadata)); + + $this->pendingMetadataStorage->remove(Argument::cetera())->shouldBeCalled(); + $this->pendingMetadataStorage->store(Argument::cetera())->shouldNotBeCalled(); + + $this->store($clientContext, null); + + self::assertFalse($metadata->isDirty()); + } + + /** + * @test + */ + public function it_does_not_store_metadata_that_is_not_change_aware_as_pending_metadata(): void { $clientContext = $this->prophesize(ClientContextInterface::class); + $clientContext->getClient()->willReturn(new Client('client-id', new Metadata(['foo' => 'bar']))); + + $this->pendingMetadataStorage->remove(Argument::cetera())->shouldBeCalled(); + $this->pendingMetadataStorage->store(Argument::cetera())->shouldNotBeCalled(); + + $response = $this->store($clientContext, null); + + self::assertCount(1, $response->headers->getCookies()); + } + + /** + * @test + */ + public function it_does_not_store_a_cookie_for_sub_requests(): void + { $cookieProvider = $this->prophesize(CookieProviderInterface::class); $cookieProvider->getCookie(Argument::cetera())->shouldNotBeCalled(); + $this->pendingMetadataStorage->remove(Argument::cetera())->shouldNotBeCalled(); + $response = new Response(); - $this->createSubscriber($clientContext->reveal(), $cookieProvider->reveal())->store( + $this->createSubscriber($this->prophesize(ClientContextInterface::class), $cookieProvider)->store( $this->createResponseEvent(new Request(), $response, HttpKernelInterface::SUB_REQUEST), ); @@ -151,30 +237,86 @@ public function it_does_not_store_a_cookie_for_sub_requests(): void public function it_does_not_store_a_cookie_when_the_pre_store_event_vetoes_it(): void { $clientContext = $this->prophesize(ClientContextInterface::class); - $cookieProvider = $this->prophesize(CookieProviderInterface::class); - $cookieProvider->getCookie(Argument::cetera())->shouldNotBeCalled(); + $clientContext->getClient()->shouldNotBeCalled(); - $eventDispatcher = new EventDispatcher(); - $eventDispatcher->addListener(PreStoreCookieEvent::class, static function (PreStoreCookieEvent $event): void { - $event->store = false; - }); + $this->pendingMetadataStorage->remove(Argument::cetera())->shouldNotBeCalled(); - $response = new Response(); - $this->createSubscriber($clientContext->reveal(), $cookieProvider->reveal(), $eventDispatcher)->store( - $this->createResponseEvent(new Request(), $response), - ); + $response = $this->store($clientContext, new Cookie('existing-id'), eventDispatcher: self::createVetoingEventDispatcher()); + + self::assertCount(0, $response->headers->getCookies()); + } + + /** + * @test + */ + public function it_discards_the_metadata_of_a_new_client_when_the_pre_store_event_vetoes_the_cookie(): void + { + $metadata = new ChangeAwareMetadata(); + $metadata->set('foo', 'bar'); + + $clientContext = $this->prophesize(ClientContextInterface::class); + $clientContext->getClient()->willReturn(new Client('client-id', $metadata)); + + $this->pendingMetadataStorage->store(Argument::cetera())->shouldNotBeCalled(); + + $response = $this->store($clientContext, null, eventDispatcher: self::createVetoingEventDispatcher()); self::assertCount(0, $response->headers->getCookies()); + self::assertFalse($metadata->isDirty()); } + /** + * @test + */ + public function it_ignores_metadata_that_is_not_change_aware_when_the_pre_store_event_vetoes_the_cookie(): void + { + $clientContext = $this->prophesize(ClientContextInterface::class); + $clientContext->getClient()->willReturn(new Client('client-id', new Metadata(['foo' => 'bar'])))->shouldBeCalled(); + + $response = $this->store($clientContext, null, eventDispatcher: self::createVetoingEventDispatcher()); + + self::assertCount(0, $response->headers->getCookies()); + } + + /** + * @param ObjectProphecy $clientContext + */ + private function store( + ObjectProphecy $clientContext, + ?Cookie $cookie, + Request $request = new Request(), + Response $response = new Response(), + EventDispatcherInterface $eventDispatcher = new EventDispatcher(), + ): Response { + $cookieProvider = $this->prophesize(CookieProviderInterface::class); + $cookieProvider->getCookie($request)->willReturn($cookie); + + $this->createSubscriber($clientContext, $cookieProvider, $eventDispatcher)->store( + $this->createResponseEvent($request, $response), + ); + + return $response; + } + + /** + * @param ObjectProphecy $clientContext + * @param ObjectProphecy $cookieProvider + */ private function createSubscriber( - ClientContextInterface $clientContext, - CookieProviderInterface $cookieProvider, + ObjectProphecy $clientContext, + ObjectProphecy $cookieProvider, EventDispatcherInterface $eventDispatcher = new EventDispatcher(), string $cookieName = self::COOKIE_NAME, string $cookieExpiration = '+365 days', ): StoreCookieSubscriber { - return new StoreCookieSubscriber($clientContext, $cookieProvider, $eventDispatcher, $cookieName, $cookieExpiration); + return new StoreCookieSubscriber( + $clientContext->reveal(), + $cookieProvider->reveal(), + $this->pendingMetadataStorage->reveal(), + $eventDispatcher, + $cookieName, + $cookieExpiration, + ); } private function createResponseEvent( @@ -189,4 +331,14 @@ private function createResponseEvent( $response, ); } + + private static function createVetoingEventDispatcher(): EventDispatcher + { + $eventDispatcher = new EventDispatcher(); + $eventDispatcher->addListener(PreStoreCookieEvent::class, static function (PreStoreCookieEvent $event): void { + $event->store = false; + }); + + return $eventDispatcher; + } } diff --git a/tests/Unit/PendingMetadataStorage/CookieBasedPendingMetadataStorageTest.php b/tests/Unit/PendingMetadataStorage/CookieBasedPendingMetadataStorageTest.php new file mode 100644 index 0000000..29d511a --- /dev/null +++ b/tests/Unit/PendingMetadataStorage/CookieBasedPendingMetadataStorageTest.php @@ -0,0 +1,228 @@ +createStorage()->store(new Request(), $response, 'client-id', [ + 'url' => 'https://example.com/æ', + 'amount' => 1.0, + ])); + + $json = '{"url":"https://example.com/æ","amount":1.0}'; + $signature = hash_hmac('sha256', 'setono_client_pending_metadata.client-id.' . $json, self::SECRET); + + $cookie = self::getOnlyCookie($response); + self::assertSame(self::COOKIE_NAME, $cookie->getName()); + self::assertSame($json . '.' . $signature, $cookie->getValue()); + self::assertSame('/', $cookie->getPath()); + self::assertTrue($cookie->isHttpOnly()); + self::assertFalse($cookie->isSecure()); + self::assertSame(Cookie::SAMESITE_LAX, $cookie->getSameSite()); + self::assertEqualsWithDelta(strtotime('+365 days'), $cookie->getExpiresTime(), 5); + } + + /** + * @test + */ + public function it_stores_a_secure_cookie_on_secure_requests(): void + { + $response = new Response(); + + $this->createStorage()->store(Request::create('https://example.com/'), $response, 'client-id', ['foo' => 'bar']); + + self::assertTrue(self::getOnlyCookie($response)->isSecure()); + } + + /** + * @test + */ + public function it_returns_the_stored_metadata(): void + { + $metadata = ['foo' => 'bar', 'amount' => 1.0, '__expires' => ['foo' => 1893456000]]; + + $storage = $this->createStorage(); + $response = new Response(); + $storage->store(new Request(), $response, 'client-id', $metadata); + + self::assertSame($metadata, $storage->get(self::createRequestFromResponse($response), 'client-id')); + } + + /** + * @test + */ + public function it_returns_null_when_the_request_has_no_cookie(): void + { + self::assertNull($this->createStorage()->get(new Request(), 'client-id')); + } + + /** + * @test + */ + public function it_returns_null_when_the_cookie_is_not_signed(): void + { + $request = new Request(cookies: [self::COOKIE_NAME => '{"foo":"bar"}']); + + self::assertNull($this->createStorage()->get($request, 'client-id')); + } + + /** + * @test + */ + public function it_returns_null_when_the_metadata_has_been_tampered_with(): void + { + $response = new Response(); + $this->createStorage()->store(new Request(), $response, 'client-id', ['foo' => 'bar']); + + $value = str_replace('"bar"', '"baz"', (string) self::getOnlyCookie($response)->getValue()); + $request = new Request(cookies: [self::COOKIE_NAME => $value]); + + self::assertNull($this->createStorage()->get($request, 'client-id')); + } + + /** + * @test + */ + public function it_returns_null_when_the_metadata_belongs_to_another_client(): void + { + $storage = $this->createStorage(); + $response = new Response(); + $storage->store(new Request(), $response, 'another-client-id', ['foo' => 'bar']); + + self::assertNull($storage->get(self::createRequestFromResponse($response), 'client-id')); + } + + /** + * @test + */ + public function it_returns_null_when_the_metadata_was_signed_with_another_secret(): void + { + $response = new Response(); + (new CookieBasedPendingMetadataStorage(self::COOKIE_NAME, '+365 days', 'another-secret')) + ->store(new Request(), $response, 'client-id', ['foo' => 'bar']); + + self::assertNull($this->createStorage()->get(self::createRequestFromResponse($response), 'client-id')); + } + + /** + * @test + */ + public function it_does_not_store_metadata_that_cannot_be_encoded(): void + { + $response = new Response(); + + self::assertFalse($this->createStorage()->store(new Request(), $response, 'client-id', ['foo' => "\xB1\x31"])); + self::assertCount(0, $response->headers->getCookies()); + } + + /** + * @test + */ + public function it_stores_metadata_up_to_the_size_limit(): void + { + $response = new Response(); + + self::assertTrue($this->createStorage()->store(new Request(), $response, 'client-id', self::createMetadataOfLength(2048))); + self::assertSame(2048, strlen(rawurlencode((string) self::getOnlyCookie($response)->getValue()))); + } + + /** + * @test + */ + public function it_does_not_store_metadata_exceeding_the_size_limit(): void + { + $response = new Response(); + + self::assertFalse($this->createStorage()->store(new Request(), $response, 'client-id', self::createMetadataOfLength(2049))); + self::assertCount(0, $response->headers->getCookies()); + } + + /** + * @test + */ + public function it_removes_the_cookie(): void + { + $response = new Response(); + + $this->createStorage()->remove(new Request(cookies: [self::COOKIE_NAME => 'value']), $response); + + $cookie = self::getOnlyCookie($response); + self::assertSame(self::COOKIE_NAME, $cookie->getName()); + self::assertTrue($cookie->isCleared()); + } + + /** + * @test + */ + public function it_does_not_remove_the_cookie_when_the_request_has_none(): void + { + $response = new Response(); + + $this->createStorage()->remove(new Request(), $response); + + self::assertCount(0, $response->headers->getCookies()); + } + + /** + * @test + */ + public function storing_after_removing_replaces_the_removal(): void + { + $storage = $this->createStorage(); + $response = new Response(); + + $storage->remove(new Request(cookies: [self::COOKIE_NAME => 'value']), $response); + $storage->store(new Request(), $response, 'client-id', ['foo' => 'bar']); + + self::assertFalse(self::getOnlyCookie($response)->isCleared()); + } + + private function createStorage(): CookieBasedPendingMetadataStorage + { + return new CookieBasedPendingMetadataStorage(self::COOKIE_NAME, '+365 days', self::SECRET); + } + + /** + * Returns metadata whose URL encoded cookie value has the given length + * + * @return array + */ + private static function createMetadataOfLength(int $length): array + { + $signatureLength = 64; + + return ['k' => str_repeat('a', $length - strlen(rawurlencode('{"k":""}.')) - $signatureLength)]; + } + + private static function getOnlyCookie(Response $response): Cookie + { + $cookies = $response->headers->getCookies(); + self::assertCount(1, $cookies); + + return $cookies[0]; + } + + private static function createRequestFromResponse(Response $response): Request + { + return new Request(cookies: [self::COOKIE_NAME => self::getOnlyCookie($response)->getValue()]); + } +} From 09757acc7c3d0faf9f252c7c04dfad936a625881 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joachim=20L=C3=B8vgaard?= Date: Mon, 26 Oct 2026 09:45:15 +0100 Subject: [PATCH 2/6] Remove SensitiveParameter attribute, which doesn't exist on PHP 8.1 --- src/PendingMetadataStorage/CookieBasedPendingMetadataStorage.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/PendingMetadataStorage/CookieBasedPendingMetadataStorage.php b/src/PendingMetadataStorage/CookieBasedPendingMetadataStorage.php index 806b19f..4e21396 100644 --- a/src/PendingMetadataStorage/CookieBasedPendingMetadataStorage.php +++ b/src/PendingMetadataStorage/CookieBasedPendingMetadataStorage.php @@ -25,7 +25,6 @@ final class CookieBasedPendingMetadataStorage implements PendingMetadataStorageI public function __construct( private readonly string $cookieName, private readonly string $cookieExpiration, - #[\SensitiveParameter] private readonly string $secret, ) { } From 5f7badb5e17c9107f3abcbe63aff4150bd063a41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joachim=20L=C3=B8vgaard?= Date: Mon, 26 Oct 2026 13:02:45 +0100 Subject: [PATCH 3/6] Adapt the functional tests to the deferred metadata of new clients The functional tests from #13 expected the metadata of a new visitor in the database right after the request that set it. Tests that check what a request stored now make the browser a returning visitor first, and MetadataTest also checks that a new visitor's metadata is stored when it comes back. NewClientMetadataTest now runs in the test app against the database instead of wiring the services by hand with an in-memory entity manager. --- CLAUDE.md | 2 +- .../GoogleClickIdSubscriber.php | 49 ++++ tests/Functional/CustomMetadataClassTest.php | 1 + tests/Functional/FunctionalTestCase.php | 35 +++ tests/Functional/LongRunningProcessTest.php | 2 + tests/Functional/MetadataTest.php | 51 ++-- tests/Functional/NewClientMetadataTest.php | 225 +++--------------- 7 files changed, 149 insertions(+), 216 deletions(-) create mode 100644 tests/Functional/App/EventSubscriber/GoogleClickIdSubscriber.php diff --git a/CLAUDE.md b/CLAUDE.md index cd52f11..f34edc7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -60,7 +60,7 @@ A *new* client is one whose request has no id cookie. Its metadata is not persis - `StoreCookieSubscriber`, when the cookie is stored: always calls `remove()`, which clears the pending cookie if the request carried one (a `store()` in the same response replaces the clearing cookie). - `PendingMetadataClientContext` adopts the pending metadata on the return request (see above). - `CookieBasedPendingMetadataStorage` (default, alias `setono_client.pending_metadata_storage.default`): cookie `_pending_metadata`, value `.` signed with `%kernel.secret%` over a context string + client id + JSON, `HttpOnly`, `Secure` on HTTPS, same expiration as the id cookie, max 2048 bytes URL-encoded. JSON flags mirror Doctrine's json type (`JSON_PRESERVE_ZERO_FRACTION`). -- `tests/Functional/NewClientMetadataTest.php` runs the real services through `HttpKernel` with an in-memory Doctrine manager and shows the whole flow. +- `tests/Functional/NewClientMetadataTest.php` shows the whole flow in the test app, whose `GoogleClickIdSubscriber` sets metadata on `kernel.response` before (`gclid`) and after (`late_gclid`) `StoreCookieSubscriber`. Functional tests that check what a request stored call `FunctionalTestCase::becomeReturningVisitor()` first, because a new visitor's metadata only reaches the database when it comes back. ### Entity mapping diff --git a/tests/Functional/App/EventSubscriber/GoogleClickIdSubscriber.php b/tests/Functional/App/EventSubscriber/GoogleClickIdSubscriber.php new file mode 100644 index 0000000..a0326fd --- /dev/null +++ b/tests/Functional/App/EventSubscriber/GoogleClickIdSubscriber.php @@ -0,0 +1,49 @@ + [ + ['storeGoogleClickId'], + ['storeLateGoogleClickId', -128], + ], + ]; + } + + public function storeGoogleClickId(ResponseEvent $event): void + { + $this->store($event, 'gclid'); + } + + public function storeLateGoogleClickId(ResponseEvent $event): void + { + $this->store($event, 'late_gclid'); + } + + private function store(ResponseEvent $event, string $parameter): void + { + $gclid = $event->getRequest()->query->get($parameter); + if (null !== $gclid) { + $this->clientContext->getClient()->metadata->set('gclid', $gclid, 7_776_000); + } + } +} diff --git a/tests/Functional/CustomMetadataClassTest.php b/tests/Functional/CustomMetadataClassTest.php index cd1ba8a..690dec0 100644 --- a/tests/Functional/CustomMetadataClassTest.php +++ b/tests/Functional/CustomMetadataClassTest.php @@ -23,6 +23,7 @@ protected static function getEnvironment(): string */ public function it_stores_the_metadata_in_the_configured_entity(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); self::assertResponseIsSuccessful(); diff --git a/tests/Functional/FunctionalTestCase.php b/tests/Functional/FunctionalTestCase.php index 936e684..4f5497b 100644 --- a/tests/Functional/FunctionalTestCase.php +++ b/tests/Functional/FunctionalTestCase.php @@ -10,6 +10,7 @@ use Doctrine\Persistence\ManagerRegistry; use Setono\Client\Cookie; use Setono\ClientBundle\Tests\Functional\App\Kernel; +use Symfony\Bridge\Doctrine\Middleware\Debug\DebugDataHolder; use Symfony\Bundle\FrameworkBundle\KernelBrowser; use Symfony\Bundle\FrameworkBundle\Test\WebTestCase; @@ -42,6 +43,15 @@ protected static function getEnvironment(): string return 'test'; } + /** + * Gives the browser a client id, so from the next request on it's a returning visitor, whose metadata is persisted + * right away. The metadata of a new visitor isn't persisted until it comes back (see NewClientMetadataTest) + */ + protected function becomeReturningVisitor(): void + { + $this->browser->request('GET', '/'); + } + /** * Returns the cookie stored in the browser */ @@ -114,6 +124,31 @@ protected static function storeMetadata(string $clientId, array $metadata): void ]); } + /** + * Returns the SQL of the queries that the last request executed, as recorded by DoctrineBundle in debug mode + * + * @return list + */ + protected static function getExecutedQueries(): array + { + $debugDataHolder = self::getContainer()->get('test.doctrine.debug_data_holder'); + self::assertInstanceOf(DebugDataHolder::class, $debugDataHolder); + + $queries = []; + foreach ($debugDataHolder->getData() as $connectionQueries) { + self::assertIsArray($connectionQueries); + + foreach ($connectionQueries as $query) { + self::assertIsArray($query); + self::assertIsString($query['sql']); + + $queries[] = $query['sql']; + } + } + + return $queries; + } + protected static function getConnection(): Connection { return self::getEntityManager()->getConnection(); diff --git a/tests/Functional/LongRunningProcessTest.php b/tests/Functional/LongRunningProcessTest.php index f0bf992..6fcdd50 100644 --- a/tests/Functional/LongRunningProcessTest.php +++ b/tests/Functional/LongRunningProcessTest.php @@ -38,10 +38,12 @@ public function it_gives_each_new_visitor_its_own_client(): void */ public function it_does_not_give_the_metadata_of_a_visitor_to_the_next_visitor(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); $firstClientId = $this->getClientId(); $this->browser->getCookieJar()->clear(); + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=baz&value=qux'); $this->browser->request('GET', '/metadata'); diff --git a/tests/Functional/MetadataTest.php b/tests/Functional/MetadataTest.php index d574052..fb77850 100644 --- a/tests/Functional/MetadataTest.php +++ b/tests/Functional/MetadataTest.php @@ -6,7 +6,6 @@ use Doctrine\ORM\Tools\SchemaValidator; use Setono\Client\Metadata; -use Symfony\Bridge\Doctrine\Middleware\Debug\DebugDataHolder; /** * Not final, because CustomMetadataClassTest runs these tests with an application's own metadata entity @@ -27,19 +26,33 @@ public function it_does_not_store_anything_for_a_visitor_that_does_not_use_metad /** * @test */ - public function it_stores_the_metadata_of_a_new_visitor(): void + public function it_stores_the_metadata_of_a_returning_visitor(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); self::assertResponseIsSuccessful(); self::assertSame(['foo' => 'bar'], self::fetchStoredMetadata($this->getClientId())); } + /** + * @test + */ + public function it_stores_the_metadata_of_a_new_visitor_when_it_comes_back(): void + { + $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); + self::assertSame(0, self::countStoredMetadata()); + + $this->browser->request('GET', '/'); + self::assertSame(['foo' => 'bar'], self::fetchStoredMetadata($this->getClientId())); + } + /** * @test */ public function it_loads_the_stored_metadata_of_a_returning_visitor(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); $this->browser->request('GET', '/metadata'); @@ -53,6 +66,7 @@ public function it_loads_the_stored_metadata_of_a_returning_visitor(): void */ public function it_updates_the_stored_metadata_of_a_returning_visitor(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); $this->browser->request('GET', '/metadata/set?key=baz&value=qux'); $this->browser->request('GET', '/metadata/set?key=foo&value=changed'); @@ -67,6 +81,7 @@ public function it_updates_the_stored_metadata_of_a_returning_visitor(): void */ public function it_removes_metadata(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); $this->browser->request('GET', '/metadata/set?key=baz&value=qux'); @@ -82,10 +97,12 @@ public function it_removes_metadata(): void */ public function it_keeps_the_metadata_of_each_visitor_apart(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=first'); $firstClientId = $this->getClientId(); $this->browser->getCookieJar()->clear(); + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=second'); $secondClientId = $this->getClientId(); @@ -99,6 +116,7 @@ public function it_keeps_the_metadata_of_each_visitor_apart(): void */ public function it_does_not_query_the_database_when_the_metadata_is_not_used(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); $this->browser->request('GET', '/'); @@ -115,6 +133,8 @@ public function it_does_not_query_the_database_when_the_metadata_is_not_used(): */ public function it_keeps_the_expiry_of_metadata_between_requests(): void { + $this->becomeReturningVisitor(); + $before = time(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar&ttl=3600'); @@ -137,6 +157,7 @@ public function it_keeps_the_expiry_of_metadata_between_requests(): void */ public function it_removes_expired_metadata(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); $clientId = $this->getClientId(); @@ -157,6 +178,7 @@ public function it_removes_expired_metadata(): void */ public function it_keeps_the_row_of_a_concurrent_request_that_stored_it_first(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set-concurrently?key=foo&value=bar'); self::assertResponseIsSuccessful(); @@ -188,29 +210,4 @@ private function getResponseData(): array return $data; } - - /** - * Returns the SQL of the queries that the last request executed, as recorded by DoctrineBundle in debug mode - * - * @return list - */ - private static function getExecutedQueries(): array - { - $debugDataHolder = self::getContainer()->get('test.doctrine.debug_data_holder'); - self::assertInstanceOf(DebugDataHolder::class, $debugDataHolder); - - $queries = []; - foreach ($debugDataHolder->getData() as $connectionQueries) { - self::assertIsArray($connectionQueries); - - foreach ($connectionQueries as $query) { - self::assertIsArray($query); - self::assertIsString($query['sql']); - - $queries[] = $query['sql']; - } - } - - return $queries; - } } diff --git a/tests/Functional/NewClientMetadataTest.php b/tests/Functional/NewClientMetadataTest.php index 215b0b3..e14359d 100644 --- a/tests/Functional/NewClientMetadataTest.php +++ b/tests/Functional/NewClientMetadataTest.php @@ -4,113 +4,31 @@ namespace Setono\ClientBundle\Tests\Functional; -use Doctrine\ORM\EntityManagerInterface; -use Doctrine\Persistence\ManagerRegistry; -use PHPUnit\Framework\TestCase; -use Prophecy\Argument; -use Prophecy\PhpUnit\ProphecyTrait; -use Setono\Client\Cookie; -use Setono\ClientBundle\Context\CachedClientContext; -use Setono\ClientBundle\Context\CookieBasedClientContext; -use Setono\ClientBundle\Context\DefaultClientContext; -use Setono\ClientBundle\Context\PendingMetadataClientContext; -use Setono\ClientBundle\CookieProvider\CachedCookieProvider; -use Setono\ClientBundle\CookieProvider\RequestBasedCookieProvider; -use Setono\ClientBundle\Entity\Metadata as MetadataEntity; -use Setono\ClientBundle\Event\PreStoreCookieEvent; -use Setono\ClientBundle\EventSubscriber\StoreCookieSubscriber; -use Setono\ClientBundle\EventSubscriber\StoreMetadataSubscriber; -use Setono\ClientBundle\MetadataPersister\DoctrineOrmBasedMetadataPersister; -use Setono\ClientBundle\MetadataProvider\DoctrineOrmBasedMetadataProvider; -use Setono\ClientBundle\MetadataProvider\EmptyMetadataProvider; -use Setono\ClientBundle\PendingMetadataStorage\CookieBasedPendingMetadataStorage; -use Symfony\Component\EventDispatcher\EventDispatcher; -use Symfony\Component\HttpFoundation\Request; -use Symfony\Component\HttpFoundation\RequestStack; -use Symfony\Component\HttpFoundation\Response; -use Symfony\Component\HttpKernel\Controller\ArgumentResolver; -use Symfony\Component\HttpKernel\Controller\ControllerResolver; -use Symfony\Component\HttpKernel\Event\ResponseEvent; -use Symfony\Component\HttpKernel\HttpKernel; -use Symfony\Component\HttpKernel\KernelEvents; +use Setono\Client\Metadata; +use Symfony\Component\HttpFoundation\Cookie; /** - * Runs the bundle's services through the HTTP kernel to show how the metadata of a new client travels + * Shows how the metadata of a new client travels: it's stored in the pending metadata cookie, and persisted when the + * client comes back with it. The test application's GoogleClickIdSubscriber sets the metadata */ -final class NewClientMetadataTest extends TestCase +final class NewClientMetadataTest extends FunctionalTestCase { - use ProphecyTrait; - - private const COOKIE_NAME = 'setono_client_id'; - private const PENDING_METADATA_COOKIE_NAME = 'setono_client_id_pending_metadata'; - /** @var \ArrayObject */ - private \ArrayObject $database; - - /** @var \ArrayObject */ - private \ArrayObject $databaseCalls; - - private ManagerRegistry $managerRegistry; - - /** - * The cookies of the browser used in the tests - * - * @var array - */ - private array $cookieJar = []; - - protected function setUp(): void - { - /** @var \ArrayObject $database */ - $database = new \ArrayObject(); - /** @var \ArrayObject $databaseCalls */ - $databaseCalls = new \ArrayObject(); - $this->database = $database; - $this->databaseCalls = $databaseCalls; - - $manager = $this->prophesize(EntityManagerInterface::class); - $manager->find(MetadataEntity::class, Argument::type('string'))->will( - static function (array $arguments) use ($database, $databaseCalls): ?MetadataEntity { - $databaseCalls->append('find'); - - /** @var string $clientId */ - $clientId = $arguments[1]; - - return $database[$clientId] ?? null; - }, - ); - $manager->persist(Argument::type(MetadataEntity::class))->will( - static function (array $arguments) use ($database, $databaseCalls): void { - $databaseCalls->append('persist'); - - /** @var MetadataEntity $entity */ - $entity = $arguments[0]; - $database[(string) $entity->getClientId()] = $entity; - }, - ); - $manager->flush()->will(static function () use ($databaseCalls): void { - $databaseCalls->append('flush'); - }); - - $managerRegistry = $this->prophesize(ManagerRegistry::class); - $managerRegistry->getManagerForClass(MetadataEntity::class)->willReturn($manager->reveal()); - - $this->managerRegistry = $managerRegistry->reveal(); - } - /** * @test */ public function it_does_not_touch_the_database_for_a_new_client_that_never_comes_back(): void { - $response = $this->handle(Request::create('/?gclid=abc')); + $this->browser->request('GET', '/?gclid=abc'); + self::assertResponseIsSuccessful(); self::assertEqualsCanonicalizing( [self::COOKIE_NAME, self::PENDING_METADATA_COOKIE_NAME], - array_map(static fn ($cookie) => $cookie->getName(), $response->headers->getCookies()), + array_map(static fn (Cookie $cookie): string => $cookie->getName(), $this->browser->getResponse()->headers->getCookies()), ); - self::assertSame([], $this->databaseCalls->getArrayCopy()); + self::assertSame([], self::getExecutedQueries()); + self::assertSame(0, self::countStoredMetadata()); } /** @@ -118,31 +36,27 @@ public function it_does_not_touch_the_database_for_a_new_client_that_never_comes */ public function it_persists_the_metadata_when_the_new_client_comes_back(): void { - $this->browse('/?gclid=abc'); - self::assertSame([], $this->databaseCalls->getArrayCopy()); + $this->browser->request('GET', '/?gclid=abc'); + self::assertSame(0, self::countStoredMetadata()); - $response = $this->browse('/'); + $this->browser->request('GET', '/'); - $clientId = Cookie::fromString($this->cookieJar[self::COOKIE_NAME])->clientId; - $entity = $this->database[$clientId] ?? null; - self::assertNotNull($entity); - - $metadata = $entity->getMetadata(); + $metadata = self::fetchStoredMetadata($this->getClientId()); + self::assertIsArray($metadata); self::assertSame('abc', $metadata['gclid']); - self::assertArrayHasKey('__expires', $metadata); + self::assertArrayHasKey(Metadata::EXPIRES_KEY, $metadata); $pendingMetadataCookies = array_values(array_filter( - $response->headers->getCookies(), - static fn ($cookie) => $cookie->getName() === self::PENDING_METADATA_COOKIE_NAME, + $this->browser->getResponse()->headers->getCookies(), + static fn (Cookie $cookie): bool => self::PENDING_METADATA_COOKIE_NAME === $cookie->getName(), )); self::assertCount(1, $pendingMetadataCookies); self::assertTrue($pendingMetadataCookies[0]->isCleared()); - self::assertArrayNotHasKey(self::PENDING_METADATA_COOKIE_NAME, $this->cookieJar); + self::assertNull($this->browser->getCookieJar()->get(self::PENDING_METADATA_COOKIE_NAME)); // from now on, the metadata is only loaded when it's used - $databaseCalls = count($this->databaseCalls); - $this->browse('/'); - self::assertCount($databaseCalls, $this->databaseCalls); + $this->browser->request('GET', '/'); + self::assertSame([], self::getExecutedQueries()); } /** @@ -150,10 +64,12 @@ public function it_persists_the_metadata_when_the_new_client_comes_back(): void */ public function it_persists_the_metadata_right_away_when_it_does_not_fit_in_the_pending_metadata_cookie(): void { - $this->browse('/?gclid=' . str_repeat('a', 2048)); + $value = str_repeat('a', 2048); + + $this->browser->request('GET', '/metadata/set?key=foo&value=' . $value); - self::assertArrayNotHasKey(self::PENDING_METADATA_COOKIE_NAME, $this->cookieJar); - self::assertCount(1, $this->database); + self::assertResponseNotHasCookie(self::PENDING_METADATA_COOKIE_NAME); + self::assertSame(['foo' => $value], self::fetchStoredMetadata($this->getClientId())); } /** @@ -161,9 +77,13 @@ public function it_persists_the_metadata_right_away_when_it_does_not_fit_in_the_ */ public function it_persists_metadata_set_after_the_cookie_was_stored_right_away(): void { - $this->browse('/?gclid=abc', metadataListenerPriority: -128); + $this->browser->request('GET', '/?late_gclid=abc'); + + self::assertResponseNotHasCookie(self::PENDING_METADATA_COOKIE_NAME); - self::assertCount(1, $this->database); + $metadata = self::fetchStoredMetadata($this->getClientId()); + self::assertIsArray($metadata); + self::assertSame('abc', $metadata['gclid']); } /** @@ -171,82 +91,11 @@ public function it_persists_metadata_set_after_the_cookie_was_stored_right_away( */ public function it_does_not_persist_the_metadata_of_a_new_client_that_does_not_get_a_cookie(): void { - $response = $this->handle(Request::create('/?gclid=abc'), storeCookie: false); - - self::assertCount(0, $response->headers->getCookies()); - self::assertSame([], $this->databaseCalls->getArrayCopy()); - } - - /** - * Sends a request with the cookies of the browser and updates them with the cookies of the response - */ - private function browse(string $uri, int $metadataListenerPriority = 0): Response - { - $response = $this->handle(Request::create($uri, cookies: $this->cookieJar), $metadataListenerPriority); - - foreach ($response->headers->getCookies() as $cookie) { - if ($cookie->isCleared()) { - unset($this->cookieJar[$cookie->getName()]); - } else { - $this->cookieJar[$cookie->getName()] = (string) $cookie->getValue(); - } - } - - return $response; - } - - private function handle(Request $request, int $metadataListenerPriority = 0, bool $storeCookie = true): Response - { - $requestStack = new RequestStack(); - $eventDispatcher = new EventDispatcher(); - - $cookieProvider = new CachedCookieProvider( - new RequestBasedCookieProvider($requestStack, self::COOKIE_NAME), - $requestStack, - ); - $pendingMetadataStorage = new CookieBasedPendingMetadataStorage( - self::PENDING_METADATA_COOKIE_NAME, - '+365 days', - 'secret', - ); - $clientContext = new CachedClientContext(new PendingMetadataClientContext( - new CookieBasedClientContext( - new DefaultClientContext(), - new DoctrineOrmBasedMetadataProvider(new EmptyMetadataProvider(), $this->managerRegistry, MetadataEntity::class), - $cookieProvider, - ), - $pendingMetadataStorage, - $requestStack, - )); - - $eventDispatcher->addSubscriber(new StoreCookieSubscriber( - $clientContext, - $cookieProvider, - $pendingMetadataStorage, - $eventDispatcher, - self::COOKIE_NAME, - '+365 days', - )); - $eventDispatcher->addSubscriber(new StoreMetadataSubscriber( - $clientContext, - new DoctrineOrmBasedMetadataPersister($this->managerRegistry, MetadataEntity::class), - )); - - // Like Setono/SyliusGoogleAdsPlugin, the application stores the Google click id on the response - $eventDispatcher->addListener(KernelEvents::RESPONSE, static function (ResponseEvent $event) use ($clientContext): void { - $gclid = $event->getRequest()->query->get('gclid'); - if (null !== $gclid) { - $clientContext->getClient()->metadata->set('gclid', $gclid, 7_776_000); - } - }, $metadataListenerPriority); - - $eventDispatcher->addListener(PreStoreCookieEvent::class, static function (PreStoreCookieEvent $event) use ($storeCookie): void { - $event->store = $storeCookie; - }); - - $request->attributes->set('_controller', static fn (): Response => new Response()); + $this->browser->request('GET', '/?gclid=abc&skip_cookie=1'); - return (new HttpKernel($eventDispatcher, new ControllerResolver(), $requestStack, new ArgumentResolver())) - ->handle($request); + self::assertResponseIsSuccessful(); + self::assertCount(0, $this->browser->getResponse()->headers->getCookies()); + self::assertSame([], self::getExecutedQueries()); + self::assertSame(0, self::countStoredMetadata()); } } From ade13888f04270cf1bafc3c8bf378ede48175239 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joachim=20L=C3=B8vgaard?= Date: Mon, 26 Oct 2026 13:20:34 +0100 Subject: [PATCH 4/6] Ignore signed pending metadata that doesn't decode to an array Only someone who knows the secret can sign such a value, but get() would throw a TypeError on it, e.g. for a JSON string or number. --- .../CookieBasedPendingMetadataStorage.php | 8 ++++- .../CookieBasedPendingMetadataStorageTest.php | 33 +++++++++++++++++-- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/PendingMetadataStorage/CookieBasedPendingMetadataStorage.php b/src/PendingMetadataStorage/CookieBasedPendingMetadataStorage.php index 4e21396..997a926 100644 --- a/src/PendingMetadataStorage/CookieBasedPendingMetadataStorage.php +++ b/src/PendingMetadataStorage/CookieBasedPendingMetadataStorage.php @@ -46,9 +46,15 @@ public function get(Request $request, string $clientId): ?array return null; } - /** @var array $metadata */ + /** @var array|scalar|null $metadata */ $metadata = json_decode($json, true); + // The signature proves that store() encoded the metadata, so this only fails for someone who knows the secret. + // Invalid JSON decodes to null too + if (!is_array($metadata)) { + return null; + } + return $metadata; } diff --git a/tests/Unit/PendingMetadataStorage/CookieBasedPendingMetadataStorageTest.php b/tests/Unit/PendingMetadataStorage/CookieBasedPendingMetadataStorageTest.php index 29d511a..309f9cd 100644 --- a/tests/Unit/PendingMetadataStorage/CookieBasedPendingMetadataStorageTest.php +++ b/tests/Unit/PendingMetadataStorage/CookieBasedPendingMetadataStorageTest.php @@ -29,11 +29,10 @@ public function it_stores_the_metadata_as_signed_json(): void ])); $json = '{"url":"https://example.com/æ","amount":1.0}'; - $signature = hash_hmac('sha256', 'setono_client_pending_metadata.client-id.' . $json, self::SECRET); $cookie = self::getOnlyCookie($response); self::assertSame(self::COOKIE_NAME, $cookie->getName()); - self::assertSame($json . '.' . $signature, $cookie->getValue()); + self::assertSame($json . '.' . self::sign('client-id', $json), $cookie->getValue()); self::assertSame('/', $cookie->getPath()); self::assertTrue($cookie->isHttpOnly()); self::assertFalse($cookie->isSecure()); @@ -123,6 +122,31 @@ public function it_returns_null_when_the_metadata_was_signed_with_another_secret self::assertNull($this->createStorage()->get(self::createRequestFromResponse($response), 'client-id')); } + /** + * Only someone who knows the secret can sign such a value + * + * @test + * + * @dataProvider provideSignedValuesThatAreNotMetadata + */ + public function it_returns_null_when_the_signed_value_does_not_decode_to_an_array(string $json): void + { + $request = new Request(cookies: [self::COOKIE_NAME => $json . '.' . self::sign('client-id', $json)]); + + self::assertNull($this->createStorage()->get($request, 'client-id')); + } + + /** + * @return iterable + */ + public static function provideSignedValuesThatAreNotMetadata(): iterable + { + yield 'string' => ['"foo"']; + yield 'integer' => ['1']; + yield 'null' => ['null']; + yield 'invalid JSON' => ['{"foo":']; + } + /** * @test */ @@ -213,6 +237,11 @@ private static function createMetadataOfLength(int $length): array return ['k' => str_repeat('a', $length - strlen(rawurlencode('{"k":""}.')) - $signatureLength)]; } + private static function sign(string $clientId, string $json): string + { + return hash_hmac('sha256', 'setono_client_pending_metadata.' . $clientId . '.' . $json, self::SECRET); + } + private static function getOnlyCookie(Response $response): Cookie { $cookies = $response->headers->getCookies(); From 0bc0a58ee9d3a8335b105ffe47d9373634596541 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joachim=20L=C3=B8vgaard?= Date: Mon, 26 Oct 2026 13:29:27 +0100 Subject: [PATCH 5/6] Merge the pending metadata into the metadata the client already has The pending metadata was ignored when the client had any metadata. But the application may persist metadata for a new client before it comes back, e.g. in a message handler that the first request dispatched, and then the pending metadata was lost. Pending keys that the client's metadata lacks are now added with their expiry timestamps. Keys the client has win, because a row that holds them was written from metadata that already held the pending metadata, or after it. --- CLAUDE.md | 2 +- README.md | 2 + src/Context/PendingMetadataClientContext.php | 33 +++++++++-- tests/Functional/FunctionalTestCase.php | 15 +++++ tests/Functional/NewClientMetadataTest.php | 20 +++++++ .../PendingMetadataClientContextTest.php | 57 +++++++++++++++++-- 6 files changed, 120 insertions(+), 9 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f34edc7..5bfec34 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -37,7 +37,7 @@ Three concerns are each built as a **Symfony service-decoration chain** in `conf - **Client context** — runtime order outermost→innermost: `CachedClientContext` (prio 32) → `PendingMetadataClientContext` (prio 48) → `CookieBasedClientContext` (prio 64) → `DefaultClientContext`. - `Cached` memoizes the `Client` for the request. It implements `ResetInterface` and is tagged `kernel.reset`, so long-running runtimes (FrankenPHP worker mode, RoadRunner) don't hand one visitor's client to the next request; `tests/Functional/LongRunningProcessTest.php` covers this. Any new service that holds per-request state needs the same, unless it keys the state by the request object like `CachedCookieProvider`. - - `PendingMetadata`: if the main request carries pending metadata signed for the client's id and the client has no metadata yet (`count()` initializes the lazy ghost), returns a new `Client` with that metadata in a `ChangeAwareMetadata` marked dirty, so the persister writes it. + - `PendingMetadata`: if the main request carries pending metadata signed for the client's id and it has keys (not expired) that the client's metadata lacks (`toArray()` initializes the lazy ghost), returns a new `Client` whose `ChangeAwareMetadata` holds the client's metadata plus those keys with their expiry timestamps, marked dirty so the persister writes it. Keys the client has win, because a stored row was written from metadata that already held the pending metadata (e.g. by a concurrent request) or by the app outside the client's requests. - `CookieBased` reads the cookie; if present, builds `Client(cookieId, metadataProvider->getMetadata(...))`; otherwise delegates. - `Default` returns an anonymous `new Client(null, new ChangeAwareMetadata())`. - **Cookie provider** — `CachedCookieProvider` (prio 32, memoizes per `Request` in a `WeakMap` keyed by the request object, so it needs no reset; not `spl_object_hash()`, which PHP hands to a new request once the old one is freed) → `RequestBasedCookieProvider` (parses the cookie off the request). The test kernel keeps its requests alive, so only `CachedCookieProviderTest` can reproduce that handle reuse. diff --git a/README.md b/README.md index 6737ceb..49784db 100644 --- a/README.md +++ b/README.md @@ -186,6 +186,8 @@ Things to be aware of: browser's developer tools until it is persisted. Changing `kernel.secret` discards pending metadata that hasn't been persisted yet. - Metadata that doesn't fit in about 2 KB, or can't be JSON encoded, is persisted right away. +- If the database already holds metadata for the visitor when it comes back, e.g. because a message handler stored + some, the pending metadata is merged into it. Where both hold a key, the database wins. - The metadata of a new client is captured when the cookie is stored (on `kernel.response` with priority `-64`). Set it before that, e.g. on `kernel.request`, in a controller, or in a `kernel.response` listener with the default priority. Metadata set later is persisted right away. diff --git a/src/Context/PendingMetadataClientContext.php b/src/Context/PendingMetadataClientContext.php index 4fe3277..fcf76c4 100644 --- a/src/Context/PendingMetadataClientContext.php +++ b/src/Context/PendingMetadataClientContext.php @@ -5,6 +5,7 @@ namespace Setono\ClientBundle\Context; use Setono\Client\Client; +use Setono\Client\Metadata; use Setono\ClientBundle\Client\ChangeAwareMetadata; use Setono\ClientBundle\PendingMetadataStorage\PendingMetadataStorageInterface; use Symfony\Component\HttpFoundation\RequestStack; @@ -32,16 +33,40 @@ public function getClient(): Client } $pendingMetadata = $this->pendingMetadataStorage->get($request, $client->id); + if (null === $pendingMetadata) { + return $client; + } - // If the client already has metadata (e.g. because a concurrent request persisted the pending metadata), - // that metadata is at least as new as the pending metadata - if (null === $pendingMetadata || count($client->metadata) > 0) { + [$data, $expires] = self::split($client->metadata); + [$pendingData, $pendingExpires] = self::split(new Metadata($pendingMetadata)); + + // A key the client already has wins, because it was persisted from metadata that held the pending metadata + // (e.g. by a concurrent request), so it's at least as new. Other keys, e.g. persisted by the application + // outside the client's requests, are kept + $missingData = array_diff_key($pendingData, $data); + if ([] === $missingData) { return $client; } - $metadata = new ChangeAwareMetadata($pendingMetadata); + $metadata = new ChangeAwareMetadata($data + $missingData + [ + Metadata::EXPIRES_KEY => $expires + array_intersect_key($pendingExpires, $missingData), + ]); $metadata->markDirty(); // the pending metadata isn't persisted yet return new Client($client->id, $metadata); } + + /** + * @return array{array, array} the data and the expiry timestamps of the metadata + */ + private static function split(Metadata $metadata): array + { + $data = $metadata->toArray(); + + /** @var array $expires */ + $expires = $data[Metadata::EXPIRES_KEY] ?? []; + unset($data[Metadata::EXPIRES_KEY]); + + return [$data, $expires]; + } } diff --git a/tests/Functional/FunctionalTestCase.php b/tests/Functional/FunctionalTestCase.php index 4f5497b..de1016d 100644 --- a/tests/Functional/FunctionalTestCase.php +++ b/tests/Functional/FunctionalTestCase.php @@ -149,6 +149,21 @@ protected static function getExecutedQueries(): array return $queries; } + /** + * Inserts the client's row, like an application does when it persists metadata outside the client's requests + * + * @param array $metadata + */ + protected static function insertMetadata(string $clientId, array $metadata): void + { + $classMetadata = self::getMetadataClassMetadata(); + + self::getConnection()->insert($classMetadata->getTableName(), [ + $classMetadata->getColumnName('clientId') => $clientId, + $classMetadata->getColumnName('metadata') => json_encode($metadata, \JSON_THROW_ON_ERROR), + ]); + } + protected static function getConnection(): Connection { return self::getEntityManager()->getConnection(); diff --git a/tests/Functional/NewClientMetadataTest.php b/tests/Functional/NewClientMetadataTest.php index e14359d..390e144 100644 --- a/tests/Functional/NewClientMetadataTest.php +++ b/tests/Functional/NewClientMetadataTest.php @@ -59,6 +59,26 @@ public function it_persists_the_metadata_when_the_new_client_comes_back(): void self::assertSame([], self::getExecutedQueries()); } + /** + * @test + */ + public function it_merges_the_metadata_into_metadata_persisted_before_the_new_client_came_back(): void + { + $this->browser->request('GET', '/?gclid=abc'); + + // e.g. by a message handler that the first request dispatched + self::insertMetadata($this->getClientId(), ['country' => 'DK']); + + $this->browser->request('GET', '/'); + + $metadata = self::fetchStoredMetadata($this->getClientId()); + self::assertIsArray($metadata); + self::assertSame('DK', $metadata['country']); + self::assertSame('abc', $metadata['gclid']); + self::assertIsArray($metadata[Metadata::EXPIRES_KEY]); + self::assertSame(['gclid'], array_keys($metadata[Metadata::EXPIRES_KEY])); + } + /** * @test */ diff --git a/tests/Unit/Context/PendingMetadataClientContextTest.php b/tests/Unit/Context/PendingMetadataClientContextTest.php index 18fc8ef..58dc828 100644 --- a/tests/Unit/Context/PendingMetadataClientContextTest.php +++ b/tests/Unit/Context/PendingMetadataClientContextTest.php @@ -79,18 +79,67 @@ public function it_hands_the_pending_metadata_to_a_client_without_metadata(): vo /** * @test */ - public function it_ignores_the_pending_metadata_when_the_client_has_metadata(): void + public function it_returns_the_decorated_client_when_its_metadata_has_all_pending_keys(): void { - $decoratedClient = new Client('client-id', new Metadata(['foo' => 'persisted'])); + $decoratedClient = new Client('client-id', new Metadata(['foo' => 'persisted', 'bar' => 'baz'])); $request = new Request(); $storage = $this->prophesize(PendingMetadataStorageInterface::class); - $storage->get($request, 'client-id')->willReturn(['foo' => 'pending']); + $storage->get($request, 'client-id')->willReturn(['foo' => 'pending', '__expires' => ['foo' => 1893456000]]); $context = $this->createContext($decoratedClient, $storage->reveal(), self::createRequestStack($request)); self::assertSame($decoratedClient, $context->getClient()); - self::assertSame(['foo' => 'persisted'], $decoratedClient->metadata->toArray()); + self::assertSame(['foo' => 'persisted', 'bar' => 'baz'], $decoratedClient->metadata->toArray()); + } + + /** + * @test + */ + public function it_does_not_add_expired_pending_metadata(): void + { + $decoratedClient = new Client('client-id', new Metadata(['bar' => 'baz'])); + $request = new Request(); + + $storage = $this->prophesize(PendingMetadataStorageInterface::class); + $storage->get($request, 'client-id')->willReturn(['foo' => 'pending', '__expires' => ['foo' => time() - 1]]); + + $context = $this->createContext($decoratedClient, $storage->reveal(), self::createRequestStack($request)); + + self::assertSame($decoratedClient, $context->getClient()); + } + + /** + * @test + */ + public function it_adds_the_pending_metadata_that_the_client_does_not_have(): void + { + $request = new Request(); + + $storage = $this->prophesize(PendingMetadataStorageInterface::class); + $storage->get($request, 'client-id')->willReturn([ + 'foo' => 'pending', + 'qux' => 'pending', + '__expires' => ['foo' => 1893456000, 'qux' => 1893456001], + ]); + + $context = $this->createContext( + new Client('client-id', new Metadata(['foo' => 'persisted', 'bar' => 'baz', '__expires' => ['bar' => 1893456002]])), + $storage->reveal(), + self::createRequestStack($request), + ); + + $client = $context->getClient(); + + self::assertSame('client-id', $client->id); + self::assertInstanceOf(ChangeAwareMetadata::class, $client->metadata); + self::assertTrue($client->metadata->isDirty()); + self::assertSame([ + 'foo' => 'persisted', + 'bar' => 'baz', + 'qux' => 'pending', + '__expires' => ['bar' => 1893456002, 'qux' => 1893456001], + ], $client->metadata->toArray()); } private function createContext( From 3c4665aa1a53c063628d9fca66691e87ea92df99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joachim=20L=C3=B8vgaard?= Date: Mon, 26 Oct 2026 13:44:33 +0100 Subject: [PATCH 6/6] Make the pending metadata storage an optional, last constructor argument Adding it as the third argument of StoreCookieSubscriber's constructor broke backwards compatibility. Like Symfony does, it's now an optional last argument, and leaving it out triggers a deprecation. Without it, the metadata of a new client is persisted right away, like before. --- CLAUDE.md | 3 +- composer.json | 1 + config/services.xml | 2 +- src/EventSubscriber/StoreCookieSubscriber.php | 18 ++++- .../StoreCookieSubscriberTest.php | 70 ++++++++++++++++++- 5 files changed, 88 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 5bfec34..8b6bac1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -23,7 +23,7 @@ PHP 8.1+ is required (CI runs 8.1 and 8.2). Use the `8.1`/`8.2` shell switchers - Dependency analysis: `vendor/bin/composer-dependency-analyser` (no composer script; see `composer-dependency-analyser.php`) - Rector (dry-run): `vendor/bin/rector process --dry-run` (config in `rector.php`, `withPhpSets(php81: true)`; not run in CI) -CI (`.github/workflows/build.yaml`) additionally runs `composer validate --strict` and `composer normalize --dry-run` across the PHP 8.1/8.2 × Symfony 6.4/7.0 matrix. +CI (`.github/workflows/build.yaml`) additionally runs `composer validate --strict` and `composer normalize --dry-run` across the PHP 8.1/8.2 × Symfony 6.4/7.0 matrix. Pull requests also run Roave's BackwardCompatibilityCheck against their base branch (`.github/workflows/backwards-compatibility-check.yaml`). > Local note (PHP 8.4): the transitive dev dep `thecodingmachine/safe` emits a flood of implicit-nullable `E_DEPRECATED` notices on PHP 8.4 (they don't exist on CI's 8.1/8.2). This is harmless for PHPStan/PHPUnit but breaks Infection's initial test run — run it with `vendor/bin/infection --initial-tests-php-options="-d error_reporting=24575"` locally on 8.4. @@ -82,4 +82,5 @@ setono_client: - Strict types everywhere (`declare(strict_types=1)`); concrete classes are `final`, except the metadata/entity classes (`ChangeAwareMetadata`, `LazyChangeAwareMetadata`, `Entity\Metadata`) which are intentionally extensible. - New services go in `config/services.xml` using the `setono_client..` id scheme, with a `setono_client..default` alias and an interface alias for autowiring. To insert behavior into a chain, add a decorator with an appropriate `decoration-priority` rather than editing existing classes. +- Public constructors are covered by the BC check, also of `final` classes. Like Symfony, add a new constructor argument at the end as an optional one and `trigger_deprecation('setono/client-bundle', '', 'Not passing an instance of "%s" as "$arg" to "%s()" is deprecated, the argument will be required in .', …)` when it's missing (see `StoreCookieSubscriber`, where a missing pending metadata storage means the metadata of a new client is persisted right away). - Tests use `matthiasnoback/symfony-dependency-injection-test` (e.g. `AbstractExtensionTestCase`), `matthiasnoback/symfony-config-test` (`ConfigurationTestCaseTrait`) and Prophecy (`ProphecyTrait`), with the `@test` annotation and snake_case method-name style. Keep the suite at 100% MSI — when adding logic, add a test that kills the corresponding mutant. diff --git a/composer.json b/composer.json index 733e184..24abd29 100644 --- a/composer.json +++ b/composer.json @@ -20,6 +20,7 @@ "setono/doctrine-orm-trait": "^1.1", "symfony/config": "^6.4 || ^7.0", "symfony/dependency-injection": "^6.4 || ^7.0", + "symfony/deprecation-contracts": "^2.5 || ^3.0", "symfony/event-dispatcher": "^6.4 || ^7.0", "symfony/http-foundation": "^6.4 || ^7.0", "symfony/http-kernel": "^6.4 || ^7.0", diff --git a/config/services.xml b/config/services.xml index 1218c38..6a13c58 100644 --- a/config/services.xml +++ b/config/services.xml @@ -69,10 +69,10 @@ class="Setono\ClientBundle\EventSubscriber\StoreCookieSubscriber"> - %setono_client.cookie.name% %setono_client.cookie.expiration% + diff --git a/src/EventSubscriber/StoreCookieSubscriber.php b/src/EventSubscriber/StoreCookieSubscriber.php index 0e10c34..e0ff4ed 100644 --- a/src/EventSubscriber/StoreCookieSubscriber.php +++ b/src/EventSubscriber/StoreCookieSubscriber.php @@ -22,14 +22,26 @@ final class StoreCookieSubscriber implements EventSubscriberInterface { + /** + * @param PendingMetadataStorageInterface|null $pendingMetadataStorage without it, the metadata of a new client is persisted right away + */ public function __construct( private readonly ClientContextInterface $clientContext, private readonly CookieProviderInterface $cookieProvider, - private readonly PendingMetadataStorageInterface $pendingMetadataStorage, private readonly EventDispatcherInterface $eventDispatcher, private readonly string $cookieName, private readonly string $cookieExpiration, + private readonly ?PendingMetadataStorageInterface $pendingMetadataStorage = null, ) { + if (null === $pendingMetadataStorage) { + trigger_deprecation( + 'setono/client-bundle', + '1.3', + 'Not passing an instance of "%s" as "$pendingMetadataStorage" to "%s()" is deprecated, the argument will be required in 2.0.', + PendingMetadataStorageInterface::class, + __METHOD__, + ); + } } public static function getSubscribedEvents(): array @@ -66,7 +78,7 @@ public function store(ResponseEvent $event): void // Pending metadata sent by the client is handed to the client when it's resolved, at the latest when its metadata // is persisted on kernel.finish_request (see PendingMetadataClientContext), so the client can forget it - $this->pendingMetadataStorage->remove($request, $response); + $this->pendingMetadataStorage?->remove($request, $response); if (null === $cookie) { $client = $this->clientContext->getClient(); @@ -87,7 +99,7 @@ public function store(ResponseEvent $event): void private function storePendingMetadata(Request $request, Response $response, Client $client): void { $metadata = $client->metadata; - if (!$metadata instanceof ChangeAwareMetadata || !$metadata->isDirty()) { + if (null === $this->pendingMetadataStorage || !$metadata instanceof ChangeAwareMetadata || !$metadata->isDirty()) { return; } diff --git a/tests/Unit/EventSubscriber/StoreCookieSubscriberTest.php b/tests/Unit/EventSubscriber/StoreCookieSubscriberTest.php index c514649..2a626cb 100644 --- a/tests/Unit/EventSubscriber/StoreCookieSubscriberTest.php +++ b/tests/Unit/EventSubscriber/StoreCookieSubscriberTest.php @@ -278,6 +278,57 @@ public function it_ignores_metadata_that_is_not_change_aware_when_the_pre_store_ self::assertCount(0, $response->headers->getCookies()); } + /** + * @test + */ + public function it_triggers_a_deprecation_when_it_does_not_get_a_pending_metadata_storage(): void + { + $deprecations = []; + set_error_handler(static function (int $level, string $message) use (&$deprecations): bool { + $deprecations[] = $message; + + return true; + }, \E_USER_DEPRECATED); + + try { + self::createSubscriberWithoutPendingMetadataStorage( + $this->prophesize(ClientContextInterface::class), + $this->prophesize(CookieProviderInterface::class), + ); + } finally { + restore_error_handler(); + } + + self::assertSame([sprintf( + 'Since setono/client-bundle 1.3: Not passing an instance of "%s" as "$pendingMetadataStorage" to "%s::__construct()" is deprecated, the argument will be required in 2.0.', + PendingMetadataStorageInterface::class, + StoreCookieSubscriber::class, + )], $deprecations); + } + + /** + * @test + */ + public function it_leaves_the_metadata_of_a_new_client_to_be_persisted_right_away_without_a_pending_metadata_storage(): void + { + $metadata = new ChangeAwareMetadata(); + $metadata->set('foo', 'bar'); + + $clientContext = $this->prophesize(ClientContextInterface::class); + $clientContext->getClient()->willReturn(new Client('client-id', $metadata)); + + $cookieProvider = $this->prophesize(CookieProviderInterface::class); + $cookieProvider->getCookie(Argument::type(Request::class))->willReturn(null); + + $response = new Response(); + self::createSubscriberWithoutPendingMetadataStorage($clientContext, $cookieProvider)->store( + $this->createResponseEvent(new Request(), $response), + ); + + self::assertCount(1, $response->headers->getCookies()); + self::assertTrue($metadata->isDirty()); + } + /** * @param ObjectProphecy $clientContext */ @@ -312,10 +363,27 @@ private function createSubscriber( return new StoreCookieSubscriber( $clientContext->reveal(), $cookieProvider->reveal(), - $this->pendingMetadataStorage->reveal(), $eventDispatcher, $cookieName, $cookieExpiration, + $this->pendingMetadataStorage->reveal(), + ); + } + + /** + * @param ObjectProphecy $clientContext + * @param ObjectProphecy $cookieProvider + */ + private static function createSubscriberWithoutPendingMetadataStorage( + ObjectProphecy $clientContext, + ObjectProphecy $cookieProvider, + ): StoreCookieSubscriber { + return new StoreCookieSubscriber( + $clientContext->reveal(), + $cookieProvider->reveal(), + new EventDispatcher(), + self::COOKIE_NAME, + '+365 days', ); }