search

snowcap / Emarsys

PHP HTTP client for Emarsys webservice
MIT License
39 stars 37 forks source link

send() method visibility changed to protected #13

Closed otzy closed 9 years ago

otzy commented 9 years ago

this allows in the descendent class to add emarsys api methods, which are not implemented in this sdk

tim-bezhashvyly commented 9 years ago

Why not submit a PR for adding a missing Emarsys API method instead? :wink:

otzy commented 9 years ago

well, I'll submit, but still if send() is available you don't need to wait the next release or update composer. Also it's good if you want to override default behavior, for example, throw exception if response is not "OK"

tim-bezhashvyly commented 9 years ago

"Send" is an internal method and I believe making it protected will break an open/closed principe along with a bunch of security practices.

otzy commented 9 years ago

I think this completely conforms that principle. If someone needs something different in this method, he writes a new class were method can be extended. If method is private you need to change the source code of SDK, thus breaking open close principle and being forced to support and develop own fork of the library.

tim-bezhashvyly commented 9 years ago

Making "send" method protected will open it for modification and that it a violation of open/closed principle.

otzy commented 9 years ago

"closed for modifications" means that you as library author are not allowed to change behavior. You can only fix bugs. So applications that are using your class will not be screwed up by the new version. So it's not about private/protected, it's about approach. "open" means that behavior can be changed through inheritance. And now I'm not able to have an advantage of this part of principle :)

Moinax commented 9 years ago

I agree with @otzy on this one. We already argue on this, and I said that I will let it private until someone else find the need to override it. So let it be overridable.