Skip to content

Conversation

chc421
Copy link
Contributor

@chc421 chc421 commented Sep 17, 2025

Description

Re-order server parambuilder library load order

Type of change

Please delete options that are not relevant

  • Fix (non-breaking change which fixes an issue)

Checklist

  • I have commented my code, particularly in hard-to-understand areas, if any.
  • I have confirmed that my changes do not introduce any new PHPCS warnings or errors.
  • I have checked plugin debug logs that my changes do not introduce any new PHP warnings or FATAL errors.
  • I followed general Pull Request best practices. Meta employees to follow this wiki.
  • I have added tests (if necessary) and all the new and existing unit tests pass locally with my changes.
  • I have completed dogfooding and QA testing, or I have conducted thorough due diligence to ensure that it does not break existing functionality.
  • I have updated or requested update to plugin documentations (if necessary). Meta employees to follow this wiki.

Changelog entry

One liner entry to be surfaced in changelog.txt

Test Plan

Manual test
Test case 1 (New visitor):

  • clear all browser cookie in incognito mode
  • visit website
  • verify fb cookies
  • verify debug.log
    Test case 2 (Existing user):
  • visit website
  • verify fb cookies
  • verify debu.log

Screenshots

Before

[17-Sep-2025 15:15:12 UTC] PHP Warning: Cannot modify header information - headers already sent in /Users/chenghaochen/Local Sites/local-color-crush/app/public/wp-content/plugins/facebook-for-woocommerce/facebook-commerce-events-tracker.php on line 203

Screenshot 2025-09-17 at 11 15 43 AM

After

Screenshot 2025-09-17 at 11 10 24 AM

@meta-cla meta-cla bot added the CLA Signed label Sep 17, 2025
Copy link
Contributor

github-actions bot commented Sep 17, 2025

📦 Latest Plugin Build

Built at: 2025-09-26T02:19:22.793Z
Commit: 5fe84f5
Size: 1.4M

Download: Click here to download the plugin

To download: Click the link above → Scroll to bottom → Download "facebook-for-woocommerce" artifact

@chc421 chc421 added changelog: add A new feature, function, or functionality was added. changelog: fix Took care of something that wasn't working. and removed changelog: add A new feature, function, or functionality was added. labels Sep 17, 2025
@cehicm cehicm requested a review from ukilla September 18, 2025 07:50
Copy link
Collaborator

@ukilla ukilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reviewing the param_builder_set_cookies and init_param_builder functions, I noticed that param_builder_set_cookies depends on the param_builder being initialized. Currently, with this fix, param_builder_set_cookies runs on the init hook at priority 1, while init_param_builder runs at priority 5. Since the cookie function relies on the param_builder, we need to run init_param_builder first, so increasing its priority to at least 6 ensures proper initialization before setting cookies. This priority should also prevent the “headers already sent” warning.

@chc421
Copy link
Contributor Author

chc421 commented Sep 18, 2025

After reviewing the param_builder_set_cookies and init_param_builder functions, I noticed that param_builder_set_cookies depends on the param_builder being initialized. Currently, with this fix, param_builder_set_cookies runs on the init hook at priority 1, while init_param_builder runs at priority 5. Since the cookie function relies on the param_builder, we need to run init_param_builder first, so increasing its priority to at least 6 ensures proper initialization before setting cookies. This priority should also prevent the “headers already sent” warning.

Yes, you are right. I think I might found the issue. get_integration did not have a priority with it and it runs other add actions that triggers interaction with browser. The previous version eliminate many cases because we try to force param_builder_set_cookies run early but there's no guarantee. Lmk if there's other concerns.

@chc421 chc421 requested a review from ukilla September 18, 2025 14:29
Copy link
Collaborator

@ukilla ukilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now we have the order:

  1. init_param_builder
  2. param_builder_set_cookies
  3. get_integration

Given that the integration class mainly handles feed-related actions (syncing, updating, etc.), this order looks fine, and after testing I don't see any warnings that were reported previously.

One possible improvement: we could add lazy initialization inside get_param_builder(), similar to how get_integration() works. That way, we ensure the param builder is always available when requested, without relying strictly on hook priority.

public function get_param_builder() {
    if ( null === $this->param_builder ) {
        $this->init_param_builder();
    }

    return $this->param_builder;
}

@chc421
Copy link
Contributor Author

chc421 commented Sep 19, 2025

iven that the integration class mainly handles feed-related actions (syncing, updating, etc.), this order looks fine, an

That's a good idea, now I can get rid of this https://github.com/facebook/facebook-for-woocommerce/blob/main/facebook-commerce-events-tracker.php#L194-L196.

@chc421 chc421 requested a review from ukilla September 19, 2025 13:55
Copy link
Collaborator

@ukilla ukilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @chc421,
I tested the flow again and can confirm both _fbp (always set) and _fbc (when fbclid is present) cookies are working correctly, and no warnings are triggered.

@chc421
Copy link
Contributor Author

chc421 commented Sep 22, 2025

Hi @chc421, I tested the flow again and can confirm both _fbp (always set) and _fbc (when fbclid is present) cookies are working correctly, and no warnings are triggered.

Thank you for testing. Could you help me to stamp?

@chc421 chc421 requested a review from ukilla September 22, 2025 12:47
@chc421 chc421 changed the title param builder set cookie order fix Fix 1 Sep 26, 2025
@chc421 chc421 changed the title Fix 1 pb fix option 1 Sep 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: fix Took care of something that wasn't working. CLA Signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants