Skip to content
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.

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

make internal classes final and mark other classes that will be final in 2.0 #321

Open
wants to merge 2 commits into
base: 1.x
Choose a base branch
from

Conversation

dbu
Copy link
Collaborator

@dbu dbu commented Mar 5, 2019

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Related tickets prepare for #317
Documentation -
License MIT

What's in this PR?

Declaring final all classes that already are marked as @internal. Marking with @final all other classes that should be final. Making them final is technically a BC break, so we do that only in the next major.

@xabbuh
Copy link
Member

xabbuh commented Mar 5, 2019

Have all the internal classes been internal since 1.0? Otherwise making them final would technically be a BC break too.

Collector/Collector.php Outdated Show resolved Hide resolved
@dbu
Copy link
Collaborator Author

dbu commented Mar 5, 2019

good point. the classes have been added in #128 and have been @internal since the beginning.

@dbu dbu force-pushed the mark-final branch 2 times, most recently from 1651c9a to b89618e Compare March 5, 2019 10:52
@dbu dbu mentioned this pull request Mar 13, 2019
@ostrolucky
Copy link
Collaborator

What's the advantage of this? In userspace I might want to extend these classes despite that they are not covered by semver.

@dbu
Copy link
Collaborator Author

dbu commented Nov 18, 2019

the problem with non-final classes is that people will start extending them, and then complain when things break because we change their signature. extending a class can break in so many ways when the base class does not consider BC (method name clashes, changing a method from protected to public in the base class, ...)

@ostrolucky
Copy link
Collaborator

You say that, but it doesn't really happen. Not for internal classes. And when it does, it's easy to quickly close such requests. It's much more likely you will receive requests to remove those finals. Look how many of such requests are in moneyphp repository.

@dbu
Copy link
Collaborator Author

dbu commented Nov 18, 2019

i do remember a few cases where people complained when we broke stuff, and i remember tons of cases where we had huge discussions about breaking BC with changes in such classes.

looking at what i want to make final, its really things about the layout in the debug toolbar. i don't expect people to need to customize that. if they want to provide visual tweaks (like you did in #355) that's awesome and should go into this repo rather than be project specific.

@dbu dbu force-pushed the mark-final branch 2 times, most recently from e05e986 to 022456a Compare November 27, 2019 08:55
@ostrolucky
Copy link
Collaborator

ostrolucky commented Nov 27, 2019

i do remember a few cases where people complained when we broke stuff

In classes marked @internal?

@dbu
Copy link
Collaborator Author

dbu commented Nov 27, 2019

that i don't remember. but what is your goal? would you only soft-mark the classes as internal, and allow people to break the rules and when things break for them, we can tell them "told you so"?

@php-http/owners what is your take on making classes final? i prefer it, but don't have a very strong opinion...

@ostrolucky
Copy link
Collaborator

Yes, @internal, or @final. That is defacto symfony's approach, even though recently they started to break this symfony/symfony#25788

Yes, I don't expect need of extending these classes ever, but software engineering runs into unforeseen change of requirements all the time.

@dbu dbu force-pushed the mark-final branch 2 times, most recently from 8a896e7 to 6a589e2 Compare May 12, 2023 09:29
@dbu dbu changed the base branch from 1.x to 2.x November 18, 2023 08:32
@dbu dbu changed the base branch from 2.x to 1.x November 18, 2023 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants