sydekumf commented 7 years ago

Preconditions

Magento 2.1.7

When you enter the checkout Magento tries to estimate the shipping method and costs. Therefore it loads available quotes with this logic: Magento\Quote\Model\QuoteRepository::getForCustomer(). For some reason it tries to load all quotes available for this customer, even quotes which are no longer active or whose products do not longer exist. This leads to an exception in Magento\Quote\Model\ResourceModel\Quote\Item\Collection::_assignProducts() where it tries to assign the not existing products. The exception is hidden because of this in Magento\Quote\Model\Webapi\ParamOverriderCartId:

} catch (NoSuchEntityException $e) {
            /* do nothing and just return null */
        }

In the frontend it leads to another error which has nothing to do with this error: https://github.com/magento/magento2/issues/1443

I am not sure what is the correct behavior:

Should old quotes get deleted?
Is it a wrong behavior that it tries to load all quotes?
Should the exception handling be much better?

Steps to reproduce

In a customer's account have several quotes and old quotes and delete some of the quote's products
Enter checkout with a new quote

Expected result

Checkout is possible
The error handling is more transparent and exceptions do not get hidden

Actual result

Misleading error message from https://github.com/magento/magento2/issues/1443 is shown
Checkout is not possible

korostii commented 7 years ago

Experienced this error myself, with similar findings. Pretty sure I wrote about it somewhere, but cannot find it now. the catch (NoSuchEntityException $e) seems to have been added in order to cover the case when the customer isn't logged in.

Should the exception handling be much better?

Yes, please. I'll take that, plus, independently, a fix for the underlying error.

Underlying error can probably be fixed by simply adding a foreign key constraint in between cart item and product entity tables, preferably with an "ON DELETE CASCADE" =)

collymore commented 7 years ago

Hi, we're having the same issue and can possible shed some light. The error occurs because although there are quotes for the user they all have is_active = 0.

I'm not sure why they have so many quotes though and would have expected them to be removed.

The query that is run when trying to get the quote from within Magento\Quote\Model\Webapi\ParamOverriderCartId is

SELECTquote.* FROMquoteWHERE (quote.customer_id=38) AND (store_id IN ('1')) AND (is_active = 1) ORDER BYupdated_atDESC LIMIT 1

So it's always trying to get the latest quote but as non are active, then it doesn't' return any for this user.

The error always happens after making a purchase and then added products to the cart in the same session. If you logout and back in a new active quote is created.

MikeSheward commented 7 years ago

Has anyone found a workaround for this?

zakgrindle commented 7 years ago

I am also experiencing this issue on Magento 2.1.7. First checkout after login is fine, second checkout attempt results in the 'cartId is a required field' error.

Any assistance would be greatly appreciated.

burgh8wp commented 7 years ago

This is still present in Magento 2.1.7 occurs on 2nd checkout for logged in user only for us, using steps described above.

magento-engcom-team commented 7 years ago

@sydekumf, thank you for your report. We were not able to reproduce this issue by following the steps you provided. If you'd like to update it, please reopen the issue. We tested the issue on 2.1.9

sydekumf commented 7 years ago

Hi, I do not have any other steps to reproduce. Maybe ask the other guys who can reproduce it, if they can add any further information? I also checked the code of 2.1.9: You still have the following in it:

} catch (NoSuchEntityException $e) {
            /* do nothing and just return null */
        }

I think there should be a better approach to this, not just hiding exceptions...

pvalium commented 7 years ago

Same problem in 2.1.8 EE. Occurs on 2nd checkout for logged in user, using steps described above.

wget commented 7 years ago

@magento-engcom-team If you need credentials to a Magento 2 installation experiencing this issue, let me know.

magento-engcom-team commented 7 years ago

Per multiple requests – reopening this for the further investigation. Even though for now we can not reproduce the issue in question, we will attempt different approaches.

magento-engcom-team commented 7 years ago

@korostii

Underlying error can probably be fixed by simply adding a foreign key constraint in between cart item and product entity tables, preferably with an "ON DELETE CASCADE" =)

Foreign key is implemented programmatically in this specific case, please refer to https://github.com/magento/magento2/blob/78e72eb8c7f3d5e77b1f8cfd2a7263813ad65019/app/code/Magento/Quote/Model/Product/QuoteItemsCleaner.php#L26-L32

wget commented 7 years ago

The correct link is: https://github.com/magento/magento2/blob/78e72eb8c7f3d5e77b1f8cfd2a7263813ad65019/app/code/Magento/Quote/Model/Product/QuoteItemsCleaner.php#L26-L32

magento-engcom-team commented 7 years ago

@wget Thank you for pointing out. Comment updated.

magento-engcom-team commented 7 years ago

@pvalium @wget @sydekumf @collymore @zakgrindle @primalspace From the investigation we can see that this behavior may be possible in Enterprise Edition, only when quoteItemCleaner queue consumer is not started. Please provide complete information on affected versions. Still, we can not reliably reproduce the issue at the moment. Precise step-by-step guide would be helpful to speed up the process.

Thank you for collaboration.

JeroenVanLeusden commented 7 years ago

Also encountered this issue. Seems that Magento\Quote\Model\ResourceModel\Quote::loadByCustomerId() tries to fetch the customers quote but there isn't any active, like @collymore mentioned.

wget commented 7 years ago

I can reproduce this issue with the following steps on 2.1.8 CE. This is rather easy.

Put an article in your cart.
Go to the checkout page located at https:///checkout/
Open that same page in another tab.
On one of the tab, click to order the product. Go to the other tab and select another shipping address for example. You got the cartId message.

This is pretty normal I get this error in this particular (silly) use case (even if a more straightforward error message should be displayed), I'm not discussing on this one. But I have to admit, when not being silly, I got this error message by doing other steps and by switching from one payment method to the other etc. The investigation goes on on my side as now I'm not able to reproduce (got the error one time in the afternoon though). I will update you when I can find the exact steps to reproduce it completely.

magento-engcom-team commented 7 years ago

@wget Thank you for the update. Please pay attention to the fact, that if you are using Magento Commerce (Enterprise Edition), the programmatical foreign key, mentioned above, is implemented in a different way, using queue. Having a consumer running is crucial in that case. On a side note: the exception masking, mentioned in the thread before, is indeed a bad practice, however, it would not influence/fix particular problem described in the ticket.

wget commented 7 years ago

@magento-engcom-team Thanks for the precision, but like I wrote, I'm actually using the Community Edition (2.1.8 CE).

klein0r commented 7 years ago

We are getting the same issue with Magento CE 2.1.9.

The reason we get this issue is, that the checkout loads another (older) quote for the customer which is still active. Since the quotes are ordered by updated_at DESC it loads the older one, since the new one has "00-00-0000 00:00:00" as updated_at.

The older quote contains a product which is no longer present, so the quote can't be loaded, the %cart_id% can't be replaced (due to NoSuchEntityException) and we are getting the error cartId is required.

So our customer has multiple active quotes. Why can this happen? When are the quotes set to is_active = 0?

pvalium commented 7 years ago

I've solved following this steps: -Adding sql relation to cart product item, to avoid 'missing' products if you manipulate database by hand (deleting products, using magmi, etc) -Creating a event in sales_quote_save_before, and checking that update_at field of quote is not null. If is null, i set current_date to update_at.

Now i can make several orders without logout, and all cart strange behaviour has dissappeared.

sydekumf commented 6 years ago

Hi @magento-engcom-team,

I think I understand the problem now with the QuoteItemsCleaner. But what is the reason, that you did not introduce a database foreign key on delete cascade constraint? What would be the consequence if we introduce this constraint on our own?

Thanks :-)

korostii commented 6 years ago

Hi @magento-engcom-team

From the investigation we can see that this behavior may be possible in Enterprise Edition

I've seen it happen on CE as well.

Foreign key is implemented programmatically in this specific case, please refer to

Well, obviously, whatever that thing is - it's not working. Is there a particular reason why it was implemented that way? Such an approach seems quite fragile, at best. (i.e. why couldn't you just add this constraint, as several people above suggest?)

On a side note: the exception masking, mentioned in the thread before, is indeed a bad practice, however, it would not influence/fix particular problem described in the ticket.

Why would it "influence/fix particular problem"? No-one said that, that would be nonsense.

The point is that exception masking hides the original exception, ignores invalid application state, making it much harder to trace and debug the code and identify the real root cause behind the second exception which appears elsewhere and seems quite cryptic.

In this particular case it's even worse: the exception throw+catch are used as a control structure in order to pass information (exception is explicitly said to be an "okay" behavior in comments) which is none better then using goto in your code AND it's also bad for performance.

Obviously, exception handling is a separate issue and should be handled independently as there are many other offending pieces of code just like the one referenced here (although this one is the most annoying), but it shouldn't be forgotten and ignored neither, wouldn't you agree?

sydekumf commented 6 years ago

So there is one thing we did to fix this problem for ever:

ALTER TABLE quote
                CHANGE updated_at 
                updated_at TIMESTAMP NOT NULL
                    DEFAULT CURRENT_TIMESTAMP 
                    ON UPDATE CURRENT_TIMESTAMP;

For a fresh insert the updated_at was 0000000... which of course lead to some problems when you order the quotes DESC. No the updated_at gets the timestamp with a fresh insert and then it gets loaded at first quote by DESC and the error will never appear again :-)

magento-engcom-team commented 6 years ago

@sydekumf @korostii

Is there a particular reason why it was implemented that way?

why couldn't you just add this constraint, as several people above suggest?

It's implemented this way as Magento Commerce Edition uses different approach to clean up.

What would be the consequence if we introduce this constraint on our own?

It is possible that adding a constraint on your own will now have any negative consequences. However, there is no guarantee, only testing can confirm.

korostii commented 6 years ago

Hello again @magento-engcom-team

As far as I can see, you've only addressed one of the issues brought up in this issue report with your comments. Suppose you're saying (please do correct me if I misunderstood) that you won't be willing to change the old cart cleanup mechanism to guarantee the deletion of invalid quote items as a constraint. Even so, you could still look into the other parts of the issue in order to mitigate the Magento's behavior upon meeting such (however unlikely) corner cases:

Is it a wrong behavior that it tries to load all quotes? Should the exception handling be much better?

Handling any of those should IMHO significantly reduce the overall level of pain brought by this issue.

birmo commented 6 years ago

WroCD

I am going to take this issue. Could you assign me?

ghost commented 5 years ago

Similar or identical github issues for this same error:

https://github.com/magento/magento2/issues/9744

https://github.com/magento/magento2/issues/7299

https://github.com/magento/magento2/issues/1443

https://github.com/magento/magento2/issues/5847

https://github.com/shipperhq/module-shipper/issues/27

https://github.com/magento/magento2/issues/6522

All related, tried every solution on all pages and still have the issue, including @sydekumf solution here: https://github.com/magento/magento2/issues/9744#issuecomment-347831102 this solution https://github.com/magento/magento2/issues/7299#issuecomment-305437174 this solution https://github.com/magento/magento2/issues/7299#issuecomment-327420755 and checked if this solution would apply but it didn't: https://github.com/magento/magento2/issues/5847#issuecomment-402379223

Issue MAGETWO-84524 is tracking it to some degree and still isn't fixed in Magento 2.3

How is this the most popular ecommerce store in the US if it doesn't even have a functioning checkout. This happens for me using authnet directpost regardless of what I do, guest checkout, or new, using stored address, or not. Only the error message will change at times, but it's always one of the error messages given in the links above.

Issue is as described, there is a quote_id in the quote table but it is not active. If I switch it manually to be active then it has duplicate foreign key when trying to replace the order. With active set to 0, get the cartId error.

The only solution anyone seems to say works 100% is buying a 3rd part module to do authorize.net payments which seems absurd, as paypal also doesn't work, citing invoice id errors.

m2-assistant[bot] commented 4 years ago

Hi @engcom-Delta. Thank you for working on this issue. Looks like this issue is already verified and confirmed. But if you 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.4-develop branch
Details
- Add the comment @magento give me 2.4-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.4-develop branch, please, add the label Reproduced on 2.4.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. If the issue is not relevant or is not reproducible any more, feel free to close it.

Join Magento Community Engineering Slack and ask your questions in #github channel.

engcom-Delta commented 4 years ago

Hi @SoloDevAtrix @sydekumf, thank you for your reports. I am not able to reproduce issue by steps you described on clean 2.4-develop In Magento\Quote\Model\QuoteRepository::getForCustomer() I always get last active quote, but not all quotes for customer. #9744issueeDebug

And exception under catch (NoSuchEntityException $e) in Magento\Quote\Model\Webapi\ParamOverriderCartId now has appropriate message: #9744issue

Please feel free to comment, reopen or create new ticket according to the Issue reporting guidelines if you are still facing this issue on the latest 2.4-develop branch. Thank you for collaboration.

magento / magento2

Shipping method estimation loads all customer's quotes #9744

Preconditions

Steps to reproduce

Expected result

Actual result

WroCD