Pb fix option 3 #3617
Pb fix option 3 #3617
Conversation
📦 Latest Plugin BuildBuilt at: 2025-10-06T17:02:35.261Z Download: Click here to download the plugin To download: Click the link above → Scroll to bottom → Download "facebook-for-woocommerce" artifact |
chc421
added
the
changelog: add
chc421
requested review from
vahidkay-meta,
iodic,
jarretth,
cehicm and
ukilla
facebook-commerce-events-tracker.php
Outdated
| */ | ||
| public function param_builder_server_setup() { | ||
| try { | ||
| $cookie_to_set = $this->get_param_builder()->getCookiesToSet(); |
There was a problem hiding this comment.
get_param_builder is a static method. It should be called with self:: and not $this.
|
|
||
| wp_enqueue_script( | ||
| 'facebook-capi-param-builder', | ||
| 'https://capi-automation.s3.us-east-2.amazonaws.com/public/client_js/capiParamBuilder/clientParamBuilder.bundle.js', |
There was a problem hiding this comment.
Is this something that's gonna live forever ( long-term )?
Some users tend to dwell on a version for a long time. So if there's a chance that in future the team decides to shut this s3 down and replace it with another URL, that needs to be addressed now.
Either we need commitment from that team that they will inform us at least 6 months in advance, or we need to handle this ourselves
There was a problem hiding this comment.
@hongj-src iirc this url should be a long term url?
There was a problem hiding this comment.
This S3 link will be used for at least 6 months + usage. We are considering move to a CDN, but the task got delayed. Even our CDN is ready, we'll maintain both for certain period. We should be good use the s3 link
| * | ||
| * @return string | ||
| */ | ||
| protected function get_browser_id() { |
There was a problem hiding this comment.
Can you do the same for fbc?
Was that skipped intentionally?
There was a problem hiding this comment.
Thanks for pointing this. When I was testing, I found that it was already handled by https://github.com/facebook/facebook-for-woocommerce/blob/main/includes/Events/Event.php#L264. I don't think parambuilder would provide more than what is already there, but I can add the logic if there are cases that parambuilder can handle but the existing implementation cannot.
There was a problem hiding this comment.
Yes, let's add that since it's going to help improve the backend coverage
| $this->aam_settings = $aam_settings; | ||
| $this->tracked_events = array(); | ||
|
|
||
| $this->param_builder_server_setup(); |
There was a problem hiding this comment.
$this is not correct.
It should be self
There was a problem hiding this comment.
param_builder_server_setup() is not a static method, do you want me to change its signature?
Description
Overview:
The CAPI Parambuilder library works with both client-side and server-side integration and can potentially improve fbc and fbp coverage and matchability.
In this PR, we are adding
Design doc: https://docs.google.com/document/d/1GH0dtJWjUrJlG8BUSzfyeoNlC9eK9-IdJBn3mp4shhM/edit?tab=t.0
Type of change
Please delete options that are not relevant
Checklist
Changelog entry
Add client and server parambuilder.
Test Plan
Manual test with following cases:
Screenshots
Please provide screenshots or snapshots of the system/state both before and after implementing the changes, if appropriate
Before
After
D83659301