Add get_entry_count() to product feed FeedInterface by wjrosa · Pull Request #64394 · woocommerce/woocommerce

wjrosa · 2026-04-24T12:25:29Z

Submission Review Guidelines

I have followed the WooCommerce Contributing Guidelines and the Contributor Code of Conduct.

Changes proposed in this Pull Request

Adds a get_entry_count(): int method to FeedInterface and implements it on JsonFileFeed. Consumers of the product feed API — notably anyone using ProductWalker — cannot currently get an accurate count of rows that were actually written to a feed: ProductWalker::walk() returns $progress->processed_items, which counts products iterated, not rows persisted. Any row rejected by FeedValidatorInterface::validate_entry() is silently dropped inside ProductWalker::iterate(), so the walker count overcounts whenever validation fails.

This change was prompted by reviewer feedback on woocommerce-gateway-stripe#5319 (1, 2), where the Stripe plugin had to add the equivalent method to its own CSV feed class to fix an incorrect "synced product count" on the Agentic Commerce dashboard. Pulling it into the interface keeps every FeedInterface implementation consistent and removes the need for consumer-side instanceof checks.

Summary of changes

FeedInterface: new get_entry_count(): int contract method.
JsonFileFeed: implements get_entry_count() by tracking a counter in add_entry().
Tests for the new behaviour in JsonFileFeedTest.

How to test the changes in this Pull Request

Automated:

pnpm --filter=@woocommerce/plugin-woocommerce test:php:env -- --filter 'JsonFileFeedTest|ProductWalkerTest' passes (13 tests / 443 assertions).
pnpm --filter=@woocommerce/plugin-woocommerce lint:php:changes clean.
pnpm --filter=@woocommerce/plugin-woocommerce lint:changes:branch clean.
composer exec -- phpstan analyse …/FeedInterface.php …/JsonFileFeed.php --memory-limit=2G clean.

Manual:

Construct a JsonFileFeed, call start(), add N entries, then end().
Confirm get_entry_count() returns N.
Confirm that calls to add_entry() made before start() (i.e. when the file handle isn't open) do not count.

Testing that has already taken place

New unit tests cover the happy path and the "entries before start" edge case.
Existing JsonFileFeedTest and ProductWalkerTest still pass with the refactored add_entry() (counter-based instead of boolean has_entries).

Changelog entry

Automatically create a changelog entry from the details below.

Changelog Entry Details

Significance

Patch
Minor
Major

Type

Message

Add get_entry_count() to FeedInterface so product feed consumers can report the number of rows actually written to the feed (distinct from the number of products iterated by ProductWalker).

Comment

Changelog Entry Comment

Comment

This PR is a Draft (untick when ready for review).
Milestone is set to the first available milestone that is not in the past.

🤖 Generated with Claude Code

Expose the number of rows actually written to the feed so consumers can report an accurate count. ProductWalker reports the number of products iterated, which overcounts when FeedValidatorInterface::validate_entry() silently drops entries before they reach FeedInterface::add_entry(). Implemented on JsonFileFeed by tracking the count in add_entry(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-04-24T12:34:31Z

📝 Walkthrough

Walkthrough

A new get_entry_count() method is added to the FeedInterface and implemented in JsonFileFeed to track the actual number of entries successfully written to a feed. The implementation refactors entry tracking from a boolean flag to an integer counter, incremented when entries are added, with corresponding test coverage.

Changes

Cohort / File(s)	Summary
Interface Definition `plugins/woocommerce/src/Internal/ProductFeed/Feed/FeedInterface.php`	Adds public method signature `get_entry_count(): int` to expose feed entry count to consumers.
Implementation `plugins/woocommerce/src/Internal/ProductFeed/Storage/JsonFileFeed.php`	Replaces boolean "has entries" flag with `$entry_count` integer; refactors `add_entry()` to increment counter after successful encoding; implements `get_entry_count()` accessor.
Tests `plugins/woocommerce/tests/php/src/Internal/ProductFeed/Storage/JsonFileFeedTest.php`	Adds two test methods verifying counter initializes to 0, increments with each `add_entry()` call, persists after `end()`, and ignores pre-`start()` entries.
Documentation `plugins/woocommerce/changelog/add-feed-interface-get-entry-count`	Changelog entry documenting the new `get_entry_count()` method addition.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely describes the main change: adding a get_entry_count() method to the FeedInterface.
Description check	✅ Passed	The description comprehensively explains the motivation, implementation details, testing approach, and changelog entry—all clearly related to the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch add/feed-interface-get-entry-count

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

plugins/woocommerce/src/Internal/ProductFeed/Storage/JsonFileFeed.php (1)
100-144: ⚠️ Potential issue | 🟡 Minor

Consider resetting $entry_count in start().

If start() is called twice on the same instance (e.g., to generate a fresh feed), $entry_count persists from the previous run. The next add_entry() will then write a leading , after the opening [, producing invalid JSON — and get_entry_count() will report a count that includes the previous run.

Even if reuse isn't currently a supported flow, resetting the counter (and ideally $file_completed/$file_url) in start() makes the API more robust and the reported count unambiguously scoped to the current run.
🛠️ Suggested guard
 	public function start(): void {
+		$this->entry_count    = 0;
+		$this->file_completed = false;
+		$this->file_url       = null;
 		/**
 		 * Allows the current time to be overridden before a feed is stored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/woocommerce/src/Internal/ProductFeed/Storage/JsonFileFeed.php` around
lines 100 - 144, start() currently does not reset state from previous runs which
allows $entry_count (and related state) to carry over and produce invalid JSON
when add_entry() writes separators; modify the start() method to reset
$this->entry_count = 0 and also clear $this->file_completed = false and
$this->file_url = '' (or null) at the beginning of start() so that
get_entry_count() and subsequent add_entry() behavior are correctly scoped to
the new feed generation.

🧹 Nitpick comments (1)

plugins/woocommerce/src/Internal/ProductFeed/Storage/JsonFileFeed.php (1)
152-168: fwrite() failure is counted as a successful entry.

fwrite() can return false or a short write, but $this->entry_count is incremented unconditionally. If a write fails partway through, get_entry_count() will overcount and the separator logic will emit a comma for a row that was never (fully) written, producing malformed JSON.

Since the stated purpose of this counter is to report the rows actually written, consider guarding the increment on the write result:
🛠️ Suggested guard
-		if ( $this->entry_count > 0 ) {
-			fwrite( $this->file_handle, ',' );
-		}
-
-		fwrite( $this->file_handle, $json );
-		++$this->entry_count;
+		$payload = ( $this->entry_count > 0 ? ',' : '' ) . $json;
+		$written = fwrite( $this->file_handle, $payload );
+		if ( false === $written || $written < strlen( $payload ) ) {
+			return;
+		}
+		++$this->entry_count;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/woocommerce/src/Internal/ProductFeed/Storage/JsonFileFeed.php` around
lines 152 - 168, The add_entry method currently increments $this->entry_count
regardless of fwrite outcomes; change add_entry (in class JsonFileFeed) to check
the return value(s) from fwrite when writing the separator and the JSON payload,
only incrementing $this->entry_count after confirming the full expected byte
count was written (handle short writes by looping until total bytes written
equals strlen($json) and the comma write succeeded, or treat a failure and abort
without increment); ensure the separator (the comma) is only emitted when the
previous entry was actually written so get_entry_count and the resulting JSON
stay consistent.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@plugins/woocommerce/src/Internal/ProductFeed/Storage/JsonFileFeed.php`:
- Around line 100-144: start() currently does not reset state from previous runs
which allows $entry_count (and related state) to carry over and produce invalid
JSON when add_entry() writes separators; modify the start() method to reset
$this->entry_count = 0 and also clear $this->file_completed = false and
$this->file_url = '' (or null) at the beginning of start() so that
get_entry_count() and subsequent add_entry() behavior are correctly scoped to
the new feed generation.

---

Nitpick comments:
In `@plugins/woocommerce/src/Internal/ProductFeed/Storage/JsonFileFeed.php`:
- Around line 152-168: The add_entry method currently increments
$this->entry_count regardless of fwrite outcomes; change add_entry (in class
JsonFileFeed) to check the return value(s) from fwrite when writing the
separator and the JSON payload, only incrementing $this->entry_count after
confirming the full expected byte count was written (handle short writes by
looping until total bytes written equals strlen($json) and the comma write
succeeded, or treat a failure and abort without increment); ensure the separator
(the comma) is only emitted when the previous entry was actually written so
get_entry_count and the resulting JSON stay consistent.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 014cd627-3221-4119-9b76-b27e130e7d51

📥 Commits

Reviewing files that changed from the base of the PR and between 63df81f and 59782d3.

📒 Files selected for processing (4)

plugins/woocommerce/changelog/add-feed-interface-get-entry-count
plugins/woocommerce/src/Internal/ProductFeed/Feed/FeedInterface.php
plugins/woocommerce/src/Internal/ProductFeed/Storage/JsonFileFeed.php
plugins/woocommerce/tests/php/src/Internal/ProductFeed/Storage/JsonFileFeedTest.php

github-actions · 2026-04-24T12:42:23Z

Testing Guidelines

Hi @daledupreez ,

Apart from reviewing the code changes, please make sure to review the testing instructions (Guide) and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed.

Reminder: PR reviewers are required to document testing performed. This includes:

🖼️ Screenshots or screen recordings.
📝 List of functionality tested / steps followed.
🌐 Site details (environment attributes such as hosting type, plugins, theme, store size, store age, and relevant settings).
🔍 Any analysis performed, such as assessing potential impacts on environment attributes and other plugins, conducting performance profiling, or using LLM/AI-based analysis.

⚠️ Within the testing details you provide, please ensure that no sensitive information (such as API keys, passwords, user data, etc.) is included in this public issue.

github-actions · 2026-04-24T12:50:30Z

Test using WordPress Playground

The changes in this pull request can be previewed and tested using a WordPress Playground instance.
WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Test this pull request with WordPress Playground.

Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a commit that changes plugin code.

malithsen

Looks good to me on inspection.

daledupreez

The change looks OK to me from inspection, but I have concerns about how we're introducing the change to the interface, as I suspect there are other plugins that have extended this interface, and this could qualify as a breaking change given that they won't implement this method yet.

I think we need to look into any standard WooCommerce practices around evolving interfaces, as I am concerned that this particular approach could be problematic. (As an example, version 10.6.0 of the Stripe plugin would not implement this interface if it was installed on top of this code.)

@SInCE

- Reset $entry_count, $file_completed, and $file_url at the top of start() so a feed instance can be regenerated without producing invalid JSON (carried-over entry_count would emit a leading "," after "[") or leaking stale completion state into get_entry_count() / get_file_url(). - Add a regression test (test_start_resets_state_from_previous_run) covering the start -> end -> start -> end flow. - Bump @SInCE 10.8.0 -> 10.9.0 on FeedInterface::get_entry_count() to match the current dev version (10.9.0-dev), per review feedback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

wjrosa · 2026-05-12T16:38:30Z

Thanks @daledupreez. FeedInterface lives in Automattic\WooCommerce\Internal, which src/README.md and src/Internal/README.md explicitly carve out as not BC-guaranteed and not for extension use ("even those not having an @internal annotation"), so adding to the interface is permitted by policy.

On the practical side: woocommerce-gateway-stripe#5319 already implements get_entry_count(): int with a matching signature on WC_Stripe_Agentic_Commerce_Csv_Feed, so the lockstep is on release timing rather than code: a Stripe release containing #5319 needs to ship before (or alongside) WC 10.9 going public, otherwise Stripe ≤10.6.0 running against WC 10.9 would fatal on the missing method. Happy to coordinate that with the Stripe team.

wjrosa · 2026-05-12T16:38:38Z

@coderabbitai Addressed the outside-diff suggestion in e61603d: start() now resets $entry_count, $file_completed, and $file_url so the feed instance can be regenerated without producing invalid JSON or leaking stale completion state. Added test_start_resets_state_from_previous_run covering the start → end → start → end flow.

Skipping the fwrite() increment-guard nitpick: on a local filesystem, fwrite() to a regular file effectively never short-writes, and if it does, the JSON output itself is already corrupted — a correctly-decremented counter doesn't help recover. The right place to detect I/O failure would be at the file boundary (end() / get_file_path()), not the per-entry increment, so a half-measure here would add complexity without solving the underlying class of problem.

coderabbitai · 2026-05-12T16:39:05Z

Tip

For best results, initiate chat on the files or code changes.

@wjrosa: Both points noted — thanks for the thorough follow-through.

The state-reset changes look great. Resetting $entry_count, $file_completed, and $file_url in start() makes the lifecycle contract much cleaner for reuse, and the test_start_resets_state_from_previous_run test gives solid coverage of that flow.

Your reasoning on the fwrite() guard is sound — if a write short-writes on a local filesystem the JSON stream is already structurally broken, so adjusting the counter is a half-measure that adds complexity without actually enabling recovery. Agreed that end()/get_file_path() is the right boundary for I/O failure detection if that ever becomes a concern.

✏️ Learnings added

Learnt from: wjrosa
URL: https://github.com/woocommerce/woocommerce/pull/64394

Timestamp: 2026-05-12T16:39:03.372Z
Learning: In `JsonFileFeed` (`plugins/woocommerce/src/Internal/ProductFeed/Storage/JsonFileFeed.php`), the `fwrite()` call in `add_entry()` intentionally has no failure guard on `$entry_count`. The rationale is: on a local filesystem `fwrite()` to a regular file effectively never short-writes; if it does, the JSON output is already structurally corrupted and a decremented counter cannot aid recovery. I/O failure detection is intentionally deferred to the file boundary (`end()` / `get_file_path()`), not the per-entry level.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

daledupreez

I wasn't sure if other plugins may have started using the interface, so I double-checked whether any public repos reference Automattic\WooCommerce\Internal\ProductFeed\Feed\FeedInterface:

GitHub results - it looks like it's only WooCommerce and the woocommerce-gateway-stripe (plus forks), as well as an archived WooCommerce repo that was the source for the implementation.
GitLab results (requires registration/login) - no results

I would also like to do a full-text search of the WordPress.org plugin directory to confirm there are no other usages, but I am not sure if there is an easy way to do that.

I think we should be OK, as we don't have any WooCommerce or Automattic usage, but I wanted to triple-check.

…WooCommerce Stripe Gateway (#65965) Remove get_entry_count() from product feed FeedInterface PR #64394 added get_entry_count() as a required method on FeedInterface, shipping in 10.9.0. Because the interface is implemented by third-party code — notably the WooCommerce Stripe Gateway's WC_Stripe_Agentic_Commerce_Csv_Feed — adding a required method is a backward-incompatible change. Stripe versions that predate their own get_entry_count() implementation (added in Stripe 10.7) fatal on load under 10.9.0: Class WC_Stripe_Agentic_Commerce_Csv_Feed contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (...FeedInterface::get_entry_count) No core code calls get_entry_count() through the interface; the method is only used on the concrete JsonFileFeed (and by consumers on their own feed classes). Remove it from the interface to restore compatibility, keeping the implementation and its tests on JsonFileFeed intact. Refs #64394

…try_count() breaking older WooCommerce Stripe Gateway (#65972) Fix fatal error from FeedInterface::get_entry_count() breaking older WooCommerce Stripe Gateway (#65965) Remove get_entry_count() from product feed FeedInterface PR #64394 added get_entry_count() as a required method on FeedInterface, shipping in 10.9.0. Because the interface is implemented by third-party code — notably the WooCommerce Stripe Gateway's WC_Stripe_Agentic_Commerce_Csv_Feed — adding a required method is a backward-incompatible change. Stripe versions that predate their own get_entry_count() implementation (added in Stripe 10.7) fatal on load under 10.9.0: Class WC_Stripe_Agentic_Commerce_Csv_Feed contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (...FeedInterface::get_entry_count) No core code calls get_entry_count() through the interface; the method is only used on the concrete JsonFileFeed (and by consumers on their own feed classes). Remove it from the interface to restore compatibility, keeping the implementation and its tests on JsonFileFeed intact. Refs #64394 Co-authored-by: Cvetan Cvetanov <cvetan.cvetanov@automattic.com>

wjrosa self-assigned this Apr 24, 2026

wjrosa requested review from a team, daledupreez and malithsen and removed request for a team April 24, 2026 12:31

wjrosa marked this pull request as ready for review April 24, 2026 12:31

coderabbitai Bot reviewed Apr 24, 2026

View reviewed changes

github-actions Bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Apr 24, 2026

malithsen approved these changes Apr 27, 2026

View reviewed changes

daledupreez reviewed May 7, 2026

View reviewed changes

Comment thread plugins/woocommerce/src/Internal/ProductFeed/Feed/FeedInterface.php Outdated

wjrosa and others added 2 commits May 12, 2026 13:00

Merge branch 'trunk' into add/feed-interface-get-entry-count

c9ee336

wjrosa requested a review from daledupreez May 12, 2026 16:40

daledupreez approved these changes May 13, 2026

View reviewed changes

Merge branch 'trunk' into add/feed-interface-get-entry-count

119ad88

wjrosa enabled auto-merge (squash) May 13, 2026 12:49

wjrosa merged commit f7b43d3 into trunk May 13, 2026
37 of 40 checks passed

wjrosa deleted the add/feed-interface-get-entry-count branch May 13, 2026 13:15

faisalahammad mentioned this pull request Jun 12, 2026

feat(product-feed): add query provider for pull integrations (#65389) #65418

Open

mordeth mentioned this pull request Jun 24, 2026

Fix fatal error from FeedInterface::get_entry_count() breaking older WooCommerce Stripe Gateway #65965

Merged

15 tasks

woocommercebot mentioned this pull request Jun 24, 2026

[Backport to release/10.9] Fix fatal error from FeedInterface::get_entry_count() breaking older WooCommerce Stripe Gateway #65972

Merged

15 tasks

coderabbitai Bot mentioned this pull request Jun 24, 2026

Release: Remove 10.9.1 change files from trunk #65994

Merged

Uh oh!

Conversation

wjrosa commented Apr 24, 2026

Submission Review Guidelines

Changes proposed in this Pull Request

Summary of changes

How to test the changes in this Pull Request

Testing that has already taken place

Changelog entry

Significance

Type

Message

Comment

Comment

Uh oh!

coderabbitai Bot commented Apr 24, 2026

Walkthrough

Changes

Estimated code review effort

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing Guidelines

Uh oh!

github-actions Bot commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test using WordPress Playground

Uh oh!

malithsen left a comment

Choose a reason for hiding this comment

Uh oh!

daledupreez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wjrosa commented May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjrosa commented May 12, 2026

Uh oh!

coderabbitai Bot commented May 12, 2026

Uh oh!

daledupreez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Apr 24, 2026 •

edited

Loading

github-actions Bot commented Apr 24, 2026 •

edited

Loading

wjrosa commented May 12, 2026 •

edited

Loading