Re: updated docs for Error_Raise

Greg Beaver wrote:

Alan Knowles wrote:

which would mean a full on version looking like.. - PEAR:raiseError(

    "file %s was not found in path %s",  // beginners level..
    ERR_NO, // reasonable quality essentail..
    array(  //options - if 3rd arg is array .. assume new format.
        'args'=>  array($file,$path),
        'package'=>'MyPackage',
        'callback'=>array($object,'method'),
        'trigger' => E_WARN,
        'actionType' => PEAR_ERROR_DIE,
        'errorType' => 'warning'
            // default applied by error::warning()
           )

);

This looks fine for an upgrade to raiseError(). It is a BC break though.

Not really.. - it should be posible to support the old format, as arg3 used to be a int, where as the new format uses an array.. !is_array() = old formant..

The solution I'm proposing actually would maintain 100% BC with PEAR_Error as raiseError() would allow the setting of a default error message - note that if there IS a default error message (it is not ''), the Error_Raise_Error->getMessage() method simply returns parent::getMessage(). So, if someone simply uses PEAR::raiseError("error message"), then it would still work - error message returned as normal. I have a serious problem with PEAR_Error that has little to do with your example, but affects it. PEAR_Error mixes error raising and error handling. This is not a good idea. Whether a callback should be used, an error triggered, or thrown is a feature of error handling that should be hidden from the package that raises an error. The only responsibility a package should have is to indicate that there is an error condition, and let other code handle the error. With this amendment, I'd like to see only 'args', 'package', and 'errorType' in the options array, and require other methods like setErrorHandling() to take care of the error handling features. Allowing error handling insertion in raiseError() was a mistake

100% agree with this.....

What I really can't understand is why this should be forced on new code - this is why I chose new method names. Instead of overriding raiseError(), there are the new methods warning, error, notice, exception, and these methods are designed for use by those who wish to use advanced error handling. I don't think the core should force people to do anything other than provide an error code and a package name.

I would suspect PEAR is now stuck with supporting the old (annoying format) for life :)... - downside to BC... - wishfull thinking, to remove it.. Package name handling would be handled far easier by wrapping raiseError - it's something a few of the packages do already for other reasons. eg. class MyMainClass { ... the body of code... function raiseError($msg,$code,$options) { $options['package']='myPackage'; // set the default error type.. $options['errorType']= (isset($options['errorType']) ? $options['errorType'] : 'notice'; // if $msg=null - then look up standard error messages here????? return PEAR::raiseError($msg,$code,$options); } } /// inside the package... MyMainClass::raiseError(null,MYCLASS_INVALID_ARGS,); MyMainClass::raiseError('Invalid args: you sent %s,%s',

       MYCLASS_INVALID_ARGS,
       array(
               'args'=>array($a,$b),
               'errorType'=>'notice'));

This makes a complete disassociation between error definition and message.. - when you are writing the real message you have to cross reference the error location... to work out what the error was supposed to say.. - It's a penalty I am beginning to find in a number of places where I've abstracted stuff to far recently.. (look up 3 files to find what was variables/etc. to use..)

I know this is starting to feel like a tennis match, but I do have a counter to your reply :)

It seems to me that this couldn't happen - you find a situation that needs to return a new error. First, you go to the single place that you define your error codes, and define a new code, then you add an error message to the callback handler, then you return to programming. Later on, another similar situation pops up. You go look at the single place that defined error codes, and read the code and its documentation (what error condition does this code cover?) If error messages and error codes *aren't* defined in the same location, how do you find them at all? Error_Raise encourages developers to define all of their error constants and place their error callback right next to one another. Using error messages in the source scattered about seems to be what you're suggesting, perhaps I misinterpreted? This would seem to introduce several critical problems: -the same error message must be typed and maintained in several locations that return the same error - typos are a serious issue then. -if you only know the code, and want to change the message returned, you can't look at the place where a code was defined, you have to find the place that uses the code in the source - and change every single occurence. With an error message generator, there is 1->1 code to message, allowing both an easier time of finding the error code and error message.

I think this approach is probably one of those personal preference ones... From what I've seen/used of gnome/gtk/php all of which currently use error strings in the text.. (gtk/gnome using gettext in g_warning etc.) - it's never really let me down (yet....) I've saw MDB yesterday, which showed justification for not using error strings (as each driver should to some degree produce the same set of drivers).. - but these factory pattern packages are less than 10% of PEAR's packages.

I think the main thing that is clearly needed is that: a) PEAR needs to require error codes, referenced by constant name, and not by number (PEAR::raiseError(6) is a disastrous idea)

YES

b) you can't require error codes to be referenced by constant name without also requiring the originating package of the error code.

YES

c) adding in the getPackage(), getErrorType() methods to PEAR_Error will enable complex error handling.

YES..

If you don't think Error_Raise has a place in PEAR_Error, I wonder if it might be bundled as a separate PFC with the changes above made to PEAR_Error. Then, PEAR_Error would be forward-compatible, and Error_Raise would be backward-compatible. As a last resort, I can see an optional default error message being provided in case error generation registration doesn't work as a 4th parameter. Error_Raise::error('package', CODE, array('param' => 'thing'), "This %param% wasn't a 'that'");

I've shown above that by using a raiseError wrapper in the main class, you dont need to keep typing 'package', each time.. - I agree its needed.. - but should it be an argument, rather than a default option on the packages raiseError wrapper...??

Would this satisfy your needs?

You know me, never satisfied :) Regards Alan

Greg