Parse the _fbc cookie the way Meta writes it - #39
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #39 +/- ##
============================================
+ Coverage 62.40% 66.14% +3.74%
- Complexity 129 131 +2
============================================
Files 30 30
Lines 375 387 +12
============================================
+ Hits 234 256 +22
+ Misses 141 131 -10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
22c554f to
097310c
Compare
| * | ||
| * This is deliberately more lenient than Fbc::fromString() in the SDK, which only accepts alphanumeric click ids | ||
| * and exactly four segments. Real click ids are base64url and contain - and _, and Meta's own parameter builder | ||
| * (facebook/capi-param-builder-php) appends a 2 or 8 character appendix, so the strict pattern rejects cookies |
There was a problem hiding this comment.
Should we use the facebook/capi-param-builder-php if possible?
There was a problem hiding this comment.
Worth doing, but I would not fold it into this PR. I ran it against the current vendored copy (1.3.1, it arrives transitively through facebook/php-business-sdk) to get real numbers:
$b = new FacebookAds\ParamBuilder();
$cookies = $b->processRequest('www.example.com', ['fbclid' => 'IwAR1a-b_c'], ['_fbp' => 'fb.1.1657051589577.1088522659']);
// fbc: 'fb.1.1788781160733.IwAR1a-b_c.AQECAQMB'
// fbp: 'fb.1.1657051589577.1088522659.AQEAAQMB'
// cookie _fbc domain=example.com <- eTLD+1, not the hostThat confirms two things the review found: Meta writes a five segment value with an eight character appendix, and it sets the cookie on the registrable domain rather than host-only.
What speaks for it
- It is Meta's own implementation of the semantics we are reverse engineering: appendix handling, when to rewrite the cookie, and the eTLD+1 resolution.
- One library would cover this issue, StoreFbpSubscriber sets the _fbp cookie on every response, also without pixels #28 and Allow configuring the cookie domain for _fbp/_fbc #29, plus
getClientIpAddress()andgetEventSourceUrl(). processRequest($host, $queries, $cookies, $referer, $xForwardedFor, $remoteAddress)takes explicit arguments, so no$_GET/$_SERVERaccess. It autoloads cleanly.
What speaks against doing it here
- It would have to become a direct
require. Today we only get it becausephp-business-sdkpulls it in. - It targets PHP 7.4 with no parameter or return types, so at PHPStan level max every call site needs narrowing.
- It is a whole-request abstraction: one call computes fbc, fbp, ip and source url and returns the cookies to set. Our
Contextclasses are small decorators a user can swap one at a time. Adopting it means redesigningContext/Fbc,Context/Fbp,StoreFbcSubscriberandStoreFbpSubscriber, and deciding whatFbcContextInterface::getFbc(): ?Fbcreturns, since the builder deals in strings. - It returns strings rather than the SDK's
Fbc/Fbpvalue objects.User::$fbcacceptsstring|Fbc|nullso it would work, but our own interfaces would change.
So: a good idea, and a design change rather than a bug fix. I opened #45 for it. This PR stays the minimal fix so the cookies Meta writes today are actually read, whichever way that decision goes.
d21eff5 to
0e4216b
Compare
The SDK's Fbc::fromString() only accepts alphanumeric click ids and exactly four segments. Real click ids are base64url, and Meta's own parameter builder appends an appendix segment, so cookies written by the browser pixel were silently dropped and no fbc was sent. Parse the cookie in the bundle with a pattern matching what Meta writes, and log at debug level when a value cannot be used. Fixes #20
097310c to
762154a
Compare
0e4216b to
812c650
Compare
| if (1 !== preg_match(self::COOKIE_PATTERN, $cookie, $matches)) { | ||
| $this->logger->debug('The _fbc cookie value "{value}" could not be parsed and is ignored', ['value' => $cookie]); | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| return (new Fbc($matches[3])) | ||
| ->withSubdomainIndex((int) $matches[1]) | ||
| ->withCreationTime((int) $matches[2]) | ||
| ; | ||
| } catch (\InvalidArgumentException $e) { | ||
| // The creation time is in the future or predates Facebook | ||
| $this->logger->debug('The _fbc cookie value "{value}" was rejected: {message}', [ | ||
| 'value' => $cookie, | ||
| 'message' => $e->getMessage(), | ||
| ]); | ||
|
|
||
| return null; |
There was a problem hiding this comment.
Why put the code here instead of rewriting Fbc::fromString?
There was a problem hiding this comment.
You're right, it belongs there. I moved it: Setono/meta-conversions-api-php-sdk#13.
My reason for putting it here was that the SDK is a separate release cycle, which is a weak argument when you maintain both, and it left the bundle carrying a second parser next to the SDK's.
Chasing it into the SDK also turned up something my review missed: Fbp::fromString() has the same bug, and there it is worse.
Fbp::fromString('fb.1.1657051589577.1088522659.AQEAAQMB'); // throwsThat is the shape Meta's parameter builder writes. CookieBasedFbcContext returning null costs you the click id. CookieBasedFbpContext falling through to GeneratedFbpContext means the server mints a brand new fbp on every single request while the browser has a stable one, so the two sides stop describing the same person. Nothing throws where you can see it, and the only symptom is a lower Event Match Quality.
The SDK PR relaxes both patterns and preserves the appendix, so a value read from a cookie is written back byte for byte rather than being rewritten into a shape the pixel does not expect. Coverage stays at 100% and Infection stays above the threshold.
Suggested order. This PR is blocked until that one is released, since CI here would still resolve ^1.1. Once you merge and tag the SDK, I will reduce this PR to: call Fbc::fromString() again, keep the debug logging for a cookie that cannot be parsed, fix the _fbp path the same way, and bump the constraint. That deletes more code here than it adds.
Say the word if you would rather I park this one and reopen it after the tag.
|
Rebased the rest of the series off this branch, so this PR is now standalone on #40 was sitting on this branch and is now based on |
1 participant
Fixes #20
Problem
CookieBasedFbcContextparsed the_fbccookie with the SDK'sFbc::fromString(), which requires/^fb\.([012])\.(\d{13})\.([a-zA-Z0-9]+)$/, and swallowed the exception when it did not match. Two kinds of perfectly legitimate cookies were silently dropped:-or_. Realfbclidvalues are base64url. The bundle wrote such a value into the cookie itself and then could not read it back on the next request.facebook/capi-param-builder-php, writesfb.<idx>.<ts>.<fbclid>.<appendix>with a 2 or 8 character appendix. The strict pattern requires exactly four segments, so a cookie written by the browser pixel yielded nofbcat all.Either way
user_data.fbcwas missing from server side events, with no log line and no exception. The only visible symptom is a lower Event Match Quality in Events Manager.Change
The bundle parses the cookie itself with a pattern that accepts the base64url alphabet and an optional appendix segment, then builds the
Fbcthrough the SDK's public API. A cookie whose creation time is in the future or predates Facebook is still rejected, becausewithCreationTime()asserts on it.Unparseable and rejected cookies are now logged at debug level on the
setono_meta_conversions_apiMonolog channel instead of vanishing.psr/logmoves from a transitive to a declared dependency.The SDK's own
Fbc::fromString()is left alone; the bundle simply no longer depends on it for this. Relaxing it upstream insetono/meta-conversions-api-php-sdkis still worthwhile, and the appendix behaviour is worth confirming against whatfbevents.jswrites in a live browser.Tests
Eleven unit tests: no request, no cookie, four accepted shapes (alphanumeric, base64url, both appendix lengths) asserting click id, subdomain index and creation time, and seven rejected shapes.