Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
thlassche opened this issue Mar 1, 2018 · 24 comments
Closed

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

thlassche opened this issue Mar 1, 2018 · 24 comments

Comments

@thlassche
Copy link
Contributor

@thlassche thlassche commented Mar 1, 2018

Preconditions

  1. Magento version >= 2.2

Steps to reproduce

  1. 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>
  1. Login as customer
  2. Go to the checkout, select created carrier and see that for example POST set-payment-information fails

Expected result

  1. Order placement works

Actual result

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

This comment has been minimized.

Copy link
Contributor

@magento-engcom-team magento-engcom-team commented Mar 1, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

@thlassche thlassche commented Mar 6, 2018

@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

This comment has been minimized.

Copy link
Contributor

@kanduvisla kanduvisla commented Mar 12, 2018

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

This comment has been minimized.

Copy link
Contributor

@kanduvisla kanduvisla commented Mar 12, 2018

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

This comment has been minimized.

Copy link
Contributor Author

@thlassche thlassche commented Mar 12, 2018

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

@davidverholen

This comment has been minimized.

Copy link
Member

@davidverholen davidverholen commented Mar 19, 2018

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

This comment has been minimized.

Copy link
Contributor

@hostep hostep commented Apr 3, 2018

@irenelagno or @nmalevanec: since you guys introduced this code in 3495e60 and 179f984, 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

This comment has been minimized.

Copy link
Contributor

@kanduvisla kanduvisla commented Apr 3, 2018

@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

This comment has been minimized.

Copy link
Contributor

@koenner01 koenner01 commented Apr 3, 2018

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

This comment has been minimized.

Copy link
Contributor

@pocallaghan pocallaghan commented Apr 3, 2018

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

This comment has been minimized.

Copy link
Contributor

@hostep hostep commented Apr 3, 2018

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

This comment has been minimized.

Copy link
Member

@davidverholen davidverholen commented Apr 3, 2018

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

This comment has been minimized.

Copy link
Contributor

@kanduvisla kanduvisla commented Apr 4, 2018

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

This comment has been minimized.

Copy link
Contributor

@hostep hostep commented Apr 12, 2018

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

This comment has been minimized.

Copy link
Contributor

@EliasKotlyar EliasKotlyar commented May 28, 2018

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

This comment has been minimized.

Copy link
Contributor

@matthew-muscat matthew-muscat commented Jun 4, 2018

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

This comment has been minimized.

Copy link
Contributor

@paliarush paliarush commented Jun 4, 2018

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

@engcom-backlog-pb

This comment has been minimized.

Copy link

@engcom-backlog-pb engcom-backlog-pb commented Aug 6, 2018

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

@engcom-backlog-pb engcom-backlog-pb removed their assignment Aug 6, 2018
@kanduvisla

This comment has been minimized.

Copy link
Contributor

@kanduvisla kanduvisla commented Oct 15, 2018

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 ... :-(

@vpodorozh vpodorozh self-assigned this Oct 24, 2018
@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

@magento-engcom-team magento-engcom-team commented Oct 24, 2018

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.

@vpodorozh vpodorozh removed their assignment Oct 24, 2018
matthew-muscat added a commit to Go-Shippit/Magento2 that referenced this issue Nov 12, 2018
- update the click and collect method instance name to remove the underscore
- ensures backwards-compatibility is maintained for orders already created using the older method instance name
- resolves a magento core bug introduced in Magento v2.1 with regards to handling shipping methods that contain an underscore in the instance name (magento/magento2#13902)

Signed-off-by: Matthew Muscat <matthew@mamis.com.au>
@hostep

This comment has been minimized.

Copy link
Contributor

@hostep hostep commented Jan 24, 2019

Hi folks

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

For Magento 2.3:

For Magento 2.2:

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

This comment has been minimized.

Copy link

@VitorGit VitorGit commented Jan 28, 2019

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

This comment has been minimized.

Copy link
Contributor

@sdzhepa sdzhepa commented Mar 4, 2019

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

This comment has been minimized.

Copy link
Contributor

@magento-engcom-team magento-engcom-team commented Apr 18, 2019

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

The fix will be available with the upcoming 2.2.9 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.