jaggedsoft / php-binance-api

PHP Binance API is an asynchronous PHP library for the Binance API designed to be easy to use. https://github.com/binance-exchange/php-binance-api
MIT License
605 stars 497 forks source link

Replace Echo with Exception/error #412

Open ePascalC opened 3 years ago

ePascalC commented 3 years ago

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

ghost commented 3 years ago

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.

ePascalC commented 3 years ago

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

ghost commented 3 years ago

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.

chrisdimas commented 3 years ago

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

ePascalC commented 3 years ago

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.

ghost commented 3 years ago

@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 commented 3 years ago

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.