Improvements to the CartProvider

Only thing missing was a documentation update for CartProvider::cartData. Only bummer about the patch is the inconsistency in ordering of the store vs. account parameters, but I understand our hands are tied on CartProvider::createCart() to preserve BC. 😬

Patch attached updates the property docs at the top, should still commit fine - no other code was changed. fwiw, in my single store site, this obviously works as expected. I haven't tried it in a multi-store setup but don't see why it shouldn't work.

I think anyone with multistore is a small portion of users. And those who have used multiple stores use them across domains. So this change improves their experience - developer and QA creating carts between domains locally.

We can pop a change record and anyone with custom code and understand why

I think anyone with multistore is a small portion of users. And those who have used multiple stores use them across domains.

Some assumptions here. It's ok that you make such decisions and changes, however, could you at least be highlighting them in the release notes when you introduce backward-breaking changes? Make the life of developers easier when updating.

I'd agree with @krystalcode.

This also breaks modules like commerce_api and commerce_marketplace and there is now no way of requesting all carts for a single user.

I understand the idea to 'improve' the cart provider, but this changes its functionality for no real reason, significantly reducing its flexibility.

We should have at least added additional methods such as getAllCarts and getAllCartIds and move the default fallback store outside of loadCartData to keep some flexibility.

If anybody relies on the previous behavior, I will be fixing the regression on Commerce Cart Advanced where I'll be keeping the previous behavior i.e. return all carts unless a specific store is explicitly requested.

@jsacksick

I think the best is that caller of the cart provider should be deciding which carts to fetch rather than the provider deciding internally. That is the cart view, for example, should have a way to say give me only the carts for this store or all carts. I can create an issue with a proposal later when I can.

@krystalcode let me know if you need some additional information for the issue. happy to help.

Let's reopen this issue.

Additionally, the query performed by loadCartData() should always have a "store_id" condition, and if no store is provided, we should default to the current store to match what's currently done in getCartId()/getCart().

This is a major BC break and needs to be reverted. It basically removes the ability to list carts belonging to multiple stores.

Ok, let's keep it here.

I've thought a bit about this and I don't think we actually need a new solution, we just need the previous behavior.

- If a site has only one store, then there's no problem, all carts will be loaded and they are all from the same store.
- If a site is a multi-domain site, it could be handy to default to the current store as that would be determined by the current domain, but then there must be custom code or through contrib to associate each store with a domain and I feel that whoever is providing that should be responsible for customizing the cart controller to their needs. There could even be multi-domain sites with multiple stores per domain, let them deal with that as they need.
- If a site is a marketplace i.e. multiple stores, then it's actually desired to have carts from multiple stores displayed.

I think we can keep the ability to provide a specific store, even though that requires other modules that extend the cart provider to be updated - that's ok. And just load all carts if no specific store was requested.

@jsacksick I just tested the patch but I don't have the behavior before the last update at all. I have a marketplace and if I add products from 2 different stores, it will create a single shopping cart.

Before it work properly and it create 2 shopping carts.

I have the Drupal Commerce and Commerce Markerplace modules.

I hope that Drupal Commerce can finally manage marketplaces properly.

Created #3193580-3: Add a cart settings form to filter the cart page to the current store as a follow-up issue to reenact the behavior we were trying to prioritize, which is the more clear use case of a multi-store site where 1 store = 1 domain and carts should not be visible off-domain.

Patch #26 fixes the issue for me and restores the ability to load all users carts at once.

I believe it still adds the functionality that this issue was trying to solve, but i'm not in a position to confirm this.

@ocastle Is the patch working for you ? I tested it and it does not allow you to create multiple shopping carts. If I add products from multiple stores, I only have one shopping cart. It should display a shopping cart by store. Do you use Commerce Marketplace ?

@zenimagine The patch restored the original setup on cart provider which mainly affected my commerce_api implementation.

I do use commerce marketplace but not the UI and I know the result you're expecting, but agree with @jsacksick, not sure this patch would have changed that. Might be worth opening issue on commerce_marketplace?

Yes, it works,
I have separate cart for each store as it was previously.
Thank You.