From http://www.whitewashing.de/2012/08/22/building_an_object_model__no_setters_allowed.html,
It says that getters and setters violate Open/Closed Principle
But, seeing from http://www.sitepoint.com/building-a-domain-model/ , it is encouraged to have getters and setters.
And now, I’m confused to use getter/setter or not.
Any advice ?
From a personal standpoint I would take setters and getters over magic methods any day of the week.
No, I’m not talking about magic methods…
I’m talking about to use it or not.
I think the author is wrong about that. The O/C principle says that a class’s source should be closed to modification, not its runtime state.
Normally, I would too, but the methods for magic getting and setting in the Sitepoint article are just a way to simplify having to write out all those get and set methods. I don’t think that is a bad thing. It avoids clunky entities with tons of getters and setters classes. Something Benjamin Eberlei also points out could be a way to reduce them (but not get rid of them).
Alex also gives good advice in the comments of his post:
As a rule of thumb: use PHP magic with caution, and make your class properties always protected/private.
I am not sure that is Benjamin Eberlei’s point. I think it is more about the “plain get and set methods” used in the FOSUserBundle’s code and them being “too direct” (for lack of a better explanation) and lacking extensibility. But, what Ben fails to make clear is the fact that the FOSUserBundle User class he links to is supposed to be extended or reused. In other words, with the FOSUB User class, we are not really at the last extended version (or shouldn’t be). Yes you could use it, but that is not the point.
What Ben creates as “proper methods” to avoid the getters and setters are what you should see in a final User class. So, I believe he isn’t correcting the User class to not have get and set methods, he is extending the user class properly, in order to use the User object properly.
I think the real point is, you shouldn’t allow the client code (the code finally using the domain objects) to access getters and setters directly, which is what the SitePoint article and Ben both try to detail and thus, in essence, both are making the same point, just in different ways.
If you notice in the Sitepoint client code at the end, you never see “setX” or “getX” anywhere. This is what is wanted. If you want a user name from a blog post comment, you just have
$comment->user->name;
That is my humble, possible totally wrong and relatively noob like OOP point of view on this. 
Scott
To me, this is the main take away from the whitewashing article:
class Post
public function publish(\DateTime $onDate = null)
{
$this->state = "published";
$this->publishDate = $onDate ?: new \DateTime("now");
}
}
Traditionally you might have a getState/setState, getPublishDate/setPublishDate, maybe even getPublishedBy,setPublishedBy etc. These are the sorts of getter/setters that can and should be avoided.
This is also where you can start to think in terms of value objects. All the publish information can be moved to it’s own stand alone object even if ultimately the information ends up being stored in a post table.
Yup, I think ahundiak is on the right track. Traditionally in OO, a class is responsible for keeping its private data in a valid state. If it was possible for a Post object to have a published date but not be in a published state, then that would be invalid.
But there’s a trade-off. We would end up using exceptions for simple validation. For example:
class User
{
// ...
public function setEmail($email)
{
// Ensure this object is always valid
if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
throw new Exception('Bad email.');
}
$this->email = $email;
}
Plus, there’s usually more validation to be done than checking individual fields, and that other validation would have to be managed in a different way, making the task of validation less simple and centralized.
I can’t say exactly when this happened, but we collectively seemed to decide to treat entity objects more like a bag of data, as though all the fields were public. We can plug in whatever data we want, then later validate the state of that data.
Sometimes being pragmatic means sacrificing OO purity.
I’m not sure if I agree with this or not. Your email example is a perfect one where a setter should be used.
One glaring omission from PHP, Java and most of the big languages is the Uniform Access Principle ( http://en.wikipedia.org/wiki/Uniform_access_principle ) I totally agree with the sentiment behind this. Someone using the class should not be concerned whether it’s a property or a computed value (In java we have arraylist.size() and array.length, for example! Even ignoring the inconsistent naming, why is one a method and the other a value?)
Given the above, I’d prefer to see a class that does use getters/setters than public properties as it’s the only way to implement uniform access. Public properties are particularly harmful because if you have this in the code:
$total = $product->price * $quantity;
And suddenly the product price needs to be calculated from a value, e.g. including tax then you have to find all references to $product->price and update it to $product->getPrice(). Where it’s even worse is where you have polymorphism and one implementation of product uses a method with a calculation and one uses a public property then suddenly they’re not polymorphic.
Finally, getters can be enforced with an interface, public properties cannot.
edit: I actually think magic is a nice way of doing this: $comment->user->name; if we’re using __get() then $comment->user is a nice clean API and it allows it to be a computed value or a fixed property, you could even design a trait for exactly this:
trait UniformAccess {
public function __get($value) {
if (method_exists($this, 'get' . $value)) return call_user_func([$this, 'get' . $value]);
else if (property_exists($this, $value)) return $this->$value;
else throw new \Exception('Invalid property ' . get_class($this) . '->' . $value);
}
}
Phew… you saved my day with your edit! 
And to be honest, I didn’t realize what I had said is dependent on the magic functions. But, I do agree, that kind of API is cleaner. Thanks for pointing that out Tom.
Scott
In principle I agree, but let’s make sure we’re well aware of the trade-offs. Either the way we do validation wouldn’t be consistent, and a large chunk of it would be done by trying to set values and catch exceptions. Or, we’d have to duplicate a lot of validation logic, once in the validator and once in the setter.
I am now back to confused mode. If I have validated once in the validator, why would I have to validate again in the setter?
Scott
I think what Jeff is referring to is that it’s difficult to do all validation in the setter. In that example the “sorry, that email address is already registered” validation couldn’t really be done in the setter. Which then means that you have validation happening in two places.
Yes, thinking along the lines of SRP, having validation in a setter would probably be breaking SRP for the class with the setX method in it. Wouldn’t it?
Scott
If we are using the “reason to change” basis for SRP then if the class properties change then so does the validation. By this measure, validation should be done inside the class. Not only that, performing validation outside the class breaks encapsulation.
That said, it probably comes down to how tightly coupled the validation is to the class. Will the same rules apply everywhere the class is used?
I mean duplicated code, not duplicated runtime effort.
In principle, the setter should do its own validation, because the class is responsible for keeping its private state valid. But, if we don’t want to run a bunch of try/catches when doing form validation, or if we want all form validation to happen in a consistent way (not all of it is as simple as checking a single field’s syntax), then our form validator would need to duplicate some validation logic so that it wouldn’t have to rely on the User class throwing exceptions.
I’m just throwing this out there and haven’t properly thought it through, but what about an IoC approach:
$user = new User(new UserValidator);
The validator can then have its own dependencies for things like That email address is already registered and it allows the validation to be different in different scenarios.
I prefer the clean API __get and __set make possible, but I will concede when magic methods need debugging they can be painful. Proper unit testing takes care of that though.
Going the other way with setters also makes sense, but with them you only check to see if the set method exists - if it does not you assume the property is meant to be read only and throw the appropriate exception.
You mean the property is private to the class (encapsulated) and not readable, don’t you? It is the same as done in the Sitepoint article.
/**
* Map the setting of non-existing fields to a mutator when
* possible, otherwise use the matching field
*/
public function __set($name, $value) {
$field = "_" . strtolower($name);
if (!property_exists($this, $field)) {
throw new InvalidArgumentException(
"Setting the field '$field' is not valid for this entity.");
}
$mutator = "set" . ucfirst(strtolower($name));
if (method_exists($this, $mutator) &&
is_callable(array($this, $mutator))) {
$this->$mutator($value)
}
else {
$this->$field = $value;
}
return $this;
}
Something else I just noticed from the PHP docs for __get() and __set(). In the example code, they use an associative array for storing overloaded property data in the class. Is that normal practice? Wouldn’t that mean client code could basically add their own properties to the class at will? Is that a good things to do?
Scott