Package home | Report new bug | New search | Development Roadmap Status: Open | Feedback | All | Closed Since Version 2.3.2

Request #18401 Consider swapping to exceptions, not trigger_error
Submitted: 2011-03-27 11:12 UTC
From: doconnor Assigned:
Status: Bogus Package: HTML_QuickForm2 (version 0.5.0)
PHP Version: Irrelevant OS:
Roadmaps: (Not assigned)    
Subscription  


 [2011-03-27 11:12 UTC] doconnor (Daniel O'Connor)
Description: ------------ In HTML_QuickForm2_Container::__call(), trigger error is used. http://svn.php.net/viewvc/pear/packages/HTML_QuickForm2/t runk/HTML/QuickForm2/Container.php?view=markup#l432 If you manage to trigger said error, you get a fatal error about an undefined method (good); which points you at the trigger_error() method in __call() - not so helpful. Without an extension like xdebug; it becomes tricky for you to work out where you made the mistake; and it looks like a fault of HTML_Quickform2_Container An exception or trigger_error working out where it was called from via debug_backtrace() would be much more helpful. Test script: --------------- <?php require_once 'HTML/QuickForm2.php'; $form = new HTML_QuickForm2(); $form->toArray(); // an unported quickform 1 method Expected result: ---------------- An exception or fatal error which points at line 3. Actual result: -------------- A fatal error along the lines of Cannot call undefined method HTML_QuickForm2::toArray() called from HTML_QuickForm2_Container; line 443 (which is misleading)

Comments

 [2011-03-27 12:54 UTC] mansion (Bertrand Mansion)
-Status: Open +Status: Bogus
Thank you for taking the time to write to us, but this is not a bug. Calling an undefined method is an error in your code and it should be handled as such. If we throw an exception instead, ok you get a nice backtrace with it, but exceptions are made to be caught, I see no reason why you would want to catch an undefined method exception because it's obviously an error in your code. There are different ways to handle user errors, you can write your own error handler that will convert user errors to exceptions if you need a backtrace, or simply use an error handler that prints a backtrace. A lot of other projects do it like us (even MDB2).
 [2011-03-27 13:12 UTC] doconnor (Daniel O'Connor)
The point is that it's not actually obviously an error in my code: the generated error message looks like an internal HTML_Quickform2 error; as it lacks info on where __call was invoked from.
 [2011-03-27 15:33 UTC] mansion (Bertrand Mansion)
I consider calling toArray() on a class that don't have this method defined, an error in your code. I also agree that not having the correct line and file information in the original error report is annoying, but there are ways to get the backtrace (xdebug does it for you by default for example), so this is a minor annoyance, usually you know where the call to toArray() was done in your code.
 [2011-03-27 18:54 UTC] dufuz (Helgi Þormar Þorbjörnsson)
A side note, referencing how MDB2 does it (among other things) seems a bit faulty; MDB2 is a PHP 4 package and does things the way it does because it couldn't make use of PHP 5 features.
 [2011-03-27 19:14 UTC] mansion (Bertrand Mansion)
The question is: what is the expected behavior when you call a non existent method on an object ? This is not said in the PHP manual, but there is what I consider a hint in the documentation of the __get() method. What if we do the same and use the backtrace to get correct information about where the error happened first ? http://www.php.net/manual/en/language.oop5.overloading.php
 [2011-03-27 20:52 UTC] dufuz (Helgi Þormar Þorbjörnsson)
I agree, the manual isn't very clear but we need to consider that __get and __call are inherently different. I would fully expect a notice from __get failing because that's what happens when I do the same thing on array keys that don't exist, where as when I call a method that does not exist PHP gives me a fatal error. If you want to insist on using trigger_error then I recommend a proper PHP Fatal as I'm calling a function which does no longer exists, just to stay in line with what PHP does - Notice is more of a "hey, something is wrong but we can let it slide because everything else can continue running in theory". If you feel like you want to use an exception, perhaps have a look at using either of these built in ones: http://php.net/manual/en/class.badmethodcallexception.php http://php.net/manual/en/class.badfunctioncallexception.php What do you guys think of either one of those solutions? I'm thinking this from the perspective of other packages as well, in case they encountered a similar situation :)
 [2011-03-28 00:59 UTC] mansion (Bertrand Mansion)
We already trigger a E_USER_ERROR, which is a fatal error, not a notice. The SPL exceptions don't extend PEAR Exception. So I think we keep it like it is and use the backtrace to set more precisely where the error happened first, as shown in the PHP manual example for __get().