Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Please use "protected" instead of "private" #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

romaninsh
Copy link

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.

This way we can actually extend this and enhance if we wish!
@johnstevenson
Copy link
Owner

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

Thanks

@romaninsh
Copy link
Author

// 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.

@erikwiffin
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants