Commit c14822b35fb for woocommerce
commit c14822b35fba961ca32025fdd08c533f6b3be950
Author: Saskia Teichmann <s-a-s-k-i-a@users.noreply.github.com>
Date: Wed Jul 29 22:56:18 2026 +0200
Fix add_item() silently overwriting an unsaved order item on remove + re-add (#65607)
* Fix add_item() silently overwriting an unsaved order item on remove + re-add
WC_Abstract_Order::add_item() keyed unsaved items by count(), which is not
collision-safe across a remove + add cycle: after a removal the count repeats a
previous value, so two distinct unsaved items map to the same 'new:<type><N>'
key and the second silently overwrites the first (lost on save()). Replace the
count()-based suffix with a per-order monotonic counter that is never reused.
Adds a regression test in WC_Abstract_Order_Test.
* Fix temp key collision test to read item names via get_name()
wp_list_pluck() delegates to WP_List_Util::pluck(), which reads object
fields with bare property access ($value->$field) and no isset guard.
WC_Order_Item extends WC_Data and exposes 'name' only through
ArrayAccess::offsetGet(); neither it nor WC_Data defines __get. The
pluck therefore returned array( null, null ) and the assertions failed
even with the production fix applied. Read the names through the
public get_name() accessor instead.
Also correct the since tag on the new temp_item_id_counter property:
trunk is 11.1.0-dev, not 10.9.0.
* Capitalize doc comment long description to satisfy phpcs
Fixes the phpcs-changed lint failure Generic.Commenting.DocComment.LongNotCapital at abstract-wc-order.php:97.
---------
Co-authored-by: Brandon Kraft <public@brandonkraft.com>
diff --git a/plugins/woocommerce/changelog/fix-add-item-temp-key-collision b/plugins/woocommerce/changelog/fix-add-item-temp-key-collision
new file mode 100644
index 00000000000..db341c999cf
--- /dev/null
+++ b/plugins/woocommerce/changelog/fix-add-item-temp-key-collision
@@ -0,0 +1,4 @@
+Significance: patch
+Type: fix
+
+Prevent WC_Abstract_Order::add_item() from silently overwriting an unsaved item when items are removed and re-added before save() (count()-based temporary key collision).
diff --git a/plugins/woocommerce/includes/abstracts/abstract-wc-order.php b/plugins/woocommerce/includes/abstracts/abstract-wc-order.php
index b41c1b14361..b33ab3fad08 100644
--- a/plugins/woocommerce/includes/abstracts/abstract-wc-order.php
+++ b/plugins/woocommerce/includes/abstracts/abstract-wc-order.php
@@ -90,6 +90,21 @@ abstract class WC_Abstract_Order extends WC_Abstract_Legacy_Order {
*/
protected $items_to_delete = array();
+ /**
+ * Monotonic counter used to build collision-free temporary array keys for
+ * not-yet-persisted items.
+ *
+ * Using count() is not safe for this: when items are removed and re-added before
+ * save(), the count repeats a previous value, so two distinct unsaved items
+ * map to the same 'new:<type><N>' key and the second silently overwrites the
+ * first (it is then lost on save()). A never-reused counter guarantees a
+ * unique key for every unsaved item for the lifetime of the object.
+ *
+ * @since 11.1.0
+ * @var int
+ */
+ protected $temp_item_id_counter = 0;
+
/**
* Bulk order item types scheduled for deletion on save().
*
@@ -1298,7 +1313,7 @@ abstract class WC_Abstract_Order extends WC_Abstract_Legacy_Order {
if ( $item_id ) {
$this->items[ $items_key ][ $item_id ] = $item;
} else {
- $this->items[ $items_key ][ 'new:' . $items_key . count( $this->items[ $items_key ] ) ] = $item;
+ $this->items[ $items_key ][ 'new:' . $items_key . $this->temp_item_id_counter++ ] = $item;
}
}
diff --git a/plugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php b/plugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php
index 06357426cad..6c0db8103ef 100644
--- a/plugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php
+++ b/plugins/woocommerce/tests/php/includes/abstracts/class-wc-abstract-order-test.php
@@ -833,6 +833,48 @@ class WC_Abstract_Order_Test extends WC_Unit_Test_Case {
$this->assertCount( 0, $reloaded_after_save->get_items( 'shipping' ), 'Shipping items should be removed from the DB after save().' );
}
+ /**
+ * @testdox add_item() must not overwrite an earlier unsaved item when items are removed and re-added before save().
+ */
+ public function test_add_item_keeps_unsaved_items_after_remove_and_readd() {
+ $make_fee = function ( $name ) {
+ $fee = new WC_Order_Item_Fee();
+ $fee->set_name( $name );
+ $fee->set_amount( '1' );
+ $fee->set_total( '1' );
+ $fee->set_tax_status( 'none' );
+ return $fee;
+ };
+
+ $order = new WC_Order();
+ $order->add_item( $make_fee( 'Existing 1' ) );
+ $order->add_item( $make_fee( 'Existing 2' ) );
+ $order->save();
+
+ $find = function ( $name ) use ( $order ) {
+ foreach ( $order->get_items( 'fee' ) as $item ) {
+ if ( $name === $item->get_name() ) {
+ return $item;
+ }
+ }
+ return null;
+ };
+
+ // Remove an existing fee and add a fresh one, twice, all before saving.
+ // With count()-based temporary keys the two fresh fees collide on the same
+ // 'new:fee_lines<N>' key and the first ('Fresh A') is silently dropped.
+ $order->remove_item( $find( 'Existing 1' )->get_id() );
+ $order->add_item( $make_fee( 'Fresh A' ) );
+ $order->remove_item( $find( 'Existing 2' )->get_id() );
+ $order->add_item( $make_fee( 'Fresh B' ) );
+ $order->save();
+
+ $names = array_map( fn( $item ) => $item->get_name(), wc_get_order( $order->get_id() )->get_items( 'fee' ) );
+ $this->assertContains( 'Fresh A', $names, 'Earlier unsaved fee must survive a later add_item().' );
+ $this->assertContains( 'Fresh B', $names );
+ $this->assertCount( 2, $names );
+ }
+
/**
* @testdox Should keep original items in the DB if save() never runs after remove_order_items().
*/