From 51eb12d98076bf5c7781f5ad304be018b939b7a8 Mon Sep 17 00:00:00 2001 From: Usman Khan Date: Sat, 29 Aug 2026 00:08:00 +0500 Subject: [PATCH] Replace a republished shipping option, and reset the manifest per cart --- packages/core/src/Base/ShippingManifest.php | 33 ++++++++-- tests/core/Unit/Base/ShippingManifestTest.php | 64 +++++++++++++++++++ 2 files changed, 91 insertions(+), 6 deletions(-) diff --git a/packages/core/src/Base/ShippingManifest.php b/packages/core/src/Base/ShippingManifest.php index 2e6ccfc5d2..99543987d0 100644 --- a/packages/core/src/Base/ShippingManifest.php +++ b/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 { diff --git a/tests/core/Unit/Base/ShippingManifestTest.php b/tests/core/Unit/Base/ShippingManifestTest.php index 1d12eacae9..9d1f63262e 100644 --- a/tests/core/Unit/Base/ShippingManifestTest.php +++ b/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); +});