Allow shipping calculators to throw an exception or return null

[mbabker]

If your store is using a shipping calculator with a dynamic shipping rate based on a third party API (one of my stores uses FedEx or USPS depending on location), there may be times where a shipping rate cannot be calculated for whatever reason (bad user input resulting in no matching location, location not being some place the provider supports shipping to, or an API outage being good examples of error scenarios I've had to account for).

However, Sylius\Component\Shipping\Calculator\CalculatorInterface::calculate() isn't documented as supporting any thrown exceptions (even though the calculators in the Core component do throw Sylius\Component\Core\Exception\MissingChannelConfigurationException in a misconfigured channel scenario) and does not support a nullable return, meaning there isn't any inbuilt mechanism to signal that a shipping rate could not be calculated.

It would be nice to have a mechanism that allows a calculator to signal that a rate could not be calculated for a shipping method that would allow developers to consistently handle these types of scenarios in some kind of graceful manner.

[stale]

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

[Zales0123]

Hello Michael! Sorry for the long response time. The explained idea seems reasonable, something like ShippingMethodCalculationException (or sth similar) would definitely increase the extendability of this feature 👍 We should only keep in mind, that the exception should not be just added, but needs to be already used somewhere (to show the use case) and properly tested.

Would you like to open a PR with the first iteration with this solution @mbabker? It would be wonderful 🖖

[mbabker]

but needs to be already used somewhere

That might be a hangup for implementing this. I can add the exception, I can document it, but I can't think of a way to implement it in the existing core calculators or anything in the core Sylius application that uses them; the provided calculators are all fixed rate and anything that would trigger this new exception would presumably be handled in the existing shipping method integrations (API or admin form validation). The only thing that might be able to handle this exception in the core Sylius build is the ShowAvailableShippingMethodsController::getCalculatedShippingMethods() method in the AdminApiBundle to show an alternative message if the rate is unavailable (I'd avoid doing anything in the ShippingMethodChoiceType form because there are a lot of error and recovery scenarios you'd have to deal with if the rate isn't available when using that particular form).

A client request for one of our Sylius applications was a shipping estimator to show estimated shipping costs on the cart page before going into checkout, this client has multiple shipping options available and uses dynamic shipping costs calculated from third party APIs (FedEx and UPS mainly). Relevant code snippet for one of our controllers building the data:

public function estimateShippingAction(Request $request): Response
{
    // Form handling and data validation logic

    $adjustmentFactory  = $this->get('sylius.factory.adjustment');
    $calculatorRegistry = $this->get('sylius.registry.shipping_calculator');
    $moneyFormatter     = $this->get('sylius.money_formatter');

    $shippingOptions = [];

    /** @var ShippingMethodInterface $shippingMethod */
    foreach ($this->get('sylius.shipping_methods_resolver')->getSupportedMethods($shipment) as $shippingMethod) {
        /** @var CalculatorInterface $calculator */
        $calculator = $calculatorRegistry->get($shippingMethod->getCalculator());

        $adjustment = $adjustmentFactory->createWithData(
            AdjustmentInterface::SHIPPING_ADJUSTMENT,
            $shippingMethod->getName(),
            $calculator->calculate($shipment, $shippingMethod->getConfiguration()) // <-- Would like to catch shipping calculation exceptions here, without handling other possible exceptions
        );

        $shippingOptions[] = [
            'name' => $shippingMethod->getName(),
            'rate' => $moneyFormatter->format($adjustment->getAmount(), $cart->getCurrencyCode()),
        ];
    }

    if (empty($shippingOptions)) {
        return new JsonResponse(['error' => true, 'options' => [], 'reason' => 'shipping_not_available']);
    }

    return new JsonResponse(['error' => false, 'options' => $shippingOptions, 'reason' => null]);
}

In that foreach loop, I could handle an exception specifically from the shipping calculator to show a "rate not available" type of message if there is an API outage, but without a dedicated exception my only options are to catch \Exception and handle every error (including the MissingChannelConfigurationException, which I'm not really interested in doing here), add local application exceptions for my calculators only, or to not catch any exceptions at all (as is the case now).

[vvasiloi]

@mbabker Should a shipping method be unavailable if the price calculation for it fails?
If so, then one possible place to use these exceptions would be the implementations of Sylius\Component\Shipping\Resolver\ShippingMethodsResolverInterface, maybe just in a new one to the chain.
Another place that could use such exception is Sylius\Component\Core\OrderProcessing\ShippingChargesProcessor.

[mbabker]

If so, then one possible place to use these exceptions would be the implementations of Sylius\Component\Shipping\Resolver\ShippingMethodsResolverInterface, maybe just in a new one to the chain.

Hmm, yeah, that could be an option here.

Another place that could use such exception is Sylius\Component\Core\OrderProcessing\ShippingChargesProcessor.

That's the class I kept forgetting how to find 🤦 , that would be a good spot to handle it too (as well as documenting DelegatingCalculatorInterface::calculate() to (re-)throw the same exceptions that CalculatorInterface::calculate() does.

Let me simmer on this for a few days and see what I can come up with.