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

Convert Exception to Throwable in Mage_Core_Block_Template. #2623

Merged
merged 1 commit into from
Sep 28, 2022

Conversation

kiatng
Copy link
Contributor

@kiatng kiatng commented Sep 28, 2022

Description (*)

This will save some hours for developers, which need to see exceptions. Without this change, a whole class of errors are not caught. See discussion #2606.

Related Pull Requests

PR #1442 is not going to be merged anytime soon, so why not fix this file first.

@github-actions github-actions bot added the Component: Core Relates to Mage_Core label Sep 28, 2022
@fballiano fballiano merged commit c5c34fc into OpenMage:1.9.4.x Sep 28, 2022
@github-actions
Copy link
Contributor

Unit Test Results

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

Results for commit c5c34fc. ± Comparison against base commit 2946fcb.

@luigifab
Copy link
Contributor

luigifab commented Dec 7, 2022

I'm not sure, but this create a new problem.

With a fresh installation of OpenMage without any changes, developer mode disabled, log enabled, PHP 8.0.
Create an error in app/design/frontend/rwd/default/template/persistent/customer/form/login.phtml, for example add: Mage::get();

With Throwable line 262 in Mage_Core_Block_Template:

  • exception.log: Error: Call to undefined method Mage::get() in app/design/frontend/rwd/default/template/persistent/customer/form/login.phtml:22
  • but also system.log: HEADERS ALREADY SENT: \<pre>[0] app/code/core/Mage/Core/Controller/Response/Http.php:45
  • the webpage (index.php/customer/account/login/) is working without some blocks

With Exception line 262 in Mage_Core_Block_Template:

  • exception.log: not created
  • system.log: not created
  • the webpage (index.php/customer/account/login/) isn't working (like an exit)

Solution seems to add (not sure):

        } catch (Throwable $e) {
            if (!$do) { // here
                ob_get_clean();
                $do = true; // here
            }
            if (Mage::getIsDeveloperMode()) {
                throw $e;
            }
            Mage::logException($e);
        }

@luigifab
Copy link
Contributor

luigifab commented Dec 7, 2022

There is another problem, YES NOT SAME.

Create an error in app/code/core/Mage/Customer/controllers/AccountController.php for loginAction, for example add: Mage::get();. Go to frontend and go to the login page, result: blank page (error 500), no logs.

An incomplete solution is to change in Mage.php for run(:

diff --git a/app/Mage.php b/app/Mage.php
@@ -741,7 +738,7 @@ final class Mage
         } catch (Mage_Core_Model_Store_Exception $e) {
             require_once(self::getBaseDir() . DS . 'errors' . DS . '404.php');
             die();
-        } catch (Exception $e) {
+        } catch (Throwable $e) {
             if (self::isInstalled()) {
                 self::printException($e);
                 exit();

Then, instead of a blank page, the error report page is displayed, and a report is created.

@luigifab
Copy link
Contributor

luigifab commented Dec 7, 2022

But if you read the error report, it is "incomplete".
The source file and line number are not displayed.

This fix the problem (but not fully tested):

diff --git a/app/Mage.php b/app/Mage.php
@@ -968,11 +965,13 @@ final class Mage

             print $e->getMessage() . "\n\n";
             print $e->getTraceAsString();
+            print "\n" . '  thrown in <b>' . $e->getFile() . '</b> on line <b>' . $e->getLine() . '</b>' . "\n" . '  catched by Mage::printException()';
             print '</pre>';
         } else {
             $reportData = [
                 (!empty($extra) ? $extra . "\n\n" : '') . $e->getMessage(),
-                $e->getTraceAsString()
+                $e->getTraceAsString() .
+                "\n" . '  thrown in ' . $e->getFile() . ' on line ' . $e->getLine() . "\n" . '  catched by Mage::printException()'
             ];

             // retrieve server data

@addison74
Copy link
Contributor

If it is an issue we should investigate it. I will allocate time for it.

@addison74 addison74 self-assigned this Dec 9, 2022
@sreichel
Copy link
Contributor

@luigifab can you please open new issues for this?

@addison74 addison74 removed their assignment Jan 19, 2023
@kiatng kiatng deleted the throwable_in_core_block_template branch January 20, 2023 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Core Relates to Mage_Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants