Skip to content

Stop failed Commerce Partner Integration calls from breaking the daily sync and feed uploads - #4070

Open
jczhuoMeta wants to merge 1 commit into
mainfrom
fix/cpi-graceful-failures
Open

jczhuoMeta wants to merge 1 commit into
mainfrom
fix/cpi-graceful-failures

Conversation

@jczhuoMeta

@jczhuoMeta jczhuoMeta commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Description

Hardens the plugin's Commerce Partner Integration (CPI) and feed-upload calls so that one failing call cannot affect more than itself. Two deliberate behaviors are unchanged:

  • Graph wc_facebook_connection_invalid detection (API::do_post_parse_response_validation)
  • finalize-install failing closed on a 401 or external business ID mismatch

This PR is independent of #4058. Both add to tests/e2e/plugin-level-tests.spec.js in different places, and they merge cleanly onto main in either order.

Internal tracking: T292702689

1. A numeric ID aborts the daily sync.
Repair\Response::get_commerce_partner_integration_id() and FBE\Installation\Read\Response::get_catalog_id() are declared under strict_types. A numeric ID in the response made them throw a TypeError. That error got past the ApiException handler and skipped the rest of refresh_installation_data(), including the configuration push and asset refresh. Both getters now accept string or integer IDs and treat any other type as missing.

2. Failures reported as success.
The legacy FBE install read logged "Refreshed asset mapping…" even when Graph returned an error or no installation. Both cases now raise, and the existing handler records the failure. No stored asset changes.

file_update Graph errors were silently discarded. They are now logged like other upload failures.

3. CPI metadata feeds run without an access token.
AbstractFeed::should_skip_feed() only checked the stored CPI and CMS IDs. A rejected finalize-install clears the token but leaves those IDs, so the promotions, shipping, navigation and ratings feeds kept regenerating and failing their upload. Feeds are now skipped until the store is connected again.

4. The feed callback can re-trigger uploads.
The ?wc-api=wc_facebook_get_feed_data_* callback regenerated a missing file with generate_feed_file(), which announces a new feed and starts another file_update upload. It did this before checking the secret. So any request, or Meta fetching a file whose write kept failing, could keep re-triggering uploads. Now the callback:

  • checks the secret first, in constant time (hash_equals()) and only against a string value;
  • rebuilds a missing file in place without starting a new upload;
  • still answers invalid-secret requests with a 401, but no longer logs them to Meta, so anonymous requests cannot flood the log queue;
  • has the broken "not readable" message string fixed.

It also replaces non-public service and class names in comments and test strings of the touched files with the public endpoint names.

Type of change

  • 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.
  • 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). No documentation change is needed.

Changelog entry

Stop failed Commerce Partner Integration calls from breaking the daily sync and feed uploads.

Test Plan

Automated

  • PHP unit suite (PHP 8.2): 2116 tests passed. New and updated tests:

    • RepairResponseTest (new) and ReadInstallationResponseTest: string, int, array, null, float and bool IDs for both getters.
    • ConnectionCpiRepairTest::test_repair_accepts_numeric_integration_id and ConnectionAssetMappingTest::test_fbe_install_read_accepts_numeric_catalog_id: the daily sync completes with numeric IDs.
    • ConnectionAssetMappingTest::test_fbe_install_read_error_is_not_reported_as_refresh: Graph error, "no install" and "empty install" responses.
    • AbstractFeedTest: no-token skip case, and testUploadGraphErrorIsLogged.
  • New e2e test, plugin-level-tests.spec.js: "Feed callback checks the secret before rebuilding a missing feed file". It moves the promotions feed file aside, then checks that:

    • a wrong secret gets a 401 and leaves the file missing;
    • the right secret gets a 200 and rebuilds the file.

    The unit tests cannot cover this ordering because the callback ends with exit;. It runs in CI's plugin-events group and passes.

  • PHPCS (phpcs.xml.dist) and PHPCompatibilityWP (PHP 7.4+) on changed files, php -l, node --check and git diff --check: clean.

Manual verification

  1. No-token skip. On a connected store, delete wc_facebook_access_token but keep the CPI and CMS IDs. Run the promotions feed's scheduled action and confirm there is no file_update request and no feed_upload error log. Restore the token and confirm the next scheduled run uploads.
  2. Feed callback. Delete the promotions feed file under wp-content/uploads/facebook_for_woocommerce/promotions/. Request ?wc-api=wc_facebook_get_feed_data_promotions&secret=wrong: expect a 401, no file regeneration, no file_update request, and no Meta log. Repeat with the real secret from wc_facebook_feed_url_secret_promotions: expect the file to be rebuilt and served, still with no file_update request.

QA results

Local WordPress store (WordPress 7.1.2, WooCommerce 11.1.2, PHP 8.2.30), 2026-10-13. The PHP debug log had no new warnings or fatal errors during any of the checks below, including heartbeats run through WP-Cron.

  • No-token skip. With the token missing but the CPI and CMS IDs stored, should_skip_feed() returned true, nothing was scheduled, and no HTTP request was made. With the token restored, the feed was no longer skipped, and POST /{cpi_id}/file_update returned 200.
  • Feed callback, wrong secret. The callback returned 401, the missing file was not rebuilt, and nothing was queued to Meta.
  • Feed callback, right secret. The callback returned 200, and the missing file was rebuilt in place and served without starting an upload.

Screenshots

Not applicable; this change has no user-interface impact.

@jczhuoMeta jczhuoMeta added the changelog: fix Took care of something that wasn't working. label Sep 30, 2026
@meta-cla meta-cla Bot added the CLA Signed label Sep 30, 2026
@github-actions github-actions Bot added the type: bug The issue is a confirmed bug. label Sep 30, 2026
@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

📦 Latest Plugin Build

Built at: 2026-11-05T10:26:21Z
Commit: 8859ece
Size: 1.9M

Download: Click here to download the plugin

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

@jczhuoMeta
jczhuoMeta force-pushed the fix/cpi-graceful-failures branch from 5e87635 to 4d77957 Compare September 30, 2026 20:03
@jczhuoMeta jczhuoMeta changed the title Handle Commerce Partner Integration call failures gracefully Stop failed Commerce Partner Integration calls from breaking the daily sync and feed uploads Sep 30, 2026
@jczhuoMeta
jczhuoMeta force-pushed the fix/cpi-graceful-failures branch from 4d77957 to 297f51f Compare October 13, 2026 19:37
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 19 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@08164c9). Learn more about missing BASE report.

Files with missing lines Patch % Lines
includes/Feed/AbstractFeed.php 0.00% 11 Missing ⚠️
includes/Handlers/Connection.php 0.00% 4 Missing ⚠️
...cludes/API/CommerceIntegration/Repair/Response.php 0.00% 2 Missing ⚠️
includes/API/FBE/Installation/Read/Response.php 0.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4070   +/-   ##
=======================================
  Coverage        ?   11.11%           
  Complexity      ?     6135           
=======================================
  Files           ?      235           
  Lines           ?    20786           
  Branches        ?        0           
=======================================
  Hits            ?     2310           
  Misses          ?    18476           
  Partials        ?        0           
Flag Coverage Δ
integration 11.11% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jczhuoMeta
jczhuoMeta marked this pull request as ready for review October 13, 2026 22:29
@meta-codesync

meta-codesync Bot commented Oct 14, 2026

Copy link
Copy Markdown

@jczhuoMeta has imported this pull request. If you are a Meta employee, you can view this in D125257831.

…y sync and feed uploads

A repair response or legacy FBE install read that encoded its ID as a
number made Repair\Response::get_commerce_partner_integration_id() or
FBE\Installation\Read\Response::get_catalog_id() throw a TypeError under
strict_types. The error escaped the ApiException handler and aborted the
rest of the daily sync, skipping the configuration push and asset
refresh. Both getters now accept string or integer IDs and treat any
other type as missing.

The legacy FBE install read reported a successful refresh even when
Graph returned an error or no installation at all. Both cases now raise,
so the existing handler records the failure and no stored asset changes.
file_update Graph errors were silently discarded; they are now logged
like any other upload failure.

CPI metadata feeds kept regenerating and failing to upload when the
store had no access token, for example after a rejected finalize-install
cleared the token but left the stored IDs. They are now skipped until
the store is connected again.

The feed callback used to regenerate a missing file through
generate_feed_file(), which announces a new feed and starts another
upload, and did so before checking the secret. Any request to the
callback, or Meta fetching a file whose write kept failing, could
re-trigger uploads. The secret is now checked first, in constant time
and only against a string, and a missing file is rebuilt in place
without starting a new upload. Invalid-secret requests still get a 401
but are no longer logged to Meta, so anonymous requests cannot flood the
log queue.
@vahidkay-meta
vahidkay-meta force-pushed the fix/cpi-graceful-failures branch from 297f51f to 1151367 Compare November 5, 2026 10:25

This branch has not been deployed

No deployments
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 type: bug The issue is a confirmed bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants