Repository navigation
Conversation
loevgaard
force-pushed
the
defer-metadata-for-new-clients
branch
from
October 26, 2026 12:02
657beb4 to
2e96110
Compare
loevgaard
commented
Oct 26, 2026
| } | ||
|
|
||
| /** @var array<string, mixed> $metadata */ | ||
| $metadata = json_decode($json, true); |
Member
Author
There was a problem hiding this comment.
Doesn't this belong in a try catch?
loevgaard
commented
Oct 26, 2026
Comment on lines
+36
to
+40
| // 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; | ||
| } |
Member
Author
There was a problem hiding this comment.
Shouldn't the two 'metadatas' be merged instead?
loevgaard
commented
Oct 26, 2026
| } | ||
| } | ||
|
|
||
| private static function markClean(Metadata $metadata): void |
Member
Author
There was a problem hiding this comment.
Do we really need this helper?
The metadata of a new client (no client id cookie on the request) is stored in a signed, HttpOnly cookie (<cookie name>_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
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.
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.
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.
loevgaard
force-pushed
the
defer-metadata-for-new-clients
branch
from
October 26, 2026 12:34
1aa9a43 to
0bc0a58
Compare
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #8.
New clients (requests without the
setono_client_idcookie) no longer get their metadata persisted on their first request. It is kept in a signed cookie,setono_client_id_pending_metadata, and persisted on the client's next request, once the client has shown that it keeps its cookies. Crawlers, link checkers, preview fetchers and cookie-blocking browsers never come back with the cookie, so they never produce a row.Choosing the approach
I compared the current behaviour, a session, the proposal in #8, and a variant of it:
PHPSESSIDcookieHttpOnly)Why not the session
gc_maxlifetime(24 min by default). A visitor who lands from an ad, views one page and comes back next week to buy has lost thegclid. Today it's kept, and with a cookie that lives as long as the id cookie (365 days) it's still kept. This is the main use case of SyliusGoogleAdsPlugin, which stores its tracking information with a 90-day TTL.AbstractSessionListenermark the responseprivate, so landing pages can't be cached by shared caches. On routes declaredstatelessit throwsUnexpectedSessionUsageExceptionin debug mode. The bundle doesn't need sessions today, and API-only apps may not have them.What changed compared to the proposal in #8
Sending the pending metadata to the browser is the right idea. This PR changes where it goes and how it's protected:
Cookie::fromString()splits withexplode('.', $cookie, 4), so an oldersetono/client, or JavaScript that reads the id (the README encourages this), would read<uuid>.<payload>as the client id during the pending hop. A separate cookie leaves the id cookie format andsetono/clientuntouched, and it can beHttpOnly.<json>.<hmac>, signed withkernel.secretover the client id as well, so it can't be forged or moved to another client.How it works
StoreCookieSubscriber, for a new client with changed metadata: stores the metadata viaPendingMetadataStorageInterface::store()and marks it clean, soStoreMetadataSubscriberskips it. If it can't be stored (too big, not JSON encodable), it stays dirty and is persisted right away as before. Metadata changed after the subscriber has run becomes dirty again and is also persisted right away.PendingMetadataClientContextchecks the signature against the client id. This is a new decorator between the cached (32) and the cookie-based (64) context, at priority 48.kernel.finish_request.StoreCookieSubscriberclears the pending cookie.Metadata::toArray(), including__expires, and is rebuilt with the constructor.JSON_PRESERVE_ZERO_FRACTION), so metadata looks the same whether it comes from the cookie or the database.CookieBasedPendingMetadataStorageis only the default implementation ofPendingMetadataStorageInterface(setono_client.pending_metadata_storage.default). It can be replaced, for example by a session-based storage for apps that prefer that trade-off.ChangeAwareMetadatagetsmarkDirty()andmarkClean().Behaviour changes
store()returnsfalse.StoreCookieSubscribernow runs onkernel.responseat priority-64instead of0. Listeners with the same priority run in registration order, and SyliusGoogleAdsPlugin sets its metadata onkernel.responseat priority 0. At 0, this bundle would usually run first, and thegclidwould be persisted right away.-64runs after default-priority listeners andCacheAttributeListener(-10), and before the profiler (-100) collects the response.PreStoreCookieEventvetoes the cookie for a new client, its metadata is no longer persisted: the client never receives its id, so nothing could ever read the row.StoreCookieSubscribertakesPendingMetadataStorageInterfaceas its third argument, and the bundle now uses%kernel.secret%.Testing
ChangeAwareMetadata. The extension test covers the new services.tests/Functional/NewClientMetadataTest.phpruns the real services throughHttpKernel, with an in-memory Doctrine manager:composer validate --strict;