Skip to content

Only write the fbp and fbc cookies when they are useful - #50

Open
loevgaard wants to merge 1 commit into
fix/17-pii-in-transportfrom
fix/28-fbp-cookie
Open

Only write the fbp and fbc cookies when they are useful#50
loevgaard wants to merge 1 commit into
fix/17-pii-in-transportfrom
fix/28-fbp-cookie

Conversation

@loevgaard

Copy link
Copy Markdown
Member

Fixes #28

Stacked on #49.

Problem

StoreFbpSubscriber wrote the _fbp cookie on the response to every main request: JSON and API responses, 404s, 500s, assets served through Symfony, and applications with zero pixels configured. Any Set-Cookie header makes the response uncacheable for Symfony HttpCache, Varnish and CDNs, so this quietly cost cacheability sitewide for something that may not be used at all.

It also never consulted the pixel provider, unlike AddLibraryToTagBagSubscriber which already checks getPixels().

The private setCookie() method was a predicate whose name read like a command.

Change

Both store subscribers now skip when:

  • the response is neither successful nor a redirect. Redirects are deliberately kept, because an ad click regularly lands on one, especially when the application strips the fbclid from the url;
  • the pixel provider returns no pixels, since there is nowhere to send events to anyway.

New cookies.fbp and cookies.fbc options (both default true) remove the corresponding subscriber from the container entirely, for sites where the browser pixel already writes them.

setCookie() is renamed to shouldSetCookie(), and the cookie names and the 90 day lifetime move into a shared Cookie\Cookies class used by the two contexts that read them and the two subscribers that write them, replacing four magic strings.

Tests

Nine unit tests for StoreFbpSubscriber, its first: cookie set, not http-only so the browser pixel can read it, skipped without pixels, without consent, on a sub request and on a 500, written on a redirect, and the two hour renewal window in both directions. Plus two extension tests for the new switches.

@codecov

codecov Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 67.64706% with 11 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (fix/17-pii-in-transport@5c3470c). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/EventSubscriber/StoreFbcSubscriber.php 0.00% 8 Missing ⚠️
src/Context/Fbc/CookieBasedFbcContext.php 0.00% 1 Missing ⚠️
src/Context/Fbp/CookieBasedFbpContext.php 0.00% 1 Missing ⚠️
src/Cookie/Cookies.php 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                    Coverage Diff                     @@
##             fix/17-pii-in-transport      #50   +/-   ##
==========================================================
  Coverage                           ?   81.42%           
  Complexity                         ?      157           
==========================================================
  Files                              ?       33           
  Lines                              ?      506           
  Branches                           ?        0           
==========================================================
  Hits                               ?      412           
  Misses                             ?       94           
  Partials                           ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Both cookies were written on every main-request response, including
responses of applications with no pixels configured at all. Any
Set-Cookie header makes a response uncacheable for shared caches.

Skip when no pixels are available and on 4xx/5xx responses, keep
redirects because ad clicks land on them, and add cookies.fbp and
cookies.fbc switches for sites where the browser pixel writes them.

Fixes #28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant