Skip to content

Conversation

@indykoning
Copy link

@indykoning indykoning commented Oct 27, 2025

Description (*)

This PR replaces the deleteById calls on the CartItemRepository with removeItem on the cart instance.
This makes sure expected observers are triggered and the item is actually updated on the cart object as well, instead of in the database only.

This fixes a case where if you mass remove items using updateCartItems only a single item is removed in the response.

Manual testing scenarios (*)

  1. Add multiple products to the cart
  2. remove the products with the following query
mutation updateCart($input: UpdateCartItemsInput) {
  updateCartItems(input: $input) {
    cart {
      items {
        uid
      }
    }
  }
}

{
  "input": {
    "cart_id": "xxxx",
    "cart_items": [
      {
        "cart_item_id": "xx",
        "quantity": 0
      },
      {
        "cart_item_id": "xx",
        "quantity": 0
      },
      {
        "cart_item_id": "xx",
        "quantity": 0
      },
      {
        "cart_item_id": "xx",
        "quantity": 0
      },
      {
        "cart_item_id": "xx",
        "quantity": 0
      }
    ]
  }
}
  1. Observe how items still appear to be in the cart until you manually request the cart again.
  2. Repeat these steps after applying this PR
  3. Observe how all selected items have immediately disappeared from the response.

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] Call RemoveItem on cart instead of ItemRepository #40255: Call RemoveItem on cart instead of ItemRepository

@m2-assistant
Copy link

m2-assistant bot commented Oct 27, 2025

Hi @indykoning. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.
❗ 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 Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

@engcom-Hotel
Copy link
Contributor

@magento create issue

@engcom-Hotel engcom-Hotel added the Priority: P2 A defect with this priority could have functionality issues which are not to expectations. label Oct 28, 2025
@github-project-automation github-project-automation bot moved this to Pending Review in Pull Requests Dashboard Oct 28, 2025
@engcom-Hotel
Copy link
Contributor

@magento run all tests

Copy link
Contributor

@engcom-Hotel engcom-Hotel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @indykoning,

Thank you for the collaboration!

The PR changes seems like a backward incompatible change for the below reasons:

  • Plugins may be registered on CartItemRepositoryInterface::deleteById()
  • Calling $cart->removeItem() bypasses all plugins on the repository method
  • This can break third-party extensions that rely on intercepting cart item deletions

@engcom-Hotel engcom-Hotel moved this from Pending Review to Review in Progress in Community Dashboard Dec 2, 2025
@engcom-Hotel engcom-Hotel moved this from Review in Progress to Changes Requested in Community Dashboard Dec 2, 2025
@indykoning
Copy link
Author

Correct, how should we deal with these breaking changes?
Currently these graphql files are the only ones directly removing cart items from the database using the repository, skipping all events fired by, and plugins listening on the removeItem (

$this->_eventManager->dispatch('sales_quote_remove_item', ['quote_item' => $item]);
) which all extensions must do to be compatible with regular Magento 2 installs.

Since the updateItems being called by Magento's controllers & apis execute this updateItems

$this->removeItem($itemId);

Copy link
Contributor

@engcom-Hotel engcom-Hotel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @indykoning,

Thank you for your contribution!

Please add some automated test in accordance to the DOD.


if ($quantity <= 0.0) {
$this->cartItemRepository->deleteById((int)$cart->getId(), $itemId);
$cart->removeItem($itemId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After further analysis, your point is valid. The current GraphQL implementation using CartItemRepositoryInterface::deleteById() is inconsistent with the rest of Magento (REST API, Controllers) which all use Quote::removeItem().

But please add a comment explaining the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: needs update Project: Community Picked PRs upvoted by the community

Projects

Status: Changes Requested

Development

Successfully merging this pull request may close these issues.

[Issue] Call RemoveItem on cart instead of ItemRepository

3 participants