magento2: Carrier codes with '_' (underscores) break several payment API's

so is the solution to not use underscores in either the carrier or the method? It’s kinda scary that such a simple thing can have such devastating impact. Is modifying the order table to store the carrier and method separately not an option? This is closed, but is very much still an issue, as explained by the #31311 issue.

Is the quick solution to modify any custom shipping method or carrier that has an underscore to use a hyphen or something instead?

Hi folks

It looks like this issue gets resolved by the following PR’s.

For Magento 2.3:

• logged in users: https://github.com/magento/magento2/pull/18689
• guests: https://github.com/magento/magento2/pull/19505

For Magento 2.2:

• logged in users: https://github.com/magento/magento2/pull/18870
• guests: https://github.com/magento/magento2/pull/21340

I’ve verified the fix for logged in users, not yet for guest users as I was using Magento 2.2.3 for my testing.

Update 2019-02-19: added backport for guest users for Magento 2.2

I have also encountered this bug after upgrading M2.1 to M2.2. It took a while to debug this issue. I have made a small module which has the fix from @kanduvisla. It can be found here: https://github.com/EliasKotlyar/Twinsen_CarrierCodeFix/tree/master

I would also give my +1 to separate the carrier code and shipping method code in the database. It makes it much more consistent and easier to work with. But I also would like to hear what @magento-engcom-team has to say about this. For example:

• What was the original reason that they are merged?
• What places of Magento will it affect if we separate it? (code, API, invoices, PDF’s, e-mail, etc.)

@hostep: I don’t exactly understand what you mean. The problem is that Magento concatenates the carrier code and the shipping method as one string, and later on tries to ‘guess’ it back, using a string. This works great for codes like flatrate_flatrate, or dhl_express, but what if you have:

• A carrier foo with the method foo_foo, and:
• A carrier foo_foo with the method foo.

Now Magento gets the string foo_foo_foo. How can it determine which carrier / method are chosen here? I think the main issue here is an architectural one: because the shipping_method is saved as a concatenated string, issues like this are unavoidable. This can be fixed in various ways:

• Add a validation to carrier_code and shipping_method, stating that it cannot contain an underscore (might break several carrier modules).
• Use something else as glue for the concatenating (for example: carrier_code::shiping_method). Might break BC for older quotes / orders that already exist in the database).
• Split shipping_method-column in database with extra carrier_code-column. Might be a big commit to the code, but is more in line with the carrier/method-model, and BC can be achieved by checking if carrier_code is null in the database.

edit: @hostep: upon reading your comment again I do know what you mean and no: carrier code and shipping method are not always the same. They are often the same if a carrier only has one method, but it’s perfectly fine for a carrier to have multiple shipping methods. In that case the code of the shipping method differs.

having the same problem in 2.1.11 since the carrier code is just determined by exploding the shipping method with ‘_’. For every shipping carrier containing an underscore the checkout does not work anymore

see: https://github.com/magento/magento2/blob/2.1-develop/app/code/Magento/Checkout/Model/PaymentInformationManagement.php#L124

For 2.1 this was introduced in the update from 2.1.10 to 2.1.11

As a temporary solution (at least for 2.1.12) I came op with a plugin that fixes this for 99.9%.

di.xml:

<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:noNamespaceSchemaLocation="urn:magento:framework:ObjectManager/etc/config.xsd">
    <!--
        Addresses GITHUB-13902, where logged in customers cannot use shipping carriers that contain an underscore
        @see https://github.com/magento/magento2/issues/13902
    -->
    <type name="Magento\Checkout\Model\PaymentInformationManagement">
        <plugin name="fix-carriers-with-underscore-bug" type="Vendor\Module\Plugin\Magento\Checkout\Model\PaymentInformationManagement"/>
    </type>
</config>

app/code/Vendor/Module/Plugin/Magento/Checkout/Model/PaymentInformationManagement.php:

<?php

namespace Vendor\Module\Plugin\Magento\Checkout\Model;

use Magento\Framework\App\Config\ScopeConfigInterface;
use Magento\Quote\Api\CartRepositoryInterface;
use Magento\Quote\Api\Data\AddressInterface;
use Magento\Quote\Api\Data\PaymentInterface;
use Magento\Quote\Api\PaymentMethodManagementInterface;
use Magento\Store\Model\ScopeInterface;

/**
 * Class PaymentInformationManagement
 * @package Vendor\Module\Plugin\Magento\Checkout\Model
 */
class PaymentInformationManagement
{
    /**
     * @var PaymentMethodManagementInterface
     */
    protected $paymentMethodManagement;

    /**
     * @var ScopeConfigInterface
     */
    protected $scopeConfig;

    /**
     * @var CartRepositoryInterface
     */
    protected $cartRepository;

    /**
     * PaymentInformationManagement constructor.
     * @param PaymentMethodManagementInterface $paymentMethodManagement
     * @param ScopeConfigInterface $scopeConfig
     * @param CartRepositoryInterface $cartRepository
     */
    public function __construct(
        PaymentMethodManagementInterface $paymentMethodManagement,
        ScopeConfigInterface $scopeConfig,
        CartRepositoryInterface $cartRepository
    ) {
        $this->paymentMethodManagement = $paymentMethodManagement;
        $this->scopeConfig = $scopeConfig;
        $this->cartRepository = $cartRepository;
    }

    /**
     * @param \Magento\Checkout\Model\PaymentInformationManagement $subject
     * @param callable $proceed
     * @param $cartId
     * @param PaymentInterface $paymentMethod
     * @param AddressInterface|null $billingAddress
     * @return bool
     * @throws \Magento\Framework\Exception\NoSuchEntityException
     * @throws \Magento\Framework\Exception\State\InvalidTransitionException
     */
    public function aroundSavePaymentInformation(
        \Magento\Checkout\Model\PaymentInformationManagement $subject,
        callable $proceed,
        $cartId,
        PaymentInterface $paymentMethod,
        AddressInterface $billingAddress = null
    ) {
        if ($billingAddress) {
            /** @var \Magento\Quote\Model\Quote $quote */
            $quote = $this->cartRepository->getActive($cartId);
            $quote->removeAddress($quote->getBillingAddress()->getId());
            $quote->setBillingAddress($billingAddress);
            $quote->setDataChanges(true);
            $shippingAddress = $quote->getShippingAddress();
            if ($shippingAddress && $shippingAddress->getShippingMethod()) {
                $shippingDataArray = explode('_', $shippingAddress->getShippingMethod());
                $shippingCarrier = array_shift($shippingDataArray);
                // Start fix for GITHUB-13902
                while (!$this->isExistingCarrier($shippingCarrier)) {
                    $shippingCarrier .= '_' . array_shift($shippingDataArray);
                }
                // End fix for GITHUB-13902
                $shippingAddress->setLimitCarrier($shippingCarrier);
            }
        }
        $this->paymentMethodManagement->set($cartId, $paymentMethod);

        return true;
    }

    /**
     * @param string $shippingCarrier
     * @return bool
     */
    protected function isExistingCarrier(string $shippingCarrier): bool
    {
        $carrierConfig = $this->scopeConfig->getValue('carriers/' . $shippingCarrier, ScopeInterface::SCOPE_STORE);
        return !empty($carrierConfig);
    }
}

The 0.1% that is not fixed is if there are carriers that have matching names with underscores. For example: carrier_foo and carrier_foo_bar. In this case, when carrier_foo_bar is selected, it will be rebuild and matched with carrier_foo. But as I said: it’s a temporary fix and it’s currently design to only work in my situation but perhaps it will help someone.

Another risk is an infinite loop if the store owner deletes or disables the selected carrier in between selecting the shipping method and completing the order 😛

@magento-engcom-team I saw this when using it in combination with onestepcheckout. Please see here for the code that’s definitely wrong in my opinion:

https://github.com/magento/magento2/blame/2.3-develop/app/code/Magento/Checkout/Model/PaymentInformationManagement.php#L118

You can’t explode by ‘_’, compare it with this one:

https://github.com/magento/magento2/blob/2.3-develop/app/code/Magento/Checkout/Model/ShippingInformationManagement.php#L160