Skip to content

Add traits to BC policy #9718

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Add traits to BC policy #9718

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 134 additions & 1 deletion contributing/code/bc.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,41 @@ covered by our backward compatibility promise:
| Access a private property (via Reflection) | No |
+-----------------------------------------------+-----------------------------+

Using our Traits
~~~~~~~~~~~~~~~~

All traits provided by Symfony may be used in your classes.

.. caution::

The exception to this rule are traits tagged with ``@internal``. Such
traits should not be used.

To be on the safe side, check the following table to know which use cases are
covered by our backward compatibility promise:

+-----------------------------------------------+-----------------------------+
| Use Case | Backward Compatibility |
+===============================================+=============================+
| **If you...** | **Then we guarantee BC...** |
+-----------------------------------------------+-----------------------------+
| Use a trait | Yes |
+-----------------------------------------------+-----------------------------+
| **If you use the trait and...** | **Then we guarantee BC...** |
+-----------------------------------------------+-----------------------------+
| Use it to implement an interface | Yes |
+-----------------------------------------------+-----------------------------+
| Use it to implement an abstract method | Yes |
+-----------------------------------------------+-----------------------------+
| Use it to extend a parent class | Yes |
+-----------------------------------------------+-----------------------------+
| Use it to define an abstract class | Yes |
+-----------------------------------------------+-----------------------------+
| Use a public, protected or private property | Yes |
+-----------------------------------------------+-----------------------------+
| Use a public, protected or private method | Yes |
+-----------------------------------------------+-----------------------------+

Working on Symfony Code
-----------------------

Expand Down Expand Up @@ -165,6 +200,9 @@ Add type hint to an argument No
Remove type hint of an argument No
Change argument type No
Change return type No
**Static Methods**
Turn non static into static No
Turn static into non static No
**Constants**
Add constant Yes
Remove constant No
Expand Down Expand Up @@ -196,21 +234,28 @@ Move to parent class Yes
Add protected property Yes
Remove protected property No [7]_
Reduce visibility No [7]_
Make public No [7]_
Move to parent class Yes
**Private Properties**
Add private property Yes
Make public or protected Yes
Remove private property Yes
**Constructors**
Add constructor without mandatory arguments Yes [1]_
Remove constructor No
Reduce visibility of a public constructor No
Reduce visibility of a protected constructor No [7]_
Move to parent class Yes
**Destructors**
Add destructor Yes
Remove destructor No
Move to parent class Yes
**Public Methods**
Add public method Yes
Remove public method No
Change name No
Reduce visibility No
Make final No [6]_
Move to parent class Yes
Add argument without a default value No
Add argument with a default value No [7]_ [8]_
Expand All @@ -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 No [7]_ [8]_
Move to parent class Yes
Add argument without a default value No [7]_
Add argument with a default value No [7]_ [8]_
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since adding a new method is allowed, making a private one public or protected should be allowed

Add argument without a default value Yes
Add argument with a default value Yes
Remove argument Yes
Expand All @@ -249,7 +297,7 @@ Add type hint to an argument Yes
Remove type hint of an argument Yes
Change argument type Yes
Change return type Yes
**Static Methods**
**Static Methods and Properties**
Turn non static into static No [7]_ [8]_
Turn static into non static No
**Constants**
Expand All @@ -258,6 +306,91 @@ Remove constant No
Change value of a constant Yes [1]_ [5]_
================================================== ==============

Changing Traits
~~~~~~~~~~~~~~~

This table tells you which changes you are allowed to do when working on
Symfony's traits:

================================================== ==============
Type of Change Change Allowed
================================================== ==============
Remove entirely No
Change name or namespace No
Use another trait Yes
**Public Properties**
Add public property Yes
Remove public property No
Reduce visibility No
Move to a used trait Yes
**Protected Properties**
Add protected property Yes
Remove protected property No
Reduce visibility No
Make public No
Move to a used trait Yes
**Private Properties**
Add private property Yes
Remove private property No
Make public or protected Yes
Copy link
Contributor

@HeahDude HeahDude Jul 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conflict with user added functions, right? sorry see below

Move to a used trait Yes
**Constructors and destructors**
Have constructor or destructor No
**Public Methods**
Add public method Yes
Remove public method No
Change name No
Reduce visibility No
Make final No [6]_
Move to used trait Yes
Add argument without a default value No
Add argument with a default value No
Remove argument No
Add default value to an argument No
Remove default value of an argument No
Add type hint to an argument No
Remove type hint of an argument No
Change argument type No
Change return type No
**Protected Methods**
Add protected method Yes
Remove protected method No
Change name No
Reduce visibility No
Make final No [6]_
Make public No [8]_
Move to used trait Yes
Add argument without a default value No
Add argument with a default value No
Remove argument No
Add default value to an argument No
Remove default value of an argument No
Add type hint to an argument No
Remove type hint of an argument No
Change argument type No
Change return type No
**Private Methods**
Add private method Yes
Remove private method No
Change name No
Make public or protected Yes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conflicts with user added functions, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same answer as above: since adding a new method is allowed, making a private one public or protected should be allowed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@nicolas-grekas nicolas-grekas Jul 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anything specific to traits?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HeahDude do you think we need that extended explanation in this PR ... or can be done later in a separate PR? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im' not even sure we should at all actually. So definitely not blocking here.

Move to used trait Yes
Add argument without a default value No
Add argument with a default value No
Remove argument No
Add default value to an argument No
Remove default value of an argument No
Add type hint to an argument No
Remove type hint of an argument No
Change argument type No
Add return type No
Remove return type No
Change return type No
**Static Methods and Properties**
Turn non static into static No
Turn static into non static No
================================================== ==============

.. [1] Should be avoided. When done, this change must be documented in the
UPGRADE file.

Expand Down