Add traits to BC policy #9718
Add traits to BC policy #9718
Conversation
contributing/code/bc.rst
Outdated
| @@ -196,21 +234,28 @@ Move to parent class Yes | |||
| Add protected property Yes | |||
| Remove protected property No [7]_ | |||
| Reduce visibility No [7]_ | |||
| Make public Yes | |||
There was a problem hiding this comment.
Such a change will break child classes that override the formerly protected property: https://3v4l.org/8vbr3
contributing/code/bc.rst
Outdated
| Move to parent class Yes | ||
| **Private Properties** | ||
| Add private property Yes | ||
| Make public or protected Yes |
contributing/code/bc.rst
Outdated
| @@ -226,6 +271,8 @@ Add protected method Yes | |||
| Remove protected method No [7]_ | |||
| Change name No [7]_ | |||
| Reduce visibility No [7]_ | |||
| Make final No [6]_ | |||
| Make public Yes | |||
| @@ -240,6 +287,7 @@ Change return type No [7]_ [8]_ | |||
| Add private method Yes | |||
| Remove private method Yes | |||
| Change name Yes | |||
| Make public or protected Yes | |||
There was a problem hiding this comment.
since adding a new method is allowed, making a private one public or protected should be allowed
|
What can we do here to overcome the issues pointed by @xabbuh? Thanks! |
|
Thanks @xabbuh, comments addressed. |
| **Private Properties** | ||
| Add private property Yes | ||
| Remove private property No | ||
| Make public or protected Yes |
There was a problem hiding this comment.
This conflict with user added functions, right? sorry see below
| Add private method Yes | ||
| Remove private method No | ||
| Change name No | ||
| Make public or protected Yes |
There was a problem hiding this comment.
This conflicts with user added functions, right?
There was a problem hiding this comment.
same answer as above: since adding a new method is allowed, making a private one public or protected should be allowed
There was a problem hiding this comment.
But if we can can add methods in core whatever their visibility we should not be able to guarantee BC on adding some in user land, I surely miss something.
There was a problem hiding this comment.
We do not actually - or we would be just blocked for any future improvements.
Didn't ever you add any public or protected method to any class already?
There was a problem hiding this comment.
Ok I agree, this is obvious, but I thought that at some point we documented that it was covered.
Maybe we could precise that when using a trait, composition might break when the trait add methods, because as obvious it is, it already caused issues when adding json or file shortcuts in the core. WDYT?
There was a problem hiding this comment.
Is there anything specific to traits?
There was a problem hiding this comment.
No of course, the same goes for extending any class from the core. Maybe we lack a sentence to explicit that it's not possible. Not all new comers have a strong PHP logic helping make that obvious.
There was a problem hiding this comment.
@HeahDude do you think we need that extended explanation in this PR ... or can be done later in a separate PR? Thanks!
There was a problem hiding this comment.
Im' not even sure we should at all actually. So definitely not blocking here.
|
Another one merged! Thanks for all your contributions Nicolas! |
This PR was submitted for the 2.7 branch but it was merged into the 2.8 branch instead (closes #9718). Discussion ---------- Add traits to BC policy Changing traits is more constrained that changing classes. Also embeds some things I found missing in other parts. Commits ------- 3c814db Add traits to BC policy
Changing traits is more constrained that changing classes.
Also embeds some things I found missing in other parts.