Replaced all catch Exception with Throwable #1442
Conversation
github-actions
bot
added
Component: AdminNotification
|
It's a recommended practise. Some more details can be read here: https://webdevetc.com/blog/why-you-should-use-spl-exceptions-in-php-for-better-exception-handling/#section_runtimeexception |
|
Thanks for the proposal, @woutersamaey! Using RuntimeException seems like a mostly subjective change, what will it improve in the case of OpenMage? This probably belongs in an RFC.. Please see the RFC template, I think this is the kind of discussion this change needs before it is accepted. I'm not so sure that a practice being recommended is a sufficient reason to make such sweeping changes. "They" also made PHP backwards compatible with the previous recommended practices so that existing projects would not be forced to adopt the new ones. :) |
|
Some news? |
|
Before getting this merged we should discussed the PR #1434 too. At least we have the same way of handling exceptions adapted to the new variants of PHP. |
I agree with @colinmollenhour on this one. To have these many differences between v19 and v20 will make it really hard in the future. But since v19 will have to be bumped to something higher than php7.0 sometimes soon, would't it be ok to have this PR on v19 too? |
|
Let's rebase the PR to 19. Sury dropped this month the support for Debian 9 and you cannot install other PHP versions anymore. Debian 10 comes with 7.3 and Debian 11 with 7.4. We should not care about versions bellow 7.3. |
|
I also vote for v19 😸. |
|
I vote for v19. I just spent many minutes debugging an error because OM does not throw anything due to this: magento-lts/app/code/core/Mage/Core/Block/Template.php Lines 263 to 266 in 5de30bf The error is thrown once I changed it to The offending code in my block: /**
* @return bool
*/
public function getCanWestEastTransfer()
{
return (bool) $this->getParentBlock()
->getCustomer()
->getCanWestEastTransfer();
}The page loaded without any error, but it is broken. After changing to
|
|
My take currently:
|
|
I confirm what @kiatng said. I recently faced the implode function in an old PHTML file and the error did not appear anywhere. When I changed Exception to Throwable the error appeared in the browser window. Although I solved the issue without modification, I lost time to find it. |
|
I have an issue with this PR. With the PR, or simply by search and replace all diff --git a/app/Mage.php b/app/Mage.php
index f1dd766a25..beb83314c4 100644
--- a/app/Mage.php
+++ b/app/Mage.php
@@ -734,21 +734,21 @@ final class Mage
'scope_type' => $type,
'options' => $options,
]);
Varien_Profiler::stop('mage');
} catch (Mage_Core_Model_Session_Exception $e) {
header('Location: ' . self::getBaseUrl());
die();
} catch (Mage_Core_Model_Store_Exception $e) {
require_once(self::getBaseDir() . DS . 'errors' . DS . '404.php');
die();
- } catch (Exception $e) {
+ } catch (Throwable $e) { // here
if (self::isInstalled()) {
self::printException($e);
exit();
}
try {
self::dispatchEvent('mage_run_exception', ['exception' => $e]);
if (!headers_sent() && self::isInstalled()) {
header('Location:' . self::getUrl('install'));
} else {
self::printException($e);When you have an error in a template file, for example: --- a/app/design/frontend/rwd/default/template/page/html/footer.phtml
+++ b/app/design/frontend/rwd/default/template/page/html/footer.phtml
@@ -17,6 +17,8 @@
* @copyright Copyright (c) 2006-2020 Magento, Inc. (https://www.magento.com)
* @license https://opensource.org/licenses/afl-3.0.php Academic Free License (AFL 3.0)
*/
+
+Mage:getIsDefault();
?>
<div class="footer-container">
<div class="footer">
When developer mode is enabled, the stack trace is wrong/incomplete: With the latest OpenMage without any changes, the stack trace is good: Is someone know why? Another side effect when we use error_reporting(E_ALL);
ini_set('display_errors', 1);
set_exception_handler('handleException');
function handleException($e) {
echo 'die:handleException: ',$e->getMessage(),"\n";
}
echo 'Test Throwable',"\n";
try {
Mage::app();
}
catch (Throwable $e) {
echo 'Exception: ',$e->getMessage(),"\n";
}
echo 'Test Exception',"\n";
try {
Mage::app();
}
catch (Exception $e) {
echo 'Exception: ',$e->getMessage(),"\n";
}
echo 'end',"\n";Result for PHP 8.2, 8.1, 8.0, 7.4, 7.3, 7.2, 7.1, 7.0 is: The |
fballiano
dismissed stale reviews from kiatng, luigifab, and kkrieger85
The base branch was changed.
|
I rebased on 1.9.4.x |
|
It seems that the author has closed this PR. I mark it with the label and if it is useful it can be proposed by someone else. |
|
New PR or not? |
I think a new PR is needed as a blanket replacement of all Exception with Throwable is probably not optimal and may not be an improvement. I think a new PR would need to spell out exactly what problems it is solving and why the specific approach was taken. That is, coder errors (e.g. type errors, calls to undefined functions) should not necessarily be handled the same as all other errors. Since there is already a "default error handler" registered, what needs to be fixed exactly? |
In PHP 5 the most generic thing to catch was Exception and Magento does this a lot. However with PHP7 and things like strict types, the most generic error to catch is now Throwable.
So, in this PR I've replaced 99% of the catch Exception with catch Throwable.
The only cases I didn't change are:
I hope this PR is appreciated, since it would greatly help the adoption of PHP7 and strict types, like I'm doing every day.