Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

String based error handling in Mage_Sales_Model_Quote:addProductAdvanced etc. #55

Closed
amenk opened this issue Jul 30, 2012 · 4 comments
Closed

Comments

@amenk
Copy link
Contributor

amenk commented Jul 30, 2012

The function addProductAdvanced and the underlying functions return strings if an error occurred.

One example from Mage_Catalog_Model_Product_Type_Abstract::_prepareProduct:

    try {
        $options = $this->_prepareOptions($buyRequest, $product, $processMode);
    } catch (Mage_Core_Exception $e) {
        return $e->getMessage();
    }

They should use exceptions instead - so a better handling of the different cases for an error can take place such as

  • distinguishing between the different kinds of errors
  • logging backtraces

As a result, parts like

    if (is_string($cartCandidates)) {
        return $cartCandidates;
    }

can be omitted then because the exceptions are passed automatically.

@magento-team
Copy link
Contributor

@amenk
Thank you for the suggested code improvement.

From a developer point of view, exception usage does have advantages over returning an error text. However, implementation seems to be a pretty complex task, which requires modification of all places where Mage_Sales_Model_Quote::addProductAdvanced() or Mage_Sales_Model_Quote::addProduct() is used and covering them with appropriate tests.

At the same time, proposed code changes do not bring any value from the business stand point: it's not a new feature (even for a developer), not a bug fix, not a performance improvement.

In order to get a progress on this ticket, it should be supplied with the pull request, which implements proposed changes along with unit/integration tests.

@magento-team
Copy link
Contributor

@amenk
We are closing this ticket because of no feedback for more than 2 weeks. If you want to contribute, feel free to submit a new pull request.

@amenk
Copy link
Contributor Author

amenk commented Oct 4, 2012

I am a bit terrified that you close issues when there was no activity. This is still an issue and should be fixed.

Please reopen.

@magento-team
Copy link
Contributor

Hello Amenk, we have really huge backlog of issues that require deep system changes and that we know bring value to developers and business. As someone noted above, this one, while needed, is low priority and requiring many changes. If someone can contribute it - great - but there is very small chance our team will do that. We need to close tickets in github as it is easier to manage them.

@stevieyu stevieyu mentioned this issue Apr 3, 2015
okorshenko pushed a commit that referenced this issue Oct 30, 2015
magento-team pushed a commit that referenced this issue Mar 23, 2016
JS-330: Hide magnifier on video images
magento-engcom-team pushed a commit that referenced this issue Apr 29, 2020
#55 : Need to update/change titles for ACL resource tree related to Login as Customer
skovalenk added a commit to skovalenk/magento2 that referenced this issue Jun 10, 2020
skovalenk added a commit to skovalenk/magento2 that referenced this issue Jun 12, 2020
skovalenk added a commit to skovalenk/magento2 that referenced this issue Jun 16, 2020
sidolov pushed a commit that referenced this issue Aug 17, 2020
@FabXav FabXav mentioned this issue Oct 11, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants