Replace a republished shipping option, and reset the manifest per cart

packages/core/src/Base/ShippingManifest.php

@@ -22,6 +22,11 @@ class ShippingManifest implements ShippingManifestInterface
      */
     protected bool $resolving = false;
 
+    /**
+     * The cart the options currently in the manifest were resolved for.
+     */
+    protected ?int $resolvedFor = null;
+
     /**
      * Initiate the class.
      */
@@ -35,13 +40,18 @@ public function __construct()
      */
     public function addOption(ShippingOption $option)
     {
-        $exists = $this->options->first(function ($opt) use ($option) {
-            return $opt->getIdentifier() == $option->getIdentifier();
-        });
-
-        // Does this option already exist?
-        if (! $exists) {
+        // Publishing an identifier that is already here replaces it. Skipping
+        // instead meant the first price ever computed for an identifier was the
+        // only one that counted: a modifier re-pricing the same option for a
+        // changed destination or basket had its answer discarded.
+        $index = $this->options->search(
+            fn ($opt) => $opt->getIdentifier() == $option->getIdentifier()
+        );
+
+        if ($index === false) {
             $this->options->push($option);
+        } else {
+            $this->options->put($index, $option);
         }
 
         return $this;
@@ -89,6 +99,17 @@ public function getOptions(Cart $cart): Collection
             return $this->options;
         }
 
+        // The manifest is a singleton, so in any process that handles more than
+        // one cart - a queue job, a console command, an Octane worker - options
+        // priced for the last cart are still here. Start clean when the cart
+        // changes; options added for THIS cart, including any published before
+        // the first resolve, are left alone.
+        if ($this->resolvedFor !== null && $this->resolvedFor !== $cart->id) {
+            $this->options = collect();
+        }
+
+        $this->resolvedFor = $cart->id;
+
         $this->resolving = true;
 
         try {

tests/core/Unit/Base/ShippingManifestTest.php

@@ -285,3 +285,67 @@
     // a single calculate runs the modifiers twice. Neither run may re-enter.
     expect(TestRecursiveShippingModifier::$calls)->toEqual(2);
 });
+
+test('can replace an option that is published again', function () {
+    $taxClass = TaxClass::factory()->create();
+
+    $option = fn (int $price) => new ShippingOption(
+        name: 'Standard delivery',
+        description: 'Standard delivery',
+        identifier: 'STANDARD',
+        price: new Price($price, $this->cart->currency, 1),
+        taxClass: $taxClass
+    );
+
+    ShippingManifest::addOption($option(500));
+    ShippingManifest::addOption($option(2500));
+
+    // One option, at the price it was last published with - a modifier
+    // re-pricing for a changed destination must not be ignored.
+    expect(ShippingManifest::getOptions($this->cart))->toHaveCount(1);
+    expect(ShippingManifest::getOptions($this->cart)->first()->price->value)->toEqual(2500);
+});
+
+test('can not offer one cart the options priced for another', function () {
+    $taxClass = TaxClass::factory()->create();
+
+    ShippingManifest::addOption(
+        new ShippingOption(
+            name: 'Standard delivery',
+            description: 'Standard delivery',
+            identifier: 'STANDARD',
+            price: new Price(500, $this->cart->currency, 1),
+            taxClass: $taxClass
+        )
+    );
+
+    expect(ShippingManifest::getOptions($this->cart))->toHaveCount(1);
+
+    // The next cart in the same process - a queue job, a console command, an
+    // Octane worker - must not inherit them.
+    $other = Cart::factory()->create([
+        'currency_id' => $this->cart->currency_id,
+    ]);
+
+    expect(ShippingManifest::getOptions($other))->toHaveCount(0);
+});
+
+test('can keep options published for the cart being resolved', function () {
+    $taxClass = TaxClass::factory()->create();
+
+    ShippingManifest::addOption(
+        new ShippingOption(
+            name: 'Standard delivery',
+            description: 'Standard delivery',
+            identifier: 'STANDARD',
+            price: new Price(500, $this->cart->currency, 1),
+            taxClass: $taxClass
+        )
+    );
+
+    // Publishing straight onto the manifest, rather than from inside a
+    // modifier, is how much of the suite sets shipping up. Resolving the same
+    // cart must not discard it.
+    expect(ShippingManifest::getOptions($this->cart))->toHaveCount(1);
+    expect(ShippingManifest::getOptions($this->cart))->toHaveCount(1);
+});