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

thlassche commented 6 years ago

Preconditions

Magento version >= 2.2

Steps to reproduce

Create a carrier with carrier code 'test_carrier' for example config.xml:

<?xml version="1.0"?>
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:module:Magento_Store:etc/config.xsd">
<default>
    <carriers>
        <test_carrier>
            <active>1</active>
            <title>Flatrate</title>
            <name>Flatrate 1</name>
            <price>0</price>
            <model>Namespace\Vendor\Model\Carrier\Flatrate1</model>
            <sallowspecific>0</sallowspecific>
            <specificerrmsg>This shipping method is not available.</specificerrmsg>
        </test_carrier>
  </carriers>
</default>
</config>

Login as customer
Go to the checkout, select created carrier and see that for example POST set-payment-information fails

Expected result

Order placement works

Actual result

Several web api request fail with 400 bad request, please specify a shipping method

The cause of this is several explode's by '_' in the code, where $addressInformation->getShippingCarrierCode() should have been used in my opinion. Example: https://github.com/magento/magento2/blame/2.3-develop/app/code/Magento/Checkout/Model/PaymentInformationManagement.php#L118

magento-engcom-team commented 6 years ago

@thlassche, thank you for your report. We were not able to reproduce this issue by following the steps you provided. Please provide more detailed steps to reproduce.

thlassche commented 6 years ago

@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

kanduvisla commented 6 years ago

I have this same problem with Magento 2.1.12. And only with logged in customers. This problem does not seem to affect guest checkout.

The bpost shipping carrier has carrier codes with underscores, like:

bpost_homedelivery
bpost_pickuppoint
bpost_clickcollect
etc.

The code in Magento\Checkout\Model\PaymentInformationManagement::savePaymentInformation() explodes the selected shipping carrier by an underscore character and sets this as limit_carrier:

// Shipping method is bpost_homedelivery_bpost_homedelivery for example:
$shippingDataArray = explode('_', $shippingAddress->getShippingMethod());
$shippingCarrier = array_shift($shippingDataArray);
// Carrier is now 'bpost', which does not exist:
$shippingAddress->setLimitCarrier($shippingCarrier);

I'm still looking where something like getLimitCarrier is used to re-attach the shipping method to the quote address (and collect the proper rates), but for now this appears to be a serious issue. It effectively renders the checkout unable to complete an order for carriers that have an underscore in their name.

In my opinion there are two options here:

Fix savePaymentInformation() to do extra checks for carriers with more underscores.
Do some validation on carrier codes upfront for developers (like, immediately throw an exception that underscores are not allowed in carrier codes for example).

kanduvisla commented 6 years ago

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 :-P

thlassche commented 6 years ago

@kanduvisla Thanks, added the login step to the steps to reproduce section

davidverholen commented 6 years ago

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

hostep commented 6 years ago

@irenelagno or @nmalevanec: since you guys introduced this code in https://github.com/magento/magento2/commit/3495e603b5789ff50eb4e33d07b26820d7e759ff and https://github.com/magento/magento2/commit/179f98416618ccf6afb7c50f10fd32fb7b98b856, would you be so kind to review the above discussion and see if there is a good solution for this problem?

@kanduvisla: would using a regular expression to try and extract the carrier work as well? Something like (.+)_\1 for example? I haven't debugged this yet, so I don't know for certain if the first part and second part of the shipping method are always exactly the same with all shipping methods, but if they are the same, the regex should work I think?

kanduvisla commented 6 years ago

@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.

koenner01 commented 6 years ago

Imo this is a pretty serious issue; If Magento would choose not to allow an underscore in the carrier code & method there will be a lot of people unhappy and a lot of extra work will be needed for people to fix this.

Of the above solutions I would prefer the 3th option as it indeed follows carrier/method model.

pocallaghan commented 6 years ago

In M1, I've seen shipping method codes with_ in, but never a carrier code. This was partially enforced by the code in Mage_Sales_Model_Order::getShippingMethod, which does this list($carrierCode, $method) = explode('_', $shippingMethod, 2); (looks like it does similar in M2). Whilst there may be different usage in other places, this code sort of implies theres a contract that a carrier code can't include an underscore.

hostep commented 6 years ago

Thanks for the feedback all! I've had some chance to dig into this a bit further in the mean time.

So here's my opinion: If Magento allowed the carrier code to have underscores in version 2.1.10 and before, then it should still allow to do this, otherwise we have BC breakage (like we have now in 2.1.11, 2.1.12 & 2.2.x).

So we need to replace the splitting of carrier & shipping method with underscore by something else, that was indeed M1 behavior, but I don't think it applies to M2 anymore. The carrier code should be available without extracting it out of another string.

The only thing we need is figure out a way to replace these lines with the carrier code which we need to get from somewhere.

The question is: where should we get it from? Possible solutions:

when saving the payment information, send the carrier code from the frontend, as an extra param in the ajax request
when storing the shipping method in the first step of the checkout, store it separately either in the database or maybe just in the session and get it from there when saving the payment information

Just my 2 cents.

Maybe @magento-engcom-team can try to invite some core devs in here to figure out the best solution for this problem?

davidverholen commented 6 years ago

although it might be a bigger change, I think it would solve the problem for good to separate carrier code and shipping method code.

They are separated (and also needed to be separated) nearly everywhere. Only at this point in the database for some reason they are merged. This really makes no sense.

kanduvisla commented 6 years ago

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 commented 6 years ago

I've just noticed this wiki page, so let's try to get @paliarush involved in here since he is in charge of the Checkout modules.

EliasKotlyar commented 6 years ago

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

matthew-muscat commented 6 years ago

Confirming we're also seeing this issue.

@paliarush — are we able to get confirmation that this can be treated as a bug that needs to be fixed?

paliarush commented 6 years ago

@thlassche thanks for reporting the issue. Internal ticket has been created MAGETWO-92416

ghost commented 6 years ago

@thlassche, thank you for your report. We've acknowledged the issue and added to our backlog.

kanduvisla commented 5 years ago

In Magento 2.2.6 I experienced the issue again. The problem now also seem to persist in \Magento\Checkout\Model\GuestPaymentInformationManagement::limitShippingCarrier()

Too bad this is a private method, this makes the temporary fix uglier ... :-(

magento-engcom-team commented 5 years ago

Hi @vpodorozh. Thank you for working on this issue. Looks like this issue is already verified and confirmed. But if your want to validate it one more time, please, go though the following instruction:

[ ] 1. Add/Edit Component: XXXXX label(s) to the ticket, indicating the components it may be related to.
[ ] 2. Verify that the issue is reproducible on 2.3-develop branch
Details
- Add the comment @magento-engcom-team give me 2.3-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
[ ] 3. Verify that the issue is reproducible on 2.2-develop branch.
Details
- Add the comment @magento-engcom-team give me 2.2-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.2-develop branch, please add the label Reproduced on 2.2.x
[ ] 4. If the issue is not relevant or is not reproducible any more, feel free to close it.

hostep commented 5 years ago

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

VitorGit commented 5 years ago

I just wasted two days of work because of this issue. Magento concatenates the carrier code and shipping method with underscores:

Up_Express

and uses explode('_') and unshift() to get the name of the carrier and put it on setLimitCarrier() hence it only gets the part of the name before the underscore even tho the name is properly stored in the database and everything!

sdzhepa commented 5 years ago

Hello @thlassche @VitorGit @hostep

Thank you all for contribution and collaboration!

As I can see from the comment #13902 (comment) the issue is fixed and delivered

There is Open PR #21340 that will be processed and delivered soon. So, this issue can be closed

magento-engcom-team commented 5 years ago

Hi @thlassche. Thank you for your report. The issue has been fixed in magento/magento2#21340 by @hostep in 2.2-develop branch Related commit(s):

The fix will be available with the upcoming 2.2.9 release.

PivitParkour94 commented 3 years ago

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?

magento / magento2