jsonrpc icon indicating copy to clipboard operation
jsonrpc copied to clipboard

Please use "protected" instead of "private"

Open romaninsh opened this issue 11 years ago • 3 comments

This way we can actually extend this and enhance if we wish! I cleaned up the server class, but the same thing probably applies to other classes. It's really bad practice to make un-extendable classes.

I wanted to extend this and add custom error logging (so that they end up inside error) and this is only possible with this change.

romaninsh avatar May 17 '13 16:05 romaninsh

Quick question: could you describe what your custom error logging looks like.

Thanks

johnstevenson avatar May 17 '13 19:05 johnstevenson

// sorry for slow response.

My code generates two type of exceptions

  • exceptions which must be sent to the client as errors for particular request
  • exceptions which must terminate any further requests.

In the framework I use there is a standard way for recording and logging exceptions. The API transport should remain flexible to allow intercepting the exceptions and then processing them. To avoid human-error we want this done on the method-calling level.

The best way to implement is to extend your class and override certain methods. Extending classes and overriding is a basic principle of object oriented programming. By marking methods / properties as private or marking classes as final you make your code un-usable. It is generally a good idea to keep things "protected" for added extensibility.

Below is the example of how can I enhance the logging logic if the methods are "protected"

class JsonRpcServer extends JsonRpc\Server {

    protected function logException(Exception $e){
        if($e instanceof BaseException){
            // Logic Exception
            $this->error = $e->getMessage();
        }else return parent::logException($e);
    }
}

Further - I'll need to implement API authentication methods and would want to keep use your library, but I should be able to extend your class.

romaninsh avatar Aug 05 '13 22:08 romaninsh

Is there anything preventing this from being merged? I'd love to be able to extend this library.

erikwiffin avatar Aug 11 '14 21:08 erikwiffin