Skip to content

[BC] Allow more changes when the class/method is final #7611

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

Merged
merged 4 commits into from
Oct 29, 2017
Merged

[BC] Allow more changes when the class/method is final #7611

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a5f6d24
[BC] Allow more changes when the class/method is final
GuilhemN Mar 11, 2017
4a3fbed
Fix comments
GuilhemN Mar 13, 2017
936200c
Update
GuilhemN Mar 14, 2017
f5b8dd4
Improve notes
GuilhemN Mar 15, 2017
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
44 changes: 25 additions & 19 deletions contributing/code/bc.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ Symfony's classes:
Type of Change Change Allowed
================================================== ==============
Remove entirely No
Make final No
Make final No [6]_
Make abstract No
Change name or namespace No
Change parent class Yes [4]_
Expand All @@ -194,8 +194,8 @@ Reduce visibility No
Move to parent class Yes
**Protected Properties**
Add protected property Yes
Remove protected property No
Reduce visibility No
Remove protected property No [7]_
Reduce visibility No [7]_
Move to parent class Yes
**Private Properties**
Add private property Yes
Expand All @@ -204,7 +204,7 @@ Remove private property Yes
Add constructor without mandatory arguments Yes [1]_
Remove constructor No
Reduce visibility of a public constructor No
Reduce visibility of a protected constructor No
Reduce visibility of a protected constructor No [7]_
Move to parent class Yes
**Public Methods**
Add public method Yes
Expand All @@ -213,29 +213,29 @@ Change name No
Reduce visibility No
Move to parent class Yes
Add argument without a default value No
Add argument with a default value No
Add argument with a default value No [7]_ [8]_
Remove argument Yes [3]_
Add default value to an argument No
Add default value to an argument No [7]_ [8]_
Remove default value of an argument No
Add type hint to an argument No
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, should be possible when final

Copy link
Contributor Author

@GuilhemN GuilhemN Mar 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so, if someone calls the method he expects that everything will work as the argument if there is no type hint. If you add a type hint then not everything will work anymore. I think this change needs a bc layer.

Copy link
Contributor

Choose a reason for hiding this comment

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

The changes are always under the premise that the behavior is BC. This also counts for the things like "Removing an argument"

Copy link
Contributor Author

@GuilhemN GuilhemN Mar 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, that's a good point. Then we should also allow Change argument type and Change return type for final methods, right?

Copy link
Member

Choose a reason for hiding this comment

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

changes should only be allowed for parent types, not siblings nor children

Remove type hint of an argument No
Remove type hint of an argument No [7]_ [8]_
Change argument type No
Change return type No
**Protected Methods**
Add protected method Yes
Remove protected method No
Change name No
Reduce visibility No
Remove protected method No [7]_
Change name No [7]_
Reduce visibility No [7]_
Copy link
Member

@xabbuh xabbuh Mar 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

[8] applies to all three above too, doesn't it? nope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No because having a final protected method doesn't prevent people extending the class from calling the method.

Move to parent class Yes
Add argument without a default value No
Add argument with a default value No
Add argument without a default value No [7]_
Copy link
Member

Choose a reason for hiding this comment

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

this looks wrong to me

Copy link
Contributor Author

@GuilhemN GuilhemN Mar 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I updated the notes to separate final classes and final methods.
A protected method in a final class is like a private method so this change is allowed, but it is not for a final method, so it should be ok now.

Thanks for your comments!

Add argument with a default value No [7]_ [8]_
Remove argument Yes [3]_
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
Add default value to an argument No [7]_ [8]_
Remove default value of an argument No [7]_
Copy link
Member

Choose a reason for hiding this comment

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

This change looks wrong to me. Removing a default argument influences not only child classes, but also changes the way a consumer can call the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍, I'll change that asap (editing the file on my phone changed the spaces).

Add type hint to an argument No [7]_
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be allowed if the method is final because if you cannot extend the method, the added typehint (that is compatible of course) won't do harm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by is compatible? We can't allow this change in all cases so imo we should not change that.

Copy link
Member

Choose a reason for hiding this comment

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

eg if the code throws after an instanceof check, then it's ok to add the type hint to me also
string also when scalar is checked
may need a few words thought to make clear what's possible (would adding "as long as it preserves BC" be enough?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the notes to:

Allowed if the class is final. Classes that received the ``@final``
annotation after their first release are considered final in their
next major version.
Changing an argument type is only possible with a parent type.
Changing a return type is only possible with a child type.

Is it ok for you?

Remove type hint of an argument No [7]_ [8]_
Change argument type No [7]_
Change return type No [7]_
Copy link
Member

Choose a reason for hiding this comment

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

The changes above would IMO need some clarification (not all changes are allowed, but only those that are compatible with how the code was used before, i.e. widen a type-hint for example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed you're right, do you think widening a type hint is worth being explained or should we just not have a note here?

**Private Methods**
Add private method Yes
Remove private method Yes
Expand All @@ -250,7 +250,7 @@ Remove type hint of an argument Yes
Change argument type Yes
Change return type Yes
**Static Methods**
Turn non static into static No
Turn non static into static No [7]_ [8]_
Turn static into non static No
**Constants**
Add constant Yes
Expand All @@ -277,6 +277,12 @@ Change value of a constant Yes [1]_ [5]_
Additionally, if a constant will likely be used in objects that are
serialized, the value of a constant should not be changed.

.. [6] Allowed using the ``@final`` annotation.

.. [7] Allowed if the class is final.
Copy link
Member

Choose a reason for hiding this comment

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

or when the class has the @final annotation since the previous major version (same below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we can call that being final, don't you think? 😛

Copy link
Member

@nicolas-grekas nicolas-grekas Mar 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

what I think doesn't matter: what the next reader will understand does :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact I don't see how we could clearly explain that the @final annotation means that the class/method is final but only if it is present since the previous major version, I think it is clear enough as is.

Copy link
Member

Choose a reason for hiding this comment

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

this doc is here do make things explicit, I think even if it's clear for us, I'm sure it's not for someone not used to our processes. since we always add the version were it was made final, I don't see the issue (except the "wording" one of course, but that's solvable :)


.. [8] Allowed if the method is final.

.. _Semantic Versioning: http://semver.org/
.. _scalar type: http://php.net/manual/en/function.is-scalar.php
.. _boolean values: http://php.net/manual/en/function.boolval.php
Expand Down