MC-26683: Removed get errors of cart allowing to add product to cart

[AleksLi]

Description (*)

I removed completely the part of throwing any kind of error in the cart in case one of the product is out of stock.
I tried to change/add out of stock product that is in the cart and I've got an error with my current implementation (removed the getErrors -> throw $e).

Please help me figure out what might go wrong in this case.

Related Pull Requests

Fixed Issues (if relevant)

1. Unable to execute addSimpleProduct mutation while other items out of stock #26683: Unable to execute addSimpleProduct mutation while other items out of stock

Manual testing scenarios (*)

1. Fully described in Unable to execute addSimpleProduct mutation while other items out of stock #26683

Questions or comments

Why did we need that part of code to throw any kind of error in the cart for GraphQL cart management?

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)
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @AleksLi. Thank you for your contribution
Here is 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

For more details, please, review the Magento Contributor Guide documentation.

[lenaorobei]

Hi @AleksLi,
Thank you for the contribution, however I don't think that removing error messages is the right way to fix this issue. This fix leads to the wrong flow when user will see cart errors only on place order step. My understanding here is that errors should be returned under error node like it is right now, but cart data should be present under data node. Could you please investigate the possibility to adjust this PR to display both - errors and data?
Thank you.

[AleksLi]

@lenaorobei Let me investigate that and come back with the commit or descriptive solution.

[AleksLi]

@lenaorobei I investigated the possible solutions for that.
Throwing an $e - will lead to building response with null data and errors.

1. So I would say we may find a way to add data in that case (in case of throwing error).
2. We could update the cart model or add one more key in response array like this:
   return [ 'cart' => [ 'model' => $cart, ], 'errors' => [ 'model' => $cart->getErrors() ] ];
   getErrors() method will return array of errors.

What do you think we should do?

P.S. I would prefer the first option I guess. (my opinion). But I have no idea right now how to do that)) I will investigate it more in case we will choose 1st one.

[lenaorobei]

@AleksLi, I agree, option 1 looks better and complies with GraphQL standard. Appreciate your involvement!

[AleksLi]

@lenaorobei Ok then. I will investigate and maybe make a commit today. Sorry, I don't have much time in the evening to make a big input into the process. It would be slow but in the right way))

[AleksLi]

@lenaorobei What I have found is:
vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php:250 says that

// Errors from sub-fields of a NonNull type may propagate to the top level,
// at which point we still log the error and null the parent field,
// in this case is the entire response.

Could you help me find out how to actually do that? At least a right way to dig. I'm debugging the whole process of logging error but cannot find anything useful to solve the issue, but I really want to.
I think somewhere here or in resolve method /vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php:255 $result should be with cart data.

My other thought create some sort of \Magento\Framework\GraphQl\Exception\GraphQlInputException but with cart data. I haven't tried that, but it might work if I will understand what should be the response and with a little help

[lenaorobei]

I would start debugging from \Magento\Framework\GraphQl\Query\QueryProcessor and try to understand why only errors are present if exception was thrown. It looks like \GraphQL\Executor\ExecutionResult::toArray can process both data and errors.

[AleksLi]

@lenaorobei Yes, thanks that's the place I'm digging now and around it.
The thing is \GraphQL\Executor\ReferenceExecutor::buildResponse is building response with almost empty data. Almost because data looks like an array with addSimpleProductToCart = null. So, I dug dipper to see why it's empty. I found out that \GraphQL\Executor\ReferenceExecutor::executeOperation returns null because it throws error and in that case $result will be null \GraphQL\Executor\ReferenceExecutor::doExecute here.

And I'm stuck on this.
Any help would be great.

[lenaorobei]

Looks like it will require framework change, see \Magento\GraphQl\Controller\GraphQl::dispatch. I found related webonyx/graphql-php#432 bug but I don't believe that global state is a good solution here. This task turns into much more complex one.

[AleksLi]

@lenaorobei totally agree with you. That require more time definitely and magento architecturer's help to resolve it right.

So what should we do? Should I unassign the issue, because I'm interested to resolve it or keep helping in future progress? Please tell me

[lenaorobei]

It would be nice if you can propose some proof of concept of how to solve this issue on the framework level.

[AleksLi]

@lenaorobei I've seen your commits. thanks.

[magento-engcom-team]

Hi @lenaorobei, thank you for the review.
ENGCOM-7105 has been created to process this Pull Request

[engcom-Bravo]

✔️ QA Passed

The second product, which is IN-STOCK, could be added to Cart while the first product in Cart is OUT-OF-STOCK

The cart query returns an error, displaying that "Some of the products are out of stock.", but also displays that the second, IN-STOCK product, was added:

{
  "errors": [
    {
      "message": "Some of the products are out of stock.",
      "extensions": {
        "category": "graphql-input"
      },
      "locations": [
        {
          "line": 4,
          "column": 5
        }
      ],
      "path": [
        "cart",
        "items",
        0
      ]
    }
  ],
  "data": {
    "cart": {
      "total_quantity": 2,
      "items": [
        null,
        {
          "product": {
            "id": 2047,
            "name": "Sprod",
            "sku": "Sprod",
            "stock_status": "OUT_OF_STOCK"
          }
        },
        {
          "product": {
            "id": 2048,
            "name": "Sprod2",
            "sku": "Sprod2",
            "stock_status": "IN_STOCK"
          }
        }
      ]
    }
  }
}

[engcom-Charlie]

Failed functional tests not related to the changes in this PR.

[m2-assistant]

Hi @AleksLi, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.