Mark method from abstract classes as final #2194
Conversation
|
Not sure what kind of label or changelog is needed for this change. @VincentLanglet wdyt? |
|
I will take care of this merge to 4.x, it will be a mess but I can do it. |
IMO, these changes require a MINOR release, and the changelog label should be "Changed", since these changes are marking existing methods as final. |
|
Additionally, I think this PR also deserves an upgrade note, since these docblocks generate deprecation messages if these methods are overridden. |
Added. |
| @@ -62,51 +62,81 @@ public function __toString() | |||
| return $this->getName() ?? '-'; | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
We should not do this for model classes because they are more likely to be customised and overridden.
There was a problem hiding this comment.
I dont know what kind of customization is needed for a getter and a setter, can you provide an example?
There was a problem hiding this comment.
Also, if some user needs to customize the methods, he can create a class from the Interfaces, no need to use this Model.
There was a problem hiding this comment.
Or at least open an issue with an explanation of the needs, and we can still remove the final if we find it's justified.
It's easier to add final now because adding final is not BC but removing final is.
There was a problem hiding this comment.
e.g. you want to update the updatedAt or enabled state when setting some property.
Also, if some user needs to customize the methods, he can create a class from the Interfaces, no need to use this Model.
Yes you could create some custom class, but that's not the idea of reusable models. With the same logic we could make the entity classes final.
There was a problem hiding this comment.
As @VincentLanglet said, what about making them final and if someone complains on a concrete use case, we can change it because it will be BC. For your example, using the prePersist could solve the issue (and if you really need to modify all the methods, you might as well create from the interface directly). wdyt?
There was a problem hiding this comment.
By the way we should make the decision consistent between the Classification and the Media bundle about entities
There was a problem hiding this comment.
IMO it should be as we did for any other final TBH, it is final until it can be proven that there is a real need that cannot be solved in other ways.
When checking for final, we should ensure we added it for classes and for abstract classes.
That being said, the more time we spend on this, the more we delay releases and working on other bundles...
There was a problem hiding this comment.
IMO it should be as we did for any other final TBH, it is final until it can be proven that there is a real need that cannot be solved in other ways.
When checking for final, we should ensure we added it for classes and for abstract classes.
I agree
There was a problem hiding this comment.
Still not a fan, but feel to merge this if I'm the only one with this considerations.
jordisala1991
dismissed
core23’s stale review
Lets see if someone raises this concern before having to discard final keyword
Subject
I am targeting this branch, because this is BC.
This change will help with #2191 . There are methods on that PR that are not present on this one, so this PR only focuses on the finals that will be useful for the 4.x version (there is no need to mark something final that in 4.x is removed).
Changelog