Better error handling for issues during bulk attribute update

[woutersamaey]

If something goes wrong during mass product attribute update, the admin gets a generic message and there is no error in the log, making it harder to debug this issue.

[jouriy]

I just would suggest to keep Exception handling for backward compatibility, i.e.

        catch (Throwable $e) {
            Mage::logException($e);
            $this->_getSession()->addException($e, $this->__('An error occurred while updating the product(s) attributes.'));
        } catch (Exception $e) {
            $this->_getSession()->addException($e, $this->__('An error occurred while updating the product(s) attributes.'));

[woutersamaey]

@jouriy your suggestion does not make much sense. Magento 1 was written at a time when Exception was the most generic thing to catch. Since PHP7 this has become Throwable, so I’m pretty sure this is what was originally meant.
Things like TypeError did not exist at the time, but can now be a regular issue if you work in strict mode. This is why (IMHO) all mentions of Exception should be replaced with Throwable.

[jouriy]

Thank you for the details. The reason why I suggested it was my impression that 1.9.4.x branch is supposed to be backward compatible with Magento 1.9.x (and drop-in replacement) which is still mostly on PHP 5.6, so removing Exception handling means no any error reporting for 5.x.

This is why (IMHO) all mentions of Exception should be replaced with Throwable.

Should not we create a new PR than to bring better error handing for all cases?

[woutersamaey]

I updated the code as per @colinmollenhour suggestion.

[woutersamaey]

@jouriy running on PHP5.6 is a security risk, so I'm trying my best to modernize up to PHP7 without going overboard.
I'm contributing small things, as they affect my work.

Replacing all Exceptions with Throwables is now suggested in #1442

[colinmollenhour]

Ahh I just remembered that addException already logs the exception so this will double-log it.

[woutersamaey]

@colinmollenhour I don’t follow. What are you trying to say?

Okay, I think you are referring to \Mage_Core_Model_Session_Abstract::addException(), except this isn't quite the same. This does log the exception, but using Mage::log() and a custom message. Not using Mage::logException().

For me this isn't acceptable because we use special exception tracking and for that to work everything needs to go through Mage::logException() which is what Magento normally does.

I've changed my code for a better solution that does not log twice anymore.

[woutersamaey]

I also created #1443 where I change the superfluous handing of the Exception in Mage_Core_Model_Session_Abstract::addException()

Magento seams to have forgotten about it's own Mage::logException(). Why they would ever format their own string and pass it to Mage::log() is beyond me.

[woutersamaey]

Should I also move this to branch 20.0 ?

All requested changes have been implemented by the author

[github-actions]

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  +5  5 ✔️ +3  2 💤 +2  0 ❌ ±0 

Results for commit 0775715. ± Comparison against base commit eaf4354.