Ensure Quote::collectTotals() correctly handles recursion

[fredden]

Description

We have written a plug-in for a shipping method. Our plugin requires access the quote object to perform its business logic. We get a reference to the current quote object by calling Magento\Checkout\Model\Session::getQuote(). This works correctly and we get a reference to the quote object and can perform the required actions.

Within Magento\Checkout\Model\Session::getQuote(), there is a call to $quote->collectTotals(). When this runs via our plug-in, Magento ends up calling $quote->collectTotals() within an existing call to $quote->collectTotals() (recursion). This becomes a problem when we introduce gift cards, which is a feature of Adobe Commerce. Because we are applying the gift cards each time $quote->collectTotals() is called, but the second time around the gift cards are already 'used', so they do not get applied to the $quote, and therefore disappear from the checkout.

Manual testing scenarios

Preconditions:

• Adobe Commerce with Gift Card Accounts feature
• Gift card with an available balance less than the total of the cart
• A plug-in within the shipping method totals collection which calls Magento\Checkout\Model\Session::getQuote()

Steps to reproduce:

1. Add items to cart to ensure cart total is more than available balance on gift card
2. Apply gift card at cart
3. Proceed to payment step in checkout
4. Notice gift card is applied correctly
5. Select payment method multiple times
6. Notice gift card is (not) applied correctly

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• All automated tests passed successfully (all builds are green)

Resolved issues:

1. resolves [Issue] Ensure Quote::collectTotals() correctly handles recursion #37856: Ensure Quote::collectTotals() correctly handles recursion

[m2-assistant]

Hi @fredden. Thank you for your contribution
Here are some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE,
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here

ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[convenient]

Not a Magento maintainer, but spotted this and I have worked on something similar before.

#18678

What is the first thing to initiate collectTotals when you grab it from the session? Is it when it detects a currency code change?

[fredden]

The stack trace / code paths that I'm looking at in this particular case are as follows. (I've taken all the interceptors out and removed lots of around* plug-ins which didn't seem relevant.)

• HTTP POST to REST API
• Magento\Checkout\Model\PaymentInformationManagement->savePaymentInformation()
• Magento\Quote\Model\PaymentMethodManagement->set()
• Magento\Quote\Model\Quote\Payment->importData()
• Magento\Quote\Model\Quote->collectTotals()
• Magento\Quote\Model\Quote\TotalsCollector->collect()
• Magento\Quote\Model\Quote\TotalsCollector->collectAddressTotals()
• Magento\Quote\Model\Quote\Address\Total\Shipping->collect()
• Magento\Quote\Model\Quote\Address->collectShippingRates()
• Magento\Quote\Model\Quote\Address->requestShippingRates()
• Magento\Shipping\Model\Shipping->collectRates()
• Magento\Shipping\Model\Shipping->collectCarrierRates()
• ShipperHQ\Shipper\Model\Carrier\Shipper->collectRates()
• ShipperHQ\Shipper\Model\Carrier\Shipper->setRequest()
• ShipperHQ\Shipper\Model\Carrier\Processor\ShipperMapper->getShipperTranslation()
• Merchant\Module\Plugin\NotRelevantClassName->afterGetCartDetails()
• Magento\Checkout\Model\Session->getQuote()
• Magento\Quote\Model\Quote->collectTotals()

The problem we were investigating is to do with gift cards being assigned to the quote on the first run (which is the inner call) and then being 'used up' by the time the totals are collected for that particular collector the second run (in the outer call). When there's only one call to Magento\Quote\Model\Quote\TotalsCollector->collect() (which is in ...\Quote->collectTotals(), after the flag check), the gift card gets applied correctly.

I'm not aware of any currency code changes in this stack / process.

---

Edit: I don't seem to have properly answered your question. Also, thanks for the link to the previous works; that's helpful.

What is the first thing to initiate collectTotals when you grab it from the session?

This is the line of code that calls Magento\Quote\Model\Quote->collectTotals() within the Magento\Checkout\Model\Session->getQuote() method. (Line 278 of vendor/magento/module-checkout/Model/Session.php, Magento v2.4.5-p1.)

magento2/app/code/Magento/Checkout/Model/Session.php

Lines 277 to 279 in 11846a1

	if ($quote->getTotalsCollectedFlag() === false) {
	$quote->collectTotals();
	}

So perhaps 29f0417 is where the regression was introduced.

[convenient]

Ahhh interesting @fredden

So this does not actually cause an infinite loo? just an additional call to collectTotals? 🤔 I think my original case may not have caught this due to how the rest api instantiates the quote separately from the checkout session singleton.

Historically I have worked around this kind of thing by getting the quote from the plugin/event object rather than trying to grab it from the session, and it seems others have posted similar on stack exchange. Just in case you wanted to review alternate considerations

[ilnytskyi]

In my opinion the quote model works ok.

Magento\Quote\Model\Quote->collectTotals()

If you need to recollect totals you can reset that via flag.
Instead of using method that triggers additional checks

Magento\Checkout\Model\Session::getQuote()

You can just extract quote id from

Magento\Checkout\Model\Session::getQuoteId()

or other objects that available inside your plugin

Merchant\Module\Plugin\NotRelevantClassName->afterGetCartDetails()

and load from chace in QuoteRepository. Then you have same object intact.

Additionally Magento\Checkout\Model\Session caches quote in its own property so there is a risk that if some code performs saves or does not handle clearing of Magento\Checkout\Model\Session state you might end up with unexpected behaviour like this.

[fredden]

@ilnytskyi & @convenient, would you suggest changing this to throw an error instead of mask the duplicate call? If so, I'll investigate this, and also look to change how the session gets the quote object, to avoid this scenario going forward.

[engcom-Lima]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Hotel]

@magento run all tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Hotel]

@magento run Functional Tests CE, Functional Tests EE, Functional Tests B2B, Unit Tests, WebAPI Tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Hotel]

Hello @fredden,

Failed tests seem flaky to me, and changes seem fine. Hence approving the PR.

Thanks

[engcom-Lima]

@magento create issue

[engcom-Echo]

@magento run Functional Tests EE,Functional Tests B2B,Functional Tests CE,Unit Tests,WebAPI Tests

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Echo]

@magento run Functional Tests EE,Functional Tests B2B,Functional Tests CE

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Hotel]

Hello @fredden,

We are unable to reproduce the issue mentioned here. Please go through this comment #37856 (comment) and let us know in case we have missed anything.

Meanwhile, we are putting this issue On Hold.

Thanks

[engcom-Hotel]

Hello @fredden,

We are closing this PR for now as the original issue is not reproducible for us. Please go through with the comment here #37856 (comment).

Feel free to reopen it and let us know if we have missed anything in order to reproduce the issue.

Thanks