make:entity: public properties instead of private + accessors

[alcaeus]

I'm creating this issue to trigger a discussion around how we want the maker bundle to generate entities. Until now, entities always store private properties and add accessors to manipulate the values. While this provides a fluent interface for setters, it's also somewhat of an anti-pattern, especially in modern PHP. There were multiple reasons why one would not want to expose properties in classes, instead using getters and various modifier methods:

• accessors needing to contain logic, e.g. validation
• asymmetric access (e.g. for the identifier which does not have a setter)
• more expressive methods, e.g. addSomething and removeSomething with typed parameters instead of working through the generic collection
• future-proofing, i.e. "let me add setters and getters in case I need to add logic later"

While all this may have been a best practice in the past, there are multiple reasons why we no longer need to do this:

• With property hooks, we can transparently add logic to property access after the fact, negating the need for dedicated setters and getters
• Asymmetric visibility is supported on the property level, negating the need for getters for unmodifiable properties
• Refactoring tools can be used instead of future-proofing in cases where property access needs to be changed to calling a method

My suggestion would be to change the maker to no longer generate setters and getters for properties, but since this is quite a big change, I wanted to get some input on this first. There are also a few special cases that might need discussing. I've prototyped some changes locally, and here's what I found so far: we can relatively easily remove accessors for most properties and make them public. This doesn't necessarily create a BC break but we may want to make the behaviour opt-in. We would have to bump the PHP requirement to PHP 8.4, as the identifier would need to have a private(set) modifier to replicate current behaviour. Where it gets tricky are the various relationships. The current methods handle things like updating the inverse side of a relationship (i.e. calling setOwner on $child when $owner->addChild($child) is called). This can't be done through property hooks, but would have to be handled in the corresponding collection instance. The problem is that this instance may not yet be aware of this requirement, as only a PersistentCollection knows about mappings and inverse sides, while the ArrayCollection created when the entity instance is created outside of Doctrine has no idea of mappings or anything. Last but not least, we would still want a private(set) modifier for collection properties by default to ensure that the collection instance itself can't be replaced, as that may trigger really unwanted behaviour.

I would create a pull request for changing "normal" properties (i.e. identifier and anything that isn't an association) to public and omitting setters and getters, but would like to know if we should make this the new default behaviour or whether we should add some kind of BC layer. For associations, I'm appreciative of any input people have with how these should behave by default.

[jennevdmeer]

How are collections with the form component? I'm not sure, but I thought they required some of the set/get/add/remove methods for it to work at all?

[stof]

For the inverse side of relations, where updating the owning side is required, I think it is a good reason to keep using methods to add and remove things.

Trying to make the collection hide this automatically would be a bad idea IMO, as the way to update the owning side can require extra logic to ensure both sides are in sync, which is kinda complex, while the actual recommendation for simplicity is to favor using unidirectional relations when possible anyway. Thus, it would be a lot more complex as it would require using a special collection also for the non-persitent-yet case to offer the same API.
To me, the proper API for the inverse side of a relation (if you really need a bidirectional relation) is to have a getter with a return type of ReadableCollection (and not Collection, so that at least static analyzers report issues if you try to mutate the collection directly, telling you this method is not part of the ReadableCollection interface) and the add and remove methods.

[bpolaszek]

My 2 cents: For most of the projects I worked on during the last 5 years, I dropped accessors on entities in favor of public properties, and I didn't wait for property hooks to land. I only use private+accessors when some logic is required. The underlying PropertyAccess component (or whatever Doctrine uses) allows a seamless change from public prop to private + accessors. The efficiency of nowadays' static analysis tools will immediately spot where you have to change your code whenever such refactoring occurs.

Dropping accessors have several advantages:

• Entities are drastically more readable, because 3x less LOC
• Accessors that actually perform some logic are immediately visible
• Every time you add an accessor, your coverage complains. Ofc you could codeCoverageIgnore your entities, but what if you put some logic in some accessors? Then they're not covered. By using accessors only when you require some logic, you can still cover your entities.
• Accessors were a good practice on PHP5-7 to ensure you could not set a Bar object on a $foo property, prior to PHP 7.4. On modern PHP applications, properties are fully typed, so this use case is no longer relevant.

Drawbacks:

• No more fluent interface (some consider it an anti-pattern anyway)
• On PHP < 8.4 your $id is writeable from the outside (do you really care?)

I'm now back to a project where all entities have accessors and it feels like a complete back-to-2010 regression.

So yeah, I dream of being able to use make:entity again without all these useless methods! 😃