php-binance-api icon indicating copy to clipboard operation
php-binance-api copied to clipboard

Replace Echo with Exception/error

Open ePascalC opened this issue 3 years ago • 7 comments

To indicate errors/warnings, multiple functions are doing an echo to display output. These should be changed with throw new \Exception('...'); or trigger_error('...')

ePascalC avatar Jun 29 '21 06:06 ePascalC

Excellent proporsal! It would be correct to have such thing. The problem is that, probably it would cause some problems to the people that's not using the corresponding try/catch statements.

ghost avatar Jul 06 '21 01:07 ghost

@sm4rtk1dz Using trigger_error with e.g. E_USER_WARNING would generate a warning without halting the execution, so that should work

ePascalC avatar Jul 06 '21 07:07 ePascalC

I still like the idea of throwing exceptions, it would be very useful. It would be interesting to make it an opt-in option, maybe adding a parameter to the constructor, like $throwExceptions = false by default.

ghost avatar Jul 06 '21 23:07 ghost

@sm4rtk1dz So by that you mean that error handling should be written twice within methods or ... ?

chrisdimas avatar Jul 07 '21 17:07 chrisdimas

To be honest, I think we should leave that to the developer as this class will probably be part of a bigger PHP program. By adding:

ini_set('display_errors', 1);
ini_set('display_startup_errors', 1);
error_reporting(E_ALL);

And eventually in PHP.ini: display_errors = on should be sufficient to see all errors and just use in the code here trigger_error, removing the echo.

ePascalC avatar Jul 07 '21 18:07 ePascalC

@chrisdimas Not twice, a simple if else statement would make it possible. Something like

if($this->throwExceptions)
   throw new ErrorException($message,0);
else trigger_error("Cannot divide by zero", E_USER_ERROR);

this way you have the backward compatibility and the possibility to use exceptions. the previous code could be implemented in a method

public function handleError($message, $code){
  if($this->throwExceptions)
     throw new ErrorException($message,$code);
  else trigger_error($message, E_USER_ERROR);
}

ghost avatar Jul 07 '21 18:07 ghost

To be honest, I think we should leave that to the developer as this class will probably be part of a bigger PHP program. By adding:

ini_set('display_errors', 1);
ini_set('display_startup_errors', 1);
error_reporting(E_ALL);

And eventually in PHP.ini: display_errors = on should be sufficient to see all errors and just use in the code here trigger_error, removing the echo.

This would be a correct solution, but how do you support the error handling? Having the possibility to catch the error and act upon it with a try-catch statement would be a big win for the developer.

ghost avatar Jul 07 '21 18:07 ghost