15

Just wondering if anyone has better strategies than I've thought of for type checking to co-exist with Magento's custom error handler. Specifically, I'm wondering about a "Catchable Fatal Errors" as thrown in the case of a typehinted parameter mismatch. Here's an example from the Mage class:

/** * Write exception to log * * @param Exception $e */ public static function logException(Exception $e) { if (!self::getConfig()) { return; } $file = self::getStoreConfig('dev/log/exception_file'); self::log("\n" . $e->__toString(), Zend_Log::ERR, $file); }

Because of the error handler, anything can be passed to the method, including a Zend_Date (which will work fine, but look super confusing in your exception log), or a Mage_Core_Model_App, which actually will have a fatal error.

It's possible to re-implement type checking at the top of a method: $e instanceof Exception but such tactics defeat the purpose of a typehint.

Any hints suggestions?

Improve this question
0

3 Answers 3

Reset to default
5

Good question +1

Did some research and tests after a good point in the direction after my discussion with @mpw on my first answer. I partly misunderstood it the first time.

Will add some code to clarify so others understand the issue better.

A note before take off

I never had such problems until this came up. Developing in Magento with developer mode enabled I do not even think a second about this. So every time I fart, it will show up and will be corrected accordingly.

The problem with a explaining sample

Your saying fatal errors will be logged(if enabled) and code will continue as usual because no error is thrown by mageCoreErrorHandler or the program will exit.

First Magento's core error handler for uncatchable errors app/code/core/Mage/Core/functions.php

/** * Custom error handler * * @param integer $errno * @param string $errstr * @param string $errfile * @param integer $errline */ function mageCoreErrorHandler($errno, $errstr, $errfile, $errline){ /** * Some internal logic here for building the error message */ $errorMessage .= ": {$errstr} in {$errfile} on line {$errline}"; if (Mage::getIsDeveloperMode()) { throw new Exception($errorMessage); } else { Mage::log($errorMessage, Zend_Log::ERR); } }

As you can see, in developer mode it will tell something useful, it throws a error. When turned off it will log(if enabled) and continue.

The proof

My testfile.php

require 'app/Mage.php'; Mage::app('admin')->setUseSessionInUrl(false); // Test function which expect Customer_Model_Customer function test(Customer_Model_Customer $customer) { var_dump('Do not show me because ' . get_class($customer) . ' is not a customer.'); } // Enabled developer mode Mage::setIsDeveloperMode(true); // Put a var in here $noGood = Mage::app(); // Make some context var_dump('hello'); try { // Call test function with a not accepted var test($noGood); // Tell if we get here var_dump('And we are here!'); } catch (Exception $e) { var_dump('You should die, because I am doing something which I should not do'); }

The result

Developermode enabled. Correct result

string(5) "hello" string(66) "You should die, because I am doing something which I should not do"

Developermode disabled, Incorrect result

string(5) "hello" string(61) "Do not show me because Mage_Core_Model_App is not a customer." string(16) "And we are here!"

So it will eventually skip the error and continue on the next line of code. Maybe with even weirder outcomes. (as @mpw points out)

Conclusion

It could happen that someone is developing in a way that errors will go unnoticed and it will eventually give unexpected results.

Ofcourse when developing in a professional manner. Errors will be noticed and attention is payed. The way to prevent this in Magento is always enabling developermode in a developer/test environment.

IMHO it should never get to this point of discussion, where checking a variable a second time(at least that's how I would describe it) is the way to go. Code should be tested before releasing on production environments. It should not be needed.

Second thoughts

Maybe Magento should stop after a fatal error. Or generate a report and show it to the visitor. This way next lines of code will never be executed, and things will be noticed.

Improve this answer
3
  • > Of coarse when developing in a professional manner. Errors will be noticed and attention is payed. The way to prevent this in Magento is always enabling developermode in a developer/test environment. ¶ I agree with that. My goal is to have Magento respect language rules in production mode. Looks like it'll require a custom module perhaps. Thanks for your insight! Commented Mar 16, 2015 at 17:22
  • Maybe Magento should throw a exception in both cases. The user will be presented a Magento errorlog page and in var/exception will have a matching logfile, the same as regular exceptions. The great catch here is that code wont be executed without notice. You could copy the functions file to the app/code/local and always throw a exception Commented Mar 16, 2015 at 19:47
  • 1
    I've decided to mark this as the answer. Though I still think muffling errors like that is dangerous, it seems unlikely that there's a way to make sure Magento respects typehints without opening up other problems. The reminder to keep dev mode turned on is a good one for future readers, and that's the most important takeaway Commented Mar 18, 2015 at 16:23
2

Good question. I think this is a general problem with E_RECOVERABLE_ERROR in PHP.

What you have in your question is the exception handler, not the error handler. The error handler is causing the actual problem you discuss here with catchable fatal errors (E_RECOVERABLE_ERROR).

PHP 7 and HHVM have this already solved.

It's worse with Magento because the error handler does not deal with this since PHP 5.2 error class.

A more useful kind of error handling would be dealing with this error class and to turn these errors into ErrorExceptions. Example (not by me, from here):

set_error_handler(function($errno, $errstr, $errfile, $errline) { if ($errno === E_RECOVERABLE_ERROR) { throw new ErrorException($errstr, $errno, 0, $errfile, $errline); } return false; });

So in the light of Magento, the default error handler is the global function mageCoreErrorHandler in app/code/core/Mage/Core/functions.php. It get's registered via Mage::app() by the init() method of the Mage_Core_Model_App (app/code/core/Mage/Core/Model/App.php) (via protected _initEnvironment() method).

An observer on controller_front_init_before which registers your own PHP error handler on top should suffice then (error handlers in PHP are stackable):

$previous = set_error_handler(function($errno, $errstr, $errfile, $errline) use (&$previous) { if ($errno === E_RECOVERABLE_ERROR) { throw new ErrorException($errstr, $errno, 0, $errfile, $errline); } if ($previous) { return call_user_func($previous, $errno, $errstr, $errfile, $errline); } return false; });

catchable fatal errors then are turned into exceptions and you can deal with them in your own extension code or they are uncaught and will be seen in the exception log (instead of having your shop run gaga on wrong types like the current behaviour is, dead programs don't lie). In PHP 7 the exception to look for isn't ErrorException then but TypeException (which is a BaseException) for the now catchable fatal errors.

All other errors are passed on to the error handler of Magento.

Note: I have not tried this, it's a write-up but I know the problem you're asking about and the error handling analysis has been done against 1.5.1.0 and verified against 1.9.1.0 through code analysis. The error handler stacking should work. I append a little extended example code that demonstrates most parts working.

I have not yet packaged this as a magento extension but it should be straight forward with modman. I'll put it on github then.

Appendix: Error Handler Demo

The following code-example (online demo) demonstrates the stacking of error handlers and exception throwing on catchable fatal error:

<?php /** * error handler demonstration * * stackable error handle with previous call and catchable error exceptions * * @author hakre <http://hakre.wordpress.com> * @link https://magento.stackexchange.com/a/64972/4115 */ set_error_handler(function() { $args = func_get_args(); var_dump("me is the previous error handler", $args); }); $previous = set_error_handler(function($errno, $errstr, $errfile, $errline) use (&$previous) { if ($errno === E_RECOVERABLE_ERROR) { throw new ErrorException($errstr, $errno, 0, $errfile, $errline); } if ($previous) { return call_user_func($previous, $errno, $errstr, $errfile, $errline); } return false; }); $test = function(callable $test) {}; $a = $undefined; // provoke little warning $test(new stdClass); // provoke catchable fatal error

Program Output

string(32) "me is the previous error handler" array(4) { [0]=> int(8) [1]=> string(29) "Undefined variable: undefined" [2]=> string(45) "/tmp/execpad-0eca072b619d/source-0eca072b619d" [3]=> int(28) } Fatal error: Uncaught exception 'ErrorException' with message 'Argument 1 passed to {closure}() must be callable, object given, called in /tmp/execpad-0eca072b619d/source-0eca072b619d on line 30 and defined' in /tmp/execpad-0eca072b619d/source-0eca072b619d:26 Stack trace: #0 /tmp/execpad-0eca072b619d/source-0eca072b619d(26): {closure}(4096, 'Argument 1 pass...', '/tmp/execpad-0e...', 26, Array) #1 /tmp/execpad-0eca072b619d/source-0eca072b619d(30): {closure}(Object(stdClass)) #2 {main} thrown in /tmp/execpad-0eca072b619d/source-0eca072b619d on line 26
Improve this answer
2
  • Excellent write-up. During your testing, was there a measurable performance degradation from re-setting the error handler? Commented May 12, 2015 at 15:46
  • I haven't so far. There is also a related area in core where on development mode all warnings / errors are converted to Exception (and not ErrorExceptuion - not even logged). This perhaps requires a patchset to fix this in a sane manner. For the error handler, there is no good dispatch method available, also here I somehow tend to patch core even to bring a fixed default error handler in. Commented May 12, 2015 at 15:58
1

It is already handled by default PHP by adding (Exception $e) in the function parameter definition.

You cannot pass anything else to this function than a Exception or extend of Exception.

Improve this answer
5
  • Take a look at the mageCoreErrorHandler function. An error triggered by incorrect params will be handled and and suppressed in non-developer mode, and throw an Exception in developer mode. Commented Mar 13, 2015 at 16:55
  • Something is seriously wrong when such happens in the first place. Magento has the mageCoreErrorHandler to be sure that visitors won't get a error thrown in their face. You could build a own try{}catch(){} to grab them yourself, and if you cannot pass them on. Commented Mar 13, 2015 at 17:14
  • Considering no exceptions are thrown in the case of a suppressed catachable fatal error, what would the try/catch get me? Commented Mar 13, 2015 at 17:47
  • 1
    I'm finally getting it, after a local test... You're very right, the error is suppressed and the code will continue. I'll update my answer and add some extra thoughts Commented Mar 13, 2015 at 18:46
  • I'll post a new answer otherwise our conversation makes no sense at all Commented Mar 13, 2015 at 18:50

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.